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

[Rob Clark]

On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
<xypron.glpk at gmx.de> wrote:
> 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.
>

Does WaitForEvent or anything else that dispatches events dispatch
more than one event, or is it just one event and then return to UEFI
world to poll again?  We aren't really multi-threaded so there can't
really be async callbacks.

Either way, I think we can make this work by changing
EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
non-nested EFI_ENTER() case we don't have to do anything special (and
the nested case probably should never happen in places where we need
EFI_CALLBACK())

BR,
-R