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

Tue Jul 25 21:26:23 UTC 2017

On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 07/25/2017 09:42 PM, Rob Clark wrote:
>> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>>> 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())
>>>
>>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>>> timer events this includes a notify_function checking the keyboard and
>>> in future I want to check the watchdog in a notify_function.
>>>
>>> Please, consider that in the notify functions any number of UEFI API
>>> functions will be called by the EFI application which themselves may
>>> signal events which in turn call notify_functions in which again UEFI
>>> API functions may be called.
>>
>> sure, one way or another I don't want to call a notifier from nested
>> EFI_ENTER()'s..
>
> We call EFI_EXIT before calling the notifier function and
> call EFI_ENTER after returning form the notifier function.
>
> So currently we always switch gd correctly. An nested EFI_ENTER simply
> does not occur.

yes, I understand how it currently works.. although I think an
EFI_CALLBACK() macro that did the equiv (with extra error checking if
we allow re-entrant EFI_ENTER()) would be clearer.  It would make the
callbacks to UEFI world easy to spot and make the code a bit more
concise.

> I still do not understand why you need to change anything here.
>
>>
>> so either EFI_CALLBACK() should detect the condition and yell loudly,
>> or it should detect it and queue up callbacks until we are not
>> nested..
>
> Your suggestions are getting more and more complicated.
>
> I would always go for easily maintainable code and not for
> sophistication. Please, stick to the KISS principle.
>
> If you want to call functions internally as you described in the first
> mail in this thread, do not call the UEFI functions but supply separate
> entry points.

The issue that I am trying to address at the moment is that
efi_load_image(), to load an image specified by file_path (as opposed
to already loaded into source_buffer, ie. the source_buffer==NULL
case, which UEFI spec allows) requires using a handful of APIs from
the EFI file interface, and also requires the
simple-file-system-protocol, to actually read the image specified by
file_path into a buffer.

We could go down the path of continuing to add _ext functions.. but it
is a bit ugly, and seems like it would keep growing worse.  And it
requires exposing functions from different modules which could
otherwise remain static.  All for a case that won't happen (for these
particular APIs at least) or at least is easy to detect.  Keeping the
_ext functions for only the special cases that could lead to a
callback into UEFI world, and avoiding them in the normal case, seems
cleaner.  It isn't terribly complicated to implement, and it is easy
to detect programming errors.

At a *minimum* I'd say that the current EFI_ENTRY()/EFI_EXIT() should
detect nesting (since the result if you screw that up is a less than
obvious crash).  And an EFI_CALLBACK() macro would make the callback
into UEFI world more obvious via grep.  Once you have that, it is
trivial to allow nesting and detect problematic EFI_CALLBACK().

BR,
-R