Re: [RFC PATCH 1/2] Introduce coroutines framework

[Michal Simek <michal.simek@amd.com>]

On 1/21/25 14:15, Jerome Forissier wrote:
> Hi Michal,
> 
> On 1/21/25 12:44, Michal Simek wrote:
>>
>>
>> On 1/20/25 14:50, Jerome Forissier wrote:
>>> Adds the COROUTINES Kconfig symbol which introduces a new internal API
>>> for coroutines support. As explained in the Kconfig file, this is meant
>>> to provide some kind of cooperative multi-tasking with the goal to
>>> improve performance by overlapping lengthy operations.
>>>
>>> The API as well as the implementation is very much inspired from libaco
>>> [1]. The reference implementation is simplified to remove all things
>>> not needed in U-Boot, the coding style is updated, and the aco_ prefix
>>> is replaced by co_.
>>>
>>> I believe the stack handling could be simplified: the stack of the main
>>> coroutine could probably probably be used by the secondary coroutines
>>> instead of allocating a new stack dynamically.
>>>
>>> Only i386, x86_64 and aarch64 are supported at the moment. Other
>>> architectures need to provide a _co_switch() function in assembly.
>>>
>>> Only aarch64 has been tested.
>>
>> I can't see the reason why to keep x86 around if it is not tested.
> 
> OK, I'll drop x86 in v2.
>   
>> Licenses should be cleared for all files.
> 
> I will add "SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later"
> to the coroutines C implementation (include/coroutines.h and
> lib/coroutines.c) since it originates from libaco which is Apache-2.0
> then was modified by me for U-Boot. I will add GPL-2.0-or-later to
> arch/arm/cpu/armv8/co_switch.S since it is a new file written by me.
> 
> 
>> Also I think this mostly should target full u-boot and not really TPL/SPL as of today.
> 
> I agree, but why do you think it targets xPL? efi_init_obj_list() is
> called as a result of "printenv -e" in the main U-Boot for example.
> I mentioned in the cover letter that I think coroutines could be
> beneficial in other places too.

It is build for SPL too if you look at files which are touched.
I think you should define separate symbols for SPL/TPL.


> 
>>
>> I have applied this patch to measure time difference on kv260 and kd240 and pretty much the time is the same.
>>
>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>> index 94160f4bd86c..18d2260b8c11 100644
>> --- a/lib/efi_loader/efi_setup.c
>> +++ b/lib/efi_loader/efi_setup.c
>> @@ -258,6 +258,7 @@ extern int udelay_yield;
>>   efi_status_t efi_init_obj_list(void)
>>   {
>>          efi_status_t ret = EFI_SUCCESS;
>> +       ulong t0, t1;
>>   #if CONFIG_IS_ENABLED(COROUTINES)
>>          struct co_stack *stk = NULL;
>>          struct co *main_co = NULL;
>> @@ -272,6 +273,7 @@ efi_status_t efi_init_obj_list(void)
>>          /* Set up console modes */
>>          efi_setup_console_size();
>>
>> +       t0 = timer_get_us();
>>   #if CONFIG_IS_ENABLED(COROUTINES)
>>          main_co = co_create(NULL, NULL, 0, NULL, NULL);
>>          if (!main_co) {
>> @@ -333,6 +335,9 @@ efi_status_t efi_init_obj_list(void)
>>                          efi_obj_list_initialized = ret;
>>          }
>>   #endif
>> +       t1 = timer_get_us();
>> +
>> +       printf("time counted %ld\n", t1 - t0);
>>
>>          /* Initialize variable services */
>>          ret = efi_init_variables();
>>
>> Please correct me if you have measured it differently.
> 
> That's fine. Do you have a SD card inserted in your board?

yes I have. I do use xilinx_zynqmp_kria_defconfig only.

> If not, the efi_disks_register() call is very quick and
> therefore it makes little difference if it's run
> sequentially or in parallel with efi_tcg2_register().

I think in my case where distro boot runs things are initialized already that's 
why diff is not there.

Thanks,
Michal