| Message ID | 1654107991-598-5-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Accepted |
| Commit | 915047048f0acd3dbfe8605b854f151815f9be96 |
| Headers | show |
| Series | Add ARMv8 CE sha1/sha256 support | expand |
On Wed, Jun 01, 2022 at 08:26:30PM +0200, Loic Poulain wrote: > Mark sha256_process as weak to allow hardware specific implementation. > Add parameter for supporting multiple blocks processing. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Applied to u-boot/next, thanks!
Hi Loic, On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > > Mark sha256_process as weak to allow hardware specific implementation. > Add parameter for supporting multiple blocks processing. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > lib/sha256.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/lib/sha256.c b/lib/sha256.c > index c1fe93d..50b0b51 100644 > --- a/lib/sha256.c > +++ b/lib/sha256.c > @@ -14,6 +14,8 @@ > #include <watchdog.h> > #include <u-boot/sha256.h> > > +#include <linux/compiler_attributes.h> > + > const uint8_t sha256_der_prefix[SHA256_DER_LEN] = { > 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, > 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, > @@ -55,7 +57,7 @@ void sha256_starts(sha256_context * ctx) > ctx->state[7] = 0x5BE0CD19; > } > > -static void sha256_process(sha256_context *ctx, const uint8_t data[64]) > +static void sha256_process_one(sha256_context *ctx, const uint8_t data[64]) > { > uint32_t temp1, temp2; > uint32_t W[64]; > @@ -186,6 +188,18 @@ static void sha256_process(sha256_context *ctx, const uint8_t data[64]) > ctx->state[7] += H; > } > > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, > + unsigned int blocks) > +{ > + if (!blocks) > + return; > + > + while (blocks--) { > + sha256_process_one(ctx, data); > + data += 64; > + } > +} > + > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > { > uint32_t left, fill; > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > if (left && length >= fill) { > memcpy((void *) (ctx->buffer + left), (void *) input, fill); > - sha256_process(ctx, ctx->buffer); > + sha256_process(ctx, ctx->buffer, 1); > length -= fill; > input += fill; > left = 0; > } > > - while (length >= 64) { > - sha256_process(ctx, input); > - length -= 64; > - input += 64; > - } > + sha256_process(ctx, input, length / 64); > + input += length / 64 * 64; > + length = length % 64; > > if (length) > memcpy((void *) (ctx->buffer + left), (void *) input, length); > -- > 2.7.4 > I just came across this patch as it broke minnowmax. This should be using driver model, not weak functions. Please can you take a look? Regards, Simon
Hi Simon, Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : > Hi Loic, > > On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > Mark sha256_process as weak to allow hardware specific implementation. > > Add parameter for supporting multiple blocks processing. > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > --- > > lib/sha256.c | 26 +++++++++++++++++++------- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/lib/sha256.c b/lib/sha256.c > > index c1fe93d..50b0b51 100644 > > --- a/lib/sha256.c > > +++ b/lib/sha256.c > > @@ -14,6 +14,8 @@ > > #include <watchdog.h> > > #include <u-boot/sha256.h> > > > > +#include <linux/compiler_attributes.h> > > + > > const uint8_t sha256_der_prefix[SHA256_DER_LEN] = { > > 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, > > 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, > > @@ -55,7 +57,7 @@ void sha256_starts(sha256_context * ctx) > > ctx->state[7] = 0x5BE0CD19; > > } > > > > -static void sha256_process(sha256_context *ctx, const uint8_t data[64]) > > +static void sha256_process_one(sha256_context *ctx, const uint8_t > data[64]) > > { > > uint32_t temp1, temp2; > > uint32_t W[64]; > > @@ -186,6 +188,18 @@ static void sha256_process(sha256_context *ctx, > const uint8_t data[64]) > > ctx->state[7] += H; > > } > > > > +__weak void sha256_process(sha256_context *ctx, const unsigned char > *data, > > + unsigned int blocks) > > +{ > > + if (!blocks) > > + return; > > + > > + while (blocks--) { > > + sha256_process_one(ctx, data); > > + data += 64; > > + } > > +} > > + > > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t > length) > > { > > uint32_t left, fill; > > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const > uint8_t *input, uint32_t length) > > > > if (left && length >= fill) { > > memcpy((void *) (ctx->buffer + left), (void *) input, > fill); > > - sha256_process(ctx, ctx->buffer); > > + sha256_process(ctx, ctx->buffer, 1); > > length -= fill; > > input += fill; > > left = 0; > > } > > > > - while (length >= 64) { > > - sha256_process(ctx, input); > > - length -= 64; > > - input += 64; > > - } > > + sha256_process(ctx, input, length / 64); > > + input += length / 64 * 64; > > + length = length % 64; > > > > if (length) > > memcpy((void *) (ctx->buffer + left), (void *) input, > length); > > -- > > 2.7.4 > > > > I just came across this patch as it broke minnowmax. Ok, is it a build time or runtime break? > > This should be using driver model, not weak functions. Please can you > take a look? Yes I can look at it in the next few days. I have used weak function because it’s an architecture feature offered by armv8 instructions, It’s not strictly speaking an internal device/IP. Regards, Loic >
Hi Loic, On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote: > > Hi Simon, > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : >> >> Hi Loic, >> >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: >> > >> > Mark sha256_process as weak to allow hardware specific implementation. >> > Add parameter for supporting multiple blocks processing. >> > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >> > --- >> > lib/sha256.c | 26 +++++++++++++++++++------- >> > 1 file changed, 19 insertions(+), 7 deletions(-) >> > >> > diff --git a/lib/sha256.c b/lib/sha256.c >> > index c1fe93d..50b0b51 100644 >> > --- a/lib/sha256.c >> > +++ b/lib/sha256.c >> > @@ -14,6 +14,8 @@ >> > #include <watchdog.h> >> > #include <u-boot/sha256.h> >> > >> > +#include <linux/compiler_attributes.h> >> > + >> > const uint8_t sha256_der_prefix[SHA256_DER_LEN] = { >> > 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, >> > 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, >> > @@ -55,7 +57,7 @@ void sha256_starts(sha256_context * ctx) >> > ctx->state[7] = 0x5BE0CD19; >> > } >> > >> > -static void sha256_process(sha256_context *ctx, const uint8_t data[64]) >> > +static void sha256_process_one(sha256_context *ctx, const uint8_t data[64]) >> > { >> > uint32_t temp1, temp2; >> > uint32_t W[64]; >> > @@ -186,6 +188,18 @@ static void sha256_process(sha256_context *ctx, const uint8_t data[64]) >> > ctx->state[7] += H; >> > } >> > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, >> > + unsigned int blocks) >> > +{ >> > + if (!blocks) >> > + return; >> > + >> > + while (blocks--) { >> > + sha256_process_one(ctx, data); >> > + data += 64; >> > + } >> > +} >> > + >> > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) >> > { >> > uint32_t left, fill; >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) >> > >> > if (left && length >= fill) { >> > memcpy((void *) (ctx->buffer + left), (void *) input, fill); >> > - sha256_process(ctx, ctx->buffer); >> > + sha256_process(ctx, ctx->buffer, 1); >> > length -= fill; >> > input += fill; >> > left = 0; >> > } >> > >> > - while (length >= 64) { >> > - sha256_process(ctx, input); >> > - length -= 64; >> > - input += 64; >> > - } >> > + sha256_process(ctx, input, length / 64); >> > + input += length / 64 * 64; >> > + length = length % 64; >> > >> > if (length) >> > memcpy((void *) (ctx->buffer + left), (void *) input, length); >> > -- >> > 2.7.4 >> > >> >> I just came across this patch as it broke minnowmax. > > > Ok, is it a build time or runtime break? Build, but you need the binary blobs to see it :-( > >> >> >> This should be using driver model, not weak functions. Please can you >> take a look? > > > Yes I can look at it in the next few days. I have used weak function because it’s an architecture feature offered by armv8 instructions, It’s not strictly speaking an internal device/IP. Thanks. Right, same as hardware-accelerated hashing hardware in my book. See hash.c which has become a mess. We have been trying to make do with a list of algos, but given all the updates I think needs a new UCLASS_HASH with the same operations as in hash.h Regards, Simon
Hi Simon, On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg@chromium.org> wrote: > > Hi Loic, > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > Hi Simon, > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : > >> > >> Hi Loic, > >> > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > >> > > >> > Mark sha256_process as weak to allow hardware specific implementation. > >> > Add parameter for supporting multiple blocks processing. > >> > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > >> > --- > >> > lib/sha256.c | 26 +++++++++++++++++++------- > >> > 1 file changed, 19 insertions(+), 7 deletions(-) > >> > [...] > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, > >> > + unsigned int blocks) > >> > +{ > >> > + if (!blocks) > >> > + return; > >> > + > >> > + while (blocks--) { > >> > + sha256_process_one(ctx, data); > >> > + data += 64; > >> > + } > >> > +} > >> > + > >> > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > >> > { > >> > uint32_t left, fill; > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > >> > > >> > if (left && length >= fill) { > >> > memcpy((void *) (ctx->buffer + left), (void *) input, fill); > >> > - sha256_process(ctx, ctx->buffer); > >> > + sha256_process(ctx, ctx->buffer, 1); > >> > length -= fill; > >> > input += fill; > >> > left = 0; > >> > } > >> > > >> > - while (length >= 64) { > >> > - sha256_process(ctx, input); > >> > - length -= 64; > >> > - input += 64; > >> > - } > >> > + sha256_process(ctx, input, length / 64); > >> > + input += length / 64 * 64; > >> > + length = length % 64; > >> > > >> > if (length) > >> > memcpy((void *) (ctx->buffer + left), (void *) input, length); > >> > -- > >> > 2.7.4 > >> > > >> > >> I just came across this patch as it broke minnowmax. > > > > > > Ok, is it a build time or runtime break? > > Build, but you need the binary blobs to see it :-( > >> > >> This should be using driver model, not weak functions. Please can you > >> take a look? Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig; gcc-11.3.0), and I've not observed any issue (but I had to fake some of the binary blobs...). Could you share the build problem/error you encountered? As you mentioned it, Is the error specifically related to _weak function linking? Would like to have a simple and quick fix before trying to move on to a more proper DM based solution. Thanks, Loic > > > > > > Yes I can look at it in the next few days. I have used weak function because it’s an architecture feature offered by armv8 instructions, It’s not strictly speaking an internal device/IP. > > Thanks. > > Right, same as hardware-accelerated hashing hardware in my book. > > See hash.c which has become a mess. We have been trying to make do > with a list of algos, but given all the updates I think needs a new > UCLASS_HASH with the same operations as in hash.h > > Regards, > Simon
Hi Loic, On Tue, 7 Feb 2023 at 14:47, Loic Poulain <loic.poulain@linaro.org> wrote: > > Hi Simon, > > On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Loic, > > > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : > > >> > > >> Hi Loic, > > >> > > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > > >> > > > >> > Mark sha256_process as weak to allow hardware specific implementation. > > >> > Add parameter for supporting multiple blocks processing. > > >> > > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > >> > --- > > >> > lib/sha256.c | 26 +++++++++++++++++++------- > > >> > 1 file changed, 19 insertions(+), 7 deletions(-) > > >> > > [...] > > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, > > >> > + unsigned int blocks) > > >> > +{ > > >> > + if (!blocks) > > >> > + return; > > >> > + > > >> > + while (blocks--) { > > >> > + sha256_process_one(ctx, data); > > >> > + data += 64; > > >> > + } > > >> > +} > > >> > + > > >> > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > >> > { > > >> > uint32_t left, fill; > > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > >> > > > >> > if (left && length >= fill) { > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, fill); > > >> > - sha256_process(ctx, ctx->buffer); > > >> > + sha256_process(ctx, ctx->buffer, 1); > > >> > length -= fill; > > >> > input += fill; > > >> > left = 0; > > >> > } > > >> > > > >> > - while (length >= 64) { > > >> > - sha256_process(ctx, input); > > >> > - length -= 64; > > >> > - input += 64; > > >> > - } > > >> > + sha256_process(ctx, input, length / 64); > > >> > + input += length / 64 * 64; > > >> > + length = length % 64; > > >> > > > >> > if (length) > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, length); > > >> > -- > > >> > 2.7.4 > > >> > > > >> > > >> I just came across this patch as it broke minnowmax. > > > > > > > > > Ok, is it a build time or runtime break? > > > > Build, but you need the binary blobs to see it :-( > > >> > > >> This should be using driver model, not weak functions. Please can you > > >> take a look? > > Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig; > gcc-11.3.0), and I've not observed any issue (but I had to fake some > of the binary blobs...). Could you share the build problem/error you Unfortunately you need the blobs! > encountered? As you mentioned it, Is the error specifically related to > _weak function linking? Would like to have a simple and quick fix > before trying to move on to a more proper DM based solution. It is just because of the code size increase, I believe. I am planning to dig into it a bit as Bin Meng asked for more info as to why I sent a revert for his patch moving U-Boot. Regards, Simon > > Thanks, > Loic > > > > > > > > > > > Yes I can look at it in the next few days. I have used weak function because it’s an architecture feature offered by armv8 instructions, It’s not strictly speaking an internal device/IP. > > > > Thanks. > > > > Right, same as hardware-accelerated hashing hardware in my book. > > > > See hash.c which has become a mess. We have been trying to make do > > with a list of algos, but given all the updates I think needs a new > > UCLASS_HASH with the same operations as in hash.h > > > > Regards, > > Simon
On Tue, Feb 07, 2023 at 03:25:16PM -0700, Simon Glass wrote: > Hi Loic, > > On Tue, 7 Feb 2023 at 14:47, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > Hi Simon, > > > > On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Loic, > > > > > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : > > > >> > > > >> Hi Loic, > > > >> > > > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > > > >> > > > > >> > Mark sha256_process as weak to allow hardware specific implementation. > > > >> > Add parameter for supporting multiple blocks processing. > > > >> > > > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > > >> > --- > > > >> > lib/sha256.c | 26 +++++++++++++++++++------- > > > >> > 1 file changed, 19 insertions(+), 7 deletions(-) > > > >> > > > [...] > > > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, > > > >> > + unsigned int blocks) > > > >> > +{ > > > >> > + if (!blocks) > > > >> > + return; > > > >> > + > > > >> > + while (blocks--) { > > > >> > + sha256_process_one(ctx, data); > > > >> > + data += 64; > > > >> > + } > > > >> > +} > > > >> > + > > > >> > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > > >> > { > > > >> > uint32_t left, fill; > > > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > > >> > > > > >> > if (left && length >= fill) { > > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, fill); > > > >> > - sha256_process(ctx, ctx->buffer); > > > >> > + sha256_process(ctx, ctx->buffer, 1); > > > >> > length -= fill; > > > >> > input += fill; > > > >> > left = 0; > > > >> > } > > > >> > > > > >> > - while (length >= 64) { > > > >> > - sha256_process(ctx, input); > > > >> > - length -= 64; > > > >> > - input += 64; > > > >> > - } > > > >> > + sha256_process(ctx, input, length / 64); > > > >> > + input += length / 64 * 64; > > > >> > + length = length % 64; > > > >> > > > > >> > if (length) > > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, length); > > > >> > -- > > > >> > 2.7.4 > > > >> > > > > >> > > > >> I just came across this patch as it broke minnowmax. > > > > > > > > > > > > Ok, is it a build time or runtime break? > > > > > > Build, but you need the binary blobs to see it :-( > > > >> > > > >> This should be using driver model, not weak functions. Please can you > > > >> take a look? > > > > Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig; > > gcc-11.3.0), and I've not observed any issue (but I had to fake some > > of the binary blobs...). Could you share the build problem/error you > > Unfortunately you need the blobs! > > > encountered? As you mentioned it, Is the error specifically related to > > _weak function linking? Would like to have a simple and quick fix > > before trying to move on to a more proper DM based solution. > > It is just because of the code size increase, I believe. I am planning > to dig into it a bit as Bin Meng asked for more info as to why I sent > a revert for his patch moving U-Boot. That honestly makes more sense, having stared at the commit in question. Perhaps Minnow needs LTO enabled.
Hi Tom, On Tue, 7 Feb 2023 at 17:10, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Feb 07, 2023 at 03:25:16PM -0700, Simon Glass wrote: > > Hi Loic, > > > > On Tue, 7 Feb 2023 at 14:47, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Loic, > > > > > > > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : > > > > >> > > > > >> Hi Loic, > > > > >> > > > > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > >> > > > > > >> > Mark sha256_process as weak to allow hardware specific implementation. > > > > >> > Add parameter for supporting multiple blocks processing. > > > > >> > > > > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > > > >> > --- > > > > >> > lib/sha256.c | 26 +++++++++++++++++++------- > > > > >> > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > >> > > > > [...] > > > > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, > > > > >> > + unsigned int blocks) > > > > >> > +{ > > > > >> > + if (!blocks) > > > > >> > + return; > > > > >> > + > > > > >> > + while (blocks--) { > > > > >> > + sha256_process_one(ctx, data); > > > > >> > + data += 64; > > > > >> > + } > > > > >> > +} > > > > >> > + > > > > >> > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > > > >> > { > > > > >> > uint32_t left, fill; > > > > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > > > >> > > > > > >> > if (left && length >= fill) { > > > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, fill); > > > > >> > - sha256_process(ctx, ctx->buffer); > > > > >> > + sha256_process(ctx, ctx->buffer, 1); > > > > >> > length -= fill; > > > > >> > input += fill; > > > > >> > left = 0; > > > > >> > } > > > > >> > > > > > >> > - while (length >= 64) { > > > > >> > - sha256_process(ctx, input); > > > > >> > - length -= 64; > > > > >> > - input += 64; > > > > >> > - } > > > > >> > + sha256_process(ctx, input, length / 64); > > > > >> > + input += length / 64 * 64; > > > > >> > + length = length % 64; > > > > >> > > > > > >> > if (length) > > > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, length); > > > > >> > -- > > > > >> > 2.7.4 > > > > >> > > > > > >> > > > > >> I just came across this patch as it broke minnowmax. > > > > > > > > > > > > > > > Ok, is it a build time or runtime break? > > > > > > > > Build, but you need the binary blobs to see it :-( > > > > >> > > > > >> This should be using driver model, not weak functions. Please can you > > > > >> take a look? > > > > > > Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig; > > > gcc-11.3.0), and I've not observed any issue (but I had to fake some > > > of the binary blobs...). Could you share the build problem/error you > > > > Unfortunately you need the blobs! > > > > > encountered? As you mentioned it, Is the error specifically related to > > > _weak function linking? Would like to have a simple and quick fix > > > before trying to move on to a more proper DM based solution. > > > > It is just because of the code size increase, I believe. I am planning > > to dig into it a bit as Bin Meng asked for more info as to why I sent > > a revert for his patch moving U-Boot. > > That honestly makes more sense, having stared at the commit in > question. Perhaps Minnow needs LTO enabled. Yes, that would likely help. But Bin's point is that it should be possible to move the text base. Anyway, the point of this thread is that this needs to be done as drivers rather than weak functions. Unfortunately hash.c has grown a few appendages...this is yet another. Regards, Simon
On Wed, Feb 08, 2023 at 11:28:21AM -0700, Simon Glass wrote: > Hi Tom, > > On Tue, 7 Feb 2023 at 17:10, Tom Rini <trini@konsulko.com> wrote: > > > > On Tue, Feb 07, 2023 at 03:25:16PM -0700, Simon Glass wrote: > > > Hi Loic, > > > > > > On Tue, 7 Feb 2023 at 14:47, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Loic, > > > > > > > > > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit : > > > > > >> > > > > > >> Hi Loic, > > > > > >> > > > > > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > >> > > > > > > >> > Mark sha256_process as weak to allow hardware specific implementation. > > > > > >> > Add parameter for supporting multiple blocks processing. > > > > > >> > > > > > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > > > > >> > --- > > > > > >> > lib/sha256.c | 26 +++++++++++++++++++------- > > > > > >> > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > > >> > > > > > [...] > > > > > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, > > > > > >> > + unsigned int blocks) > > > > > >> > +{ > > > > > >> > + if (!blocks) > > > > > >> > + return; > > > > > >> > + > > > > > >> > + while (blocks--) { > > > > > >> > + sha256_process_one(ctx, data); > > > > > >> > + data += 64; > > > > > >> > + } > > > > > >> > +} > > > > > >> > + > > > > > >> > void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > > > > >> > { > > > > > >> > uint32_t left, fill; > > > > > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) > > > > > >> > > > > > > >> > if (left && length >= fill) { > > > > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, fill); > > > > > >> > - sha256_process(ctx, ctx->buffer); > > > > > >> > + sha256_process(ctx, ctx->buffer, 1); > > > > > >> > length -= fill; > > > > > >> > input += fill; > > > > > >> > left = 0; > > > > > >> > } > > > > > >> > > > > > > >> > - while (length >= 64) { > > > > > >> > - sha256_process(ctx, input); > > > > > >> > - length -= 64; > > > > > >> > - input += 64; > > > > > >> > - } > > > > > >> > + sha256_process(ctx, input, length / 64); > > > > > >> > + input += length / 64 * 64; > > > > > >> > + length = length % 64; > > > > > >> > > > > > > >> > if (length) > > > > > >> > memcpy((void *) (ctx->buffer + left), (void *) input, length); > > > > > >> > -- > > > > > >> > 2.7.4 > > > > > >> > > > > > > >> > > > > > >> I just came across this patch as it broke minnowmax. > > > > > > > > > > > > > > > > > > Ok, is it a build time or runtime break? > > > > > > > > > > Build, but you need the binary blobs to see it :-( > > > > > >> > > > > > >> This should be using driver model, not weak functions. Please can you > > > > > >> take a look? > > > > > > > > Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig; > > > > gcc-11.3.0), and I've not observed any issue (but I had to fake some > > > > of the binary blobs...). Could you share the build problem/error you > > > > > > Unfortunately you need the blobs! > > > > > > > encountered? As you mentioned it, Is the error specifically related to > > > > _weak function linking? Would like to have a simple and quick fix > > > > before trying to move on to a more proper DM based solution. > > > > > > It is just because of the code size increase, I believe. I am planning > > > to dig into it a bit as Bin Meng asked for more info as to why I sent > > > a revert for his patch moving U-Boot. > > > > That honestly makes more sense, having stared at the commit in > > question. Perhaps Minnow needs LTO enabled. > > Yes, that would likely help. But Bin's point is that it should be > possible to move the text base. I suspect that might just be more fall-out of the size problem and possibly some hard coded assumptions in the blobs? > Anyway, the point of this thread is that this needs to be done as > drivers rather than weak functions. Unfortunately hash.c has grown a > few appendages...this is yet another. I don't know, it seems fine, especially since we aren't really talking about a "hash driver" but rather introducing some performance sensitive code.
diff --git a/lib/sha256.c b/lib/sha256.c index c1fe93d..50b0b51 100644 --- a/lib/sha256.c +++ b/lib/sha256.c @@ -14,6 +14,8 @@ #include <watchdog.h> #include <u-boot/sha256.h> +#include <linux/compiler_attributes.h> + const uint8_t sha256_der_prefix[SHA256_DER_LEN] = { 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, @@ -55,7 +57,7 @@ void sha256_starts(sha256_context * ctx) ctx->state[7] = 0x5BE0CD19; } -static void sha256_process(sha256_context *ctx, const uint8_t data[64]) +static void sha256_process_one(sha256_context *ctx, const uint8_t data[64]) { uint32_t temp1, temp2; uint32_t W[64]; @@ -186,6 +188,18 @@ static void sha256_process(sha256_context *ctx, const uint8_t data[64]) ctx->state[7] += H; } +__weak void sha256_process(sha256_context *ctx, const unsigned char *data, + unsigned int blocks) +{ + if (!blocks) + return; + + while (blocks--) { + sha256_process_one(ctx, data); + data += 64; + } +} + void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) { uint32_t left, fill; @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) if (left && length >= fill) { memcpy((void *) (ctx->buffer + left), (void *) input, fill); - sha256_process(ctx, ctx->buffer); + sha256_process(ctx, ctx->buffer, 1); length -= fill; input += fill; left = 0; } - while (length >= 64) { - sha256_process(ctx, input); - length -= 64; - input += 64; - } + sha256_process(ctx, input, length / 64); + input += length / 64 * 64; + length = length % 64; if (length) memcpy((void *) (ctx->buffer + left), (void *) input, length);
Mark sha256_process as weak to allow hardware specific implementation. Add parameter for supporting multiple blocks processing. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- lib/sha256.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)