* [PATCH] crypto: crc32: make crc32 available in PBL @ 2023-08-29 9:28 Johannes Zink 2023-08-29 10:29 ` Ahmad Fatoum 0 siblings, 1 reply; 7+ messages in thread From: Johannes Zink @ 2023-08-29 9:28 UTC (permalink / raw) To: patchwork-jzi; +Cc: Johannes Zink, Barebox Mailing List crc32 may be required in PBL for checking data integrity. Add CONFIG_CRC32_EARLY to kconfig which allows adding crc32 also in the PBL. As the PBL has no dynamic allocation, do not use dynamic CRC table generation when CRC32 is added in PBL. Signed-off-by: Johannes Zink <j.zink@pengutronix.de> --- To: Barebox Mailing List <barebox@lists.infradead.org> Cc: Johannes Zink <j.zink@pengutronix.de> Cc: patchwork-jzi@pengutronix.de --- crypto/Kconfig | 3 +++ crypto/Makefile | 1 + crypto/crc32.c | 6 +++--- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/crypto/Kconfig b/crypto/Kconfig index 04e5ef43705b..6e18b5868061 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -5,6 +5,9 @@ menu "Crypto support" config CRC32 bool +config CRC32_EARLY + bool + config CRC_ITU_T bool diff --git a/crypto/Makefile b/crypto/Makefile index 22035d4f69ee..24e1f3d91797 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_DIGEST_SHA1_GENERIC) += sha1.o obj-$(CONFIG_DIGEST_SHA224_GENERIC) += sha2.o obj-$(CONFIG_DIGEST_SHA256_GENERIC) += sha2.o pbl-y += sha2.o digest.o +lwl-$(CONFIG_CRC32_EARLY) += crc32.o obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o obj-y += memneq.o diff --git a/crypto/crc32.c b/crypto/crc32.c index 95cb2212db2b..284d39351682 100644 --- a/crypto/crc32.c +++ b/crypto/crc32.c @@ -22,7 +22,7 @@ #define STATIC static inline #endif -#ifdef CONFIG_DYNAMIC_CRC_TABLE +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) static uint32_t *crc_table; @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) { const unsigned char *buf = _buf; -#ifdef CONFIG_DYNAMIC_CRC_TABLE +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) if (!crc_table) make_crc_table(); #endif @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) { const unsigned char *buf = _buf; -#ifdef CONFIG_DYNAMIC_CRC_TABLE +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) if (!crc_table) make_crc_table(); #endif --- base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c change-id: 20230829-crc32_in_pbl-4d824629d4e2 Best regards, -- Johannes Zink <j.zink@pengutronix.de> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: crc32: make crc32 available in PBL 2023-08-29 9:28 [PATCH] crypto: crc32: make crc32 available in PBL Johannes Zink @ 2023-08-29 10:29 ` Ahmad Fatoum 2023-08-29 10:45 ` Ahmad Fatoum 0 siblings, 1 reply; 7+ messages in thread From: Ahmad Fatoum @ 2023-08-29 10:29 UTC (permalink / raw) To: Johannes Zink, patchwork-jzi; +Cc: Barebox Mailing List On 29.08.23 11:28, Johannes Zink wrote: > crc32 may be required in PBL for checking data integrity. Add > CONFIG_CRC32_EARLY to kconfig which allows adding crc32 also in the PBL. > > As the PBL has no dynamic allocation, do not use dynamic CRC table > generation when CRC32 is added in PBL. > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de> > --- > To: Barebox Mailing List <barebox@lists.infradead.org> > Cc: Johannes Zink <j.zink@pengutronix.de> > Cc: patchwork-jzi@pengutronix.de > --- > crypto/Kconfig | 3 +++ > crypto/Makefile | 1 + > crypto/crc32.c | 6 +++--- > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/crypto/Kconfig b/crypto/Kconfig > index 04e5ef43705b..6e18b5868061 100644 > --- a/crypto/Kconfig > +++ b/crypto/Kconfig > @@ -5,6 +5,9 @@ menu "Crypto support" > config CRC32 > bool > > +config CRC32_EARLY > + bool > + > config CRC_ITU_T > bool > > diff --git a/crypto/Makefile b/crypto/Makefile > index 22035d4f69ee..24e1f3d91797 100644 > --- a/crypto/Makefile > +++ b/crypto/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_DIGEST_SHA1_GENERIC) += sha1.o > obj-$(CONFIG_DIGEST_SHA224_GENERIC) += sha2.o > obj-$(CONFIG_DIGEST_SHA256_GENERIC) += sha2.o > pbl-y += sha2.o digest.o > +lwl-$(CONFIG_CRC32_EARLY) += crc32.o pbl-obj- is the correct prefix. lwl- means pbl- if we have PBL support at all and obj- otherwise (for legacy systems without PBL), while pbl-obj- is equivalent to duplicating the line once with pbl- and once with obj- > obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o > obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o > obj-y += memneq.o > diff --git a/crypto/crc32.c b/crypto/crc32.c > index 95cb2212db2b..284d39351682 100644 > --- a/crypto/crc32.c > +++ b/crypto/crc32.c > @@ -22,7 +22,7 @@ > #define STATIC static inline > #endif > > -#ifdef CONFIG_DYNAMIC_CRC_TABLE > +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) You could also replace the dynamic allocation with a static array initialized to zero. That way you can have a dynamic crc table even in PBL without affecting image size as the BSS is not part of the image. > > static uint32_t *crc_table; > > @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) > { > const unsigned char *buf = _buf; > > -#ifdef CONFIG_DYNAMIC_CRC_TABLE > +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) > if (!crc_table) > make_crc_table(); > #endif > @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) > { > const unsigned char *buf = _buf; > > -#ifdef CONFIG_DYNAMIC_CRC_TABLE > +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) > if (!crc_table) > make_crc_table(); > #endif > > --- > base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c > change-id: 20230829-crc32_in_pbl-4d824629d4e2 > > Best regards, -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: crc32: make crc32 available in PBL 2023-08-29 10:29 ` Ahmad Fatoum @ 2023-08-29 10:45 ` Ahmad Fatoum 2023-08-29 10:55 ` Johannes Zink 0 siblings, 1 reply; 7+ messages in thread From: Ahmad Fatoum @ 2023-08-29 10:45 UTC (permalink / raw) To: Johannes Zink, patchwork-jzi; +Cc: Barebox Mailing List On 29.08.23 12:29, Ahmad Fatoum wrote: > On 29.08.23 11:28, Johannes Zink wrote: >> crc32 may be required in PBL for checking data integrity. Add >> CONFIG_CRC32_EARLY to kconfig which allows adding crc32 also in the PBL. >> >> As the PBL has no dynamic allocation, do not use dynamic CRC table >> generation when CRC32 is added in PBL. >> >> Signed-off-by: Johannes Zink <j.zink@pengutronix.de> >> --- >> To: Barebox Mailing List <barebox@lists.infradead.org> >> Cc: Johannes Zink <j.zink@pengutronix.de> >> Cc: patchwork-jzi@pengutronix.de >> --- >> crypto/Kconfig | 3 +++ >> crypto/Makefile | 1 + >> crypto/crc32.c | 6 +++--- >> 3 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/crypto/Kconfig b/crypto/Kconfig >> index 04e5ef43705b..6e18b5868061 100644 >> --- a/crypto/Kconfig >> +++ b/crypto/Kconfig >> @@ -5,6 +5,9 @@ menu "Crypto support" >> config CRC32 >> bool >> >> +config CRC32_EARLY >> + bool >> + >> config CRC_ITU_T >> bool >> >> diff --git a/crypto/Makefile b/crypto/Makefile >> index 22035d4f69ee..24e1f3d91797 100644 >> --- a/crypto/Makefile >> +++ b/crypto/Makefile >> @@ -11,6 +11,7 @@ obj-$(CONFIG_DIGEST_SHA1_GENERIC) += sha1.o >> obj-$(CONFIG_DIGEST_SHA224_GENERIC) += sha2.o >> obj-$(CONFIG_DIGEST_SHA256_GENERIC) += sha2.o >> pbl-y += sha2.o digest.o >> +lwl-$(CONFIG_CRC32_EARLY) += crc32.o > > pbl-obj- is the correct prefix. lwl- means pbl- if we have PBL > support at all and obj- otherwise (for legacy systems without PBL), > while pbl-obj- is equivalent to duplicating the line once with pbl- > and once with obj- s/pbl-obj-/obj-pbl-/ :) > >> obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o >> obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o >> obj-y += memneq.o >> diff --git a/crypto/crc32.c b/crypto/crc32.c >> index 95cb2212db2b..284d39351682 100644 >> --- a/crypto/crc32.c >> +++ b/crypto/crc32.c >> @@ -22,7 +22,7 @@ >> #define STATIC static inline >> #endif >> >> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) > > You could also replace the dynamic allocation with a static array initialized > to zero. That way you can have a dynamic crc table even in PBL without affecting > image size as the BSS is not part of the image. > >> >> static uint32_t *crc_table; >> >> @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) >> { >> const unsigned char *buf = _buf; >> >> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >> if (!crc_table) >> make_crc_table(); >> #endif >> @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) >> { >> const unsigned char *buf = _buf; >> >> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >> if (!crc_table) >> make_crc_table(); >> #endif >> >> --- >> base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c >> change-id: 20230829-crc32_in_pbl-4d824629d4e2 >> >> Best regards, > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: crc32: make crc32 available in PBL 2023-08-29 10:45 ` Ahmad Fatoum @ 2023-08-29 10:55 ` Johannes Zink 2023-08-29 11:08 ` Ahmad Fatoum 0 siblings, 1 reply; 7+ messages in thread From: Johannes Zink @ 2023-08-29 10:55 UTC (permalink / raw) To: Ahmad Fatoum, patchwork-jzi; +Cc: Barebox Mailing List Hi Ahmad, thank you for your review. On 8/29/23 12:45, Ahmad Fatoum wrote: > On 29.08.23 12:29, Ahmad Fatoum wrote: >> On 29.08.23 11:28, Johannes Zink wrote: >>> crc32 may be required in PBL for checking data integrity. Add >>> CONFIG_CRC32_EARLY to kconfig which allows adding crc32 also in the PBL. >>> >>> As the PBL has no dynamic allocation, do not use dynamic CRC table >>> generation when CRC32 is added in PBL. >>> >>> Signed-off-by: Johannes Zink <j.zink@pengutronix.de> >>> --- >>> To: Barebox Mailing List <barebox@lists.infradead.org> >>> Cc: Johannes Zink <j.zink@pengutronix.de> >>> Cc: patchwork-jzi@pengutronix.de >>> --- >>> crypto/Kconfig | 3 +++ >>> crypto/Makefile | 1 + >>> crypto/crc32.c | 6 +++--- >>> 3 files changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/crypto/Kconfig b/crypto/Kconfig >>> index 04e5ef43705b..6e18b5868061 100644 >>> --- a/crypto/Kconfig >>> +++ b/crypto/Kconfig >>> @@ -5,6 +5,9 @@ menu "Crypto support" >>> config CRC32 >>> bool >>> >>> +config CRC32_EARLY >>> + bool >>> + >>> config CRC_ITU_T >>> bool >>> >>> diff --git a/crypto/Makefile b/crypto/Makefile >>> index 22035d4f69ee..24e1f3d91797 100644 >>> --- a/crypto/Makefile >>> +++ b/crypto/Makefile >>> @@ -11,6 +11,7 @@ obj-$(CONFIG_DIGEST_SHA1_GENERIC) += sha1.o >>> obj-$(CONFIG_DIGEST_SHA224_GENERIC) += sha2.o >>> obj-$(CONFIG_DIGEST_SHA256_GENERIC) += sha2.o >>> pbl-y += sha2.o digest.o >>> +lwl-$(CONFIG_CRC32_EARLY) += crc32.o >> >> pbl-obj- is the correct prefix. lwl- means pbl- if we have PBL >> support at all and obj- otherwise (for legacy systems without PBL), >> while pbl-obj- is equivalent to duplicating the line once with pbl- >> and once with obj- > > s/pbl-obj-/obj-pbl-/ :) > ack, gonna fix this for v2. >> >>> obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o >>> obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o >>> obj-y += memneq.o >>> diff --git a/crypto/crc32.c b/crypto/crc32.c >>> index 95cb2212db2b..284d39351682 100644 >>> --- a/crypto/crc32.c >>> +++ b/crypto/crc32.c >>> @@ -22,7 +22,7 @@ >>> #define STATIC static inline >>> #endif >>> >>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >> >> You could also replace the dynamic allocation with a static array initialized >> to zero. That way you can have a dynamic crc table even in PBL without affecting >> image size as the BSS is not part of the image. ack. Is this ok? #ifdef __PBL__ static uint32_t _crc_table_memory[sizeof(uint32_t) * 256] = { 0 }; #endif static void *alloc_crc_table() { #ifdef __PBL__ return _crc_table_memory; #else return xmalloc(sizeof(uint32_t) * 256); #endif } If so, I can change it for v2. Best regards Johannes >> >>> >>> static uint32_t *crc_table; >>> >>> @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) >>> { >>> const unsigned char *buf = _buf; >>> >>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>> if (!crc_table) >>> make_crc_table(); >>> #endif >>> @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) >>> { >>> const unsigned char *buf = _buf; >>> >>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>> if (!crc_table) >>> make_crc_table(); >>> #endif >>> >>> --- >>> base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c >>> change-id: 20230829-crc32_in_pbl-4d824629d4e2 >>> >>> Best regards, >> > -- Pengutronix e.K. | Johannes Zink | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: crc32: make crc32 available in PBL 2023-08-29 10:55 ` Johannes Zink @ 2023-08-29 11:08 ` Ahmad Fatoum 2023-08-29 12:05 ` Johannes Zink 0 siblings, 1 reply; 7+ messages in thread From: Ahmad Fatoum @ 2023-08-29 11:08 UTC (permalink / raw) To: Johannes Zink, patchwork-jzi; +Cc: Barebox Mailing List On 29.08.23 12:55, Johannes Zink wrote: > On 8/29/23 12:45, Ahmad Fatoum wrote: >>>> +lwl-$(CONFIG_CRC32_EARLY) += crc32.o >>> >>> pbl-obj- is the correct prefix. lwl- means pbl- if we have PBL >>> support at all and obj- otherwise (for legacy systems without PBL), >>> while pbl-obj- is equivalent to duplicating the line once with pbl- >>> and once with obj- >> >> s/pbl-obj-/obj-pbl-/ :) Sorry, had a small brain fart here. You didn't remove the original obj-, so now lwl- either expands to and extra obj- or to pbl-. obj-pbl- makes sense when you use the same symbol for both PBL and barebox proper, but as you're introducing a new symbol, you can leave it as lwl- or make it pbl- for explicitness. > ack, gonna fix this for v2. > >>> >>>> obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o >>>> obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o >>>> obj-y += memneq.o >>>> diff --git a/crypto/crc32.c b/crypto/crc32.c >>>> index 95cb2212db2b..284d39351682 100644 >>>> --- a/crypto/crc32.c >>>> +++ b/crypto/crc32.c >>>> @@ -22,7 +22,7 @@ >>>> #define STATIC static inline >>>> #endif >>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>> >>> You could also replace the dynamic allocation with a static array initialized >>> to zero. That way you can have a dynamic crc table even in PBL without affecting >>> image size as the BSS is not part of the image. > > ack. Is this ok? > > #ifdef __PBL__ > static uint32_t _crc_table_memory[sizeof(uint32_t) * 256] = { 0 }; The array is 256 elements, not 1024 elements. Explicit intialization is unnecessary. > #endif > > static void *alloc_crc_table() { > #ifdef __PBL__ > return _crc_table_memory; > #else > return xmalloc(sizeof(uint32_t) * 256); > #endif > } > > If so, I can change it for v2. My idea was to drop the allocation altogether by using BSS. If you do this, you should not need any __PBL__ checking at all. Either you have CONFIG_DYNAMIC_CRC_TABLE and the table is dynamically filled in bss on first access or you have CONFIG_DYNAMIC_CRC_TABLE=n and the table is already there and need not be allocated. On a second thought, I am not sure if we want a table at all in PBL. Do you do a lot of CRC32 computation? Maybe we should just not use a table at all in PBL and just calculate a single crc32? That's what Sascha did here: 2d13b856604b ("crc: Add PBL variant for crc_itu_t()") Let me know what you think. Cheers, Ahmad > > Best regards > Johannes > > >>> >>>> static uint32_t *crc_table; >>>> @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) >>>> { >>>> const unsigned char *buf = _buf; >>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>> if (!crc_table) >>>> make_crc_table(); >>>> #endif >>>> @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) >>>> { >>>> const unsigned char *buf = _buf; >>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>> if (!crc_table) >>>> make_crc_table(); >>>> #endif >>>> >>>> --- >>>> base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c >>>> change-id: 20230829-crc32_in_pbl-4d824629d4e2 >>>> >>>> Best regards, >>> >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: crc32: make crc32 available in PBL 2023-08-29 11:08 ` Ahmad Fatoum @ 2023-08-29 12:05 ` Johannes Zink 2023-08-29 12:17 ` Ahmad Fatoum 0 siblings, 1 reply; 7+ messages in thread From: Johannes Zink @ 2023-08-29 12:05 UTC (permalink / raw) To: Ahmad Fatoum, patchwork-jzi; +Cc: Barebox Mailing List Hi Ahmad, On 8/29/23 13:08, Ahmad Fatoum wrote: > On 29.08.23 12:55, Johannes Zink wrote: >> On 8/29/23 12:45, Ahmad Fatoum wrote: >>>>> +lwl-$(CONFIG_CRC32_EARLY) += crc32.o >>>> >>>> pbl-obj- is the correct prefix. lwl- means pbl- if we have PBL >>>> support at all and obj- otherwise (for legacy systems without PBL), >>>> while pbl-obj- is equivalent to duplicating the line once with pbl- >>>> and once with obj- >>> >>> s/pbl-obj-/obj-pbl-/ :) > > Sorry, had a small brain fart here. > > You didn't remove the original obj-, > so now lwl- either expands to and extra obj- or to pbl-. > > obj-pbl- makes sense when you use the same symbol for both PBL and > barebox proper, but as you're introducing a new symbol, you can > leave it as lwl- or make it pbl- for explicitness. > ok. I think I won't glob it in obj-pbl, as one might want to explicitly add crc32.o only for PBL or only for barebox-proper. >> ack, gonna fix this for v2. >> >>>> >>>>> obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o >>>>> obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o >>>>> obj-y += memneq.o >>>>> diff --git a/crypto/crc32.c b/crypto/crc32.c >>>>> index 95cb2212db2b..284d39351682 100644 >>>>> --- a/crypto/crc32.c >>>>> +++ b/crypto/crc32.c >>>>> @@ -22,7 +22,7 @@ >>>>> #define STATIC static inline >>>>> #endif >>>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>> >>>> You could also replace the dynamic allocation with a static array initialized >>>> to zero. That way you can have a dynamic crc table even in PBL without affecting >>>> image size as the BSS is not part of the image. >> >> ack. Is this ok? >> >> #ifdef __PBL__ >> static uint32_t _crc_table_memory[sizeof(uint32_t) * 256] = { 0 }; > > The array is 256 elements, not 1024 elements. Explicit intialization > is unnecessary. good catch. thanks. > >> #endif >> >> static void *alloc_crc_table() { >> #ifdef __PBL__ >> return _crc_table_memory; >> #else >> return xmalloc(sizeof(uint32_t) * 256); >> #endif >> } >> >> If so, I can change it for v2. > > My idea was to drop the allocation altogether by using BSS. > If you do this, you should not need any __PBL__ checking at all. I guess that should work indeed. Do I need to explicitly mark the array with a prefix to force it into BSS (sorry, I am not too familiar with the barebox linker scripts...)? > > Either you have CONFIG_DYNAMIC_CRC_TABLE and the table is dynamically > filled in bss on first access or you have CONFIG_DYNAMIC_CRC_TABLE=n > and the table is already there and need not be allocated. > > On a second thought, I am not sure if we want a table at all in PBL. > Do you do a lot of CRC32 computation? Maybe we should just not use > a table at all in PBL and just calculate a single crc32? > That's what Sascha did here: > > 2d13b856604b ("crc: Add PBL variant for crc_itu_t()") > For my specific usecase I need to check a single CRC32 once, so probably there is the same or even more overhead to dynamically filling the Table than just doing a single CRC32 computation. Maybe more overhead than using the precompiled, pre-filled table, but I fully acknowledge that size matters in PBL. I like Sascha's approach, so let me try to do a dynamic computation for the PBL implementation in my v2 patch. Best regards Johannes > Let me know what you think. > > Cheers, > Ahmad > >> >> Best regards >> Johannes >> >> >>>> >>>>> static uint32_t *crc_table; >>>>> @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) >>>>> { >>>>> const unsigned char *buf = _buf; >>>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>>> if (!crc_table) >>>>> make_crc_table(); >>>>> #endif >>>>> @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) >>>>> { >>>>> const unsigned char *buf = _buf; >>>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>>> if (!crc_table) >>>>> make_crc_table(); >>>>> #endif >>>>> >>>>> --- >>>>> base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c >>>>> change-id: 20230829-crc32_in_pbl-4d824629d4e2 >>>>> >>>>> Best regards, >>>> >>> >> > -- Pengutronix e.K. | Johannes Zink | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] crypto: crc32: make crc32 available in PBL 2023-08-29 12:05 ` Johannes Zink @ 2023-08-29 12:17 ` Ahmad Fatoum 0 siblings, 0 replies; 7+ messages in thread From: Ahmad Fatoum @ 2023-08-29 12:17 UTC (permalink / raw) To: Johannes Zink, patchwork-jzi; +Cc: Barebox Mailing List Hello Johannes, On 29.08.23 14:05, Johannes Zink wrote: > Hi Ahmad, > > On 8/29/23 13:08, Ahmad Fatoum wrote: >> On 29.08.23 12:55, Johannes Zink wrote: >>> On 8/29/23 12:45, Ahmad Fatoum wrote: >>>>>> +lwl-$(CONFIG_CRC32_EARLY) += crc32.o >>>>> >>>>> pbl-obj- is the correct prefix. lwl- means pbl- if we have PBL >>>>> support at all and obj- otherwise (for legacy systems without PBL), >>>>> while pbl-obj- is equivalent to duplicating the line once with pbl- >>>>> and once with obj- >>>> >>>> s/pbl-obj-/obj-pbl-/ :) >> >> Sorry, had a small brain fart here. >> >> You didn't remove the original obj-, >> so now lwl- either expands to and extra obj- or to pbl-. >> >> obj-pbl- makes sense when you use the same symbol for both PBL and >> barebox proper, but as you're introducing a new symbol, you can >> leave it as lwl- or make it pbl- for explicitness. >> > > ok. I think I won't glob it in obj-pbl, as one might want to explicitly add crc32.o only for PBL or only for barebox-proper. Linker garbage collection throws away any unreferenced function, so the extra Kconfig symbol only saves a little bit of compile time. Given that we don't have special symbols for the other CRC/SHA implementation used in PBL, it might be out of place to have one for CRC32, but not for the others. > >>> ack, gonna fix this for v2. >>> >>>>> >>>>>> obj-$(CONFIG_DIGEST_SHA384_GENERIC) += sha4.o >>>>>> obj-$(CONFIG_DIGEST_SHA512_GENERIC) += sha4.o >>>>>> obj-y += memneq.o >>>>>> diff --git a/crypto/crc32.c b/crypto/crc32.c >>>>>> index 95cb2212db2b..284d39351682 100644 >>>>>> --- a/crypto/crc32.c >>>>>> +++ b/crypto/crc32.c >>>>>> @@ -22,7 +22,7 @@ >>>>>> #define STATIC static inline >>>>>> #endif >>>>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>>> >>>>> You could also replace the dynamic allocation with a static array initialized >>>>> to zero. That way you can have a dynamic crc table even in PBL without affecting >>>>> image size as the BSS is not part of the image. >>> >>> ack. Is this ok? >>> >>> #ifdef __PBL__ >>> static uint32_t _crc_table_memory[sizeof(uint32_t) * 256] = { 0 }; >> >> The array is 256 elements, not 1024 elements. Explicit intialization >> is unnecessary. > > good catch. thanks. > >> >>> #endif >>> >>> static void *alloc_crc_table() { >>> #ifdef __PBL__ >>> return _crc_table_memory; >>> #else >>> return xmalloc(sizeof(uint32_t) * 256); >>> #endif >>> } >>> >>> If so, I can change it for v2. >> >> My idea was to drop the allocation altogether by using BSS. >> If you do this, you should not need any __PBL__ checking at all. > > I guess that should work indeed. Do I need to explicitly mark the array with a prefix to force it into BSS (sorry, I am not too familiar with the barebox linker scripts...)? By default, any variable with static storage duration that is not initialized to a non-zero value, will be allocated inside .bss. .bss in barebox and elsewhere is allocated at runtime and zeroed. This avoids the need to allocate zeroes in the binary. The CRC code uses a table with values != zero, so it can't be elided. >> Either you have CONFIG_DYNAMIC_CRC_TABLE and the table is dynamically >> filled in bss on first access or you have CONFIG_DYNAMIC_CRC_TABLE=n >> and the table is already there and need not be allocated. >> >> On a second thought, I am not sure if we want a table at all in PBL. >> Do you do a lot of CRC32 computation? Maybe we should just not use >> a table at all in PBL and just calculate a single crc32? >> That's what Sascha did here: >> >> 2d13b856604b ("crc: Add PBL variant for crc_itu_t()") >> > > For my specific usecase I need to check a single CRC32 once, so probably there is the same or even more overhead to dynamically filling the Table than just doing a single CRC32 computation. Maybe more overhead than using the precompiled, pre-filled table, but I fully acknowledge that size matters in PBL. > > I like Sascha's approach, so let me try to do a dynamic computation for the PBL implementation in my v2 patch. Ye, adding a PBL only implementation sounds like the best way to go about it then. Cheers, Ahmad > > Best regards > Johannes > >> Let me know what you think. >> >> Cheers, >> Ahmad >> >>> >>> Best regards >>> Johannes >>> >>> >>>>> >>>>>> static uint32_t *crc_table; >>>>>> @@ -145,7 +145,7 @@ STATIC uint32_t crc32(uint32_t crc, const void *_buf, unsigned int len) >>>>>> { >>>>>> const unsigned char *buf = _buf; >>>>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>>>> if (!crc_table) >>>>>> make_crc_table(); >>>>>> #endif >>>>>> @@ -171,7 +171,7 @@ STATIC uint32_t crc32_no_comp(uint32_t crc, const void *_buf, unsigned int len) >>>>>> { >>>>>> const unsigned char *buf = _buf; >>>>>> -#ifdef CONFIG_DYNAMIC_CRC_TABLE >>>>>> +#if defined(CONFIG_DYNAMIC_CRC_TABLE) && !defined(__PBL__) >>>>>> if (!crc_table) >>>>>> make_crc_table(); >>>>>> #endif >>>>>> >>>>>> --- >>>>>> base-commit: bef38b18eeb5d2f1fac334fb8b831e47261e099c >>>>>> change-id: 20230829-crc32_in_pbl-4d824629d4e2 >>>>>> >>>>>> Best regards, >>>>> >>>> >>> >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-08-29 12:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-29 9:28 [PATCH] crypto: crc32: make crc32 available in PBL Johannes Zink 2023-08-29 10:29 ` Ahmad Fatoum 2023-08-29 10:45 ` Ahmad Fatoum 2023-08-29 10:55 ` Johannes Zink 2023-08-29 11:08 ` Ahmad Fatoum 2023-08-29 12:05 ` Johannes Zink 2023-08-29 12:17 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox