* [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