[U-Boot] [PATCH] efi_loader: allow efi_loader code to call EFI interfaces

[Heinrich Schuchardt]

On 07/25/2017 04:28 PM, Alexander Graf wrote:
> 
> 
> On 25.07.17 14:28, Rob Clark wrote:
>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>
>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf at suse.de>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>> from a
>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>> it is
>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>> Allow
>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>> simplifies
>>>>>>>> things.
>>>>>>>>
>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>> to call
>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>
>>>>>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> So there are 2 ways to do this:
>>>>>>>
>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>> functions
>>>>>>>
>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>> normal
>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>> efi_loader.
>>>>>>>
>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>> approach
>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>
>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>> version
>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>> reentrant,
>>>>>>> I'll
>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>> though :).
>>>>>>>
>>>>>>
>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>> functions
>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>> variants.
>>>>>
>>>>>
>>>>>
>>>>> Yes, but imagine a case like this:
>>>>>
>>>>> open_protocol
>>>>>     -> open()
>>>>>       -> random_junk()
>>>>>         -> efi_timer_check()
>>>>>           -> timer notifier
>>>>>             -> console print()
>>>>>
>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>> needs
>>>>> to
>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>> print()
>>>>> need to be run in U-Boot context.
>>>>>
>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>> that
>>>>> 2
>>>>> weeks down some corner case nobody thought of falls apart because
>>>>> people
>>>>> don't manage to fully grasp the code flow anymore.
>>>>>
>>>>
>>>> ugg.. gd is a gd pita ;-)
>>>>
>>>> how many places do we have callbacks into UEFI context like this?  If
>>>> just one or two maybe we can suppress them while in u-boot context and
>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>
>>>
>>> What do you mean by suppressing? We need to call those helpers
>>> synchronously. And yes, we can probably hand massage the refcount on the
>>> ones that we remember, but I'm just afraid that the whole thing will
>>> be so
>>> complicated eventually that nobody understands what's going on.
>>
>> Maybe suppress isn't the right word.. I was thinking of delaying the
>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>> from the PoV of the UEFI world, it is still synchronous.
>>
>> I haven't looked at the efi_event stuff until just now, but from a
>> 30sec look, maybe efi_signal_event() could just put the event on a
>> list of signalled events and not immediately call
>> event->notify_function(), and handle the calling of notify_function()
>> in the last EFI_EXIT()??
> 
> Maybe, I'll leave Heinrich to comment on that.

Let the application call WaitForEvent for a keyboard event.
Rob suggests that neither timer events nor any other events are served
until the user possibly presses a key.

No, we have to call notification functions immediately to serve time
critical communication like network traffic.

Best regards

Heinrich

> 
>>
>>> I personally think having _ext functions in anything exposed to UEFI
>>> payloads makes more sense.
>>>
>>
>> having 2x all the interfaces for file related protocols would be
> 
> Why 2x the interfaces?
> 
>> unfortunate.  (Also currently they can stay static in efi_file.c and
>> just be called via same efi_file_handle fxn ptrs that the UEFI world
>> would be using, which is kinda nice.)
> 
> Ah, that indeed is nice.
> 
> So, there is a third option if you want to tackle it:
> 
>   3) Remove register variable need for gd
> 
> If gd is a simple global variable rather than a register variable, we
> wouldn't have that problem. On x86 it's just a variable for example. I
> don't know why arm and aarch64 have it as register variable, but I can't
> see an immediate need for it.
> 
> If you manage to solve that, we don't need any ext functions or
> reference counters ;).
> 
> 
> Alex
>