Re: [PATCH] crypto: crc32: make crc32 available in PBL

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |