[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Alexander Graf]

On 18.01.18 19:28, Heinrich Schuchardt wrote:
> On 01/18/2018 11:05 AM, Alexander Graf wrote:
>>
>>
>> On 18.01.18 10:52, Heinrich Schuchardt wrote:
>>>
>>>
>>> On 01/18/2018 10:24 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 18.01.18 08:24, Heinrich Schuchardt wrote:
>>>>> Avoid a failed assertion when an EFI app calls an EFI app.
>>>>>
>>>>> Avoid that the indent level increases when calling 'bootefi hello'
>>>>> repeatedly.
>>>>>
>>>>> Avoid negative indent level when an EFI app calls an EFI app that
>>>>> calls an EFI app (e.g. iPXE loads grub which starts the kernel).
>>>>>
>>>>> Return the status code of a loaded image that returns without
>>>>> calling the Exit boot service.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>> ---
>>>>>   lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
>>>>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_boottime.c
>>>>> b/lib/efi_loader/efi_boottime.c
>>>>> index 2c5499e0c8..538cc55d20 100644
>>>>> --- a/lib/efi_loader/efi_boottime.c
>>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>>> @@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI
>>>>> efi_start_image(efi_handle_t image_handle,
>>>>>       asmlinkage ulong (*entry)(efi_handle_t image_handle,
>>>>>                     struct efi_system_table *st);
>>>>>       struct efi_loaded_image *info = image_handle;
>>>>> +    efi_status_t ret;
>>>>>         EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size,
>>>>> exit_data);
>>>>>       entry = info->reserved;
>>>>> @@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI
>>>>> efi_start_image(efi_handle_t image_handle,
>>>>>       /* call the image! */
>>>>>       if (setjmp(&info->exit_jmp)) {
>>>>>           /* We returned from the child image */
>>>>> +#ifdef CONFIG_ARM
>>>>> +        /* efi_exit() called efi_restore_gd() */
>>>>> +        gd = app_gd;
>>>>> +#endif
>>>>> +        /* Execute the return part of EFI_CALL */
>>>>> +        assert(__efi_entry_check());
>>>>> +        debug("%sEFI: %lu returned by started image\n",
>>>>> +              __efi_nesting_dec(),
>>>>
>>>> I don't understand why you need to decrease the nesting level here after
>>>> the other rework. You're now calling EFI_ENTRY/EFI_EXIT in all normal
>>>> paths when going in/out of an application, no?
>>>
>>> bootefi -> level 0
>>> ** EFI application running at level 0
>>> LoadImage EFI_ENTRY -> level 1
>>> LoadImage EFI_EXIT -> level 0
>>> ** EFI application running at  level 0
>>
>> -- base level at 0
>>
>>> StartImage EFI_ENTRY -> level 1
>>
>> This is decreased in EFI_EXIT of StartImage
>>
>>> StartImage EFI_CALL -> level 2
>>
>> This is the one that needs manual decrease then?
>>
>>> Exit EFI_ENTRY -> level 3
>>
>> Gets decreased right below in Exit again
>>
>>> Exit EFI_EXIT -> level 2
>>> longjmp -> level 2
>>> __efi_nesting_dec() -> level 1
>>> StartImage EFI_EXIT -> level 0
>>
>> --- base level again
>>
>>
>> So I guess the problem is that we never get into the second half of
>> EFI_CALL when ->exit() gets called because of the longjmp.
>>
>> Can you please add a comment explaining that rationale with a hint to
>> EFI_CALL and that all we do is execute the lower half of it manually
>> again because it got interrupted by the longjmp?
>>
> 
> I already wrote:
> /* Execute the return part of EFI_CALL */

Could you make that slightly more verbose? Which EFI_CALL? Why do we
need to execute the return path?

Keep in mind that people 2 years down the road should be able to grasp
what the code does :).


Alex