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

Tue Jul 25 21:42:05 UTC 2017

On 25.07.17 23:26, Rob Clark wrote:
> 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.

Maybe we could turn all of this around and you just call EFI_EXIT() 
before calling all the callbacks? That way you're back in UEFI context 
and are free to call anything that doesn't need gd.

So something like

   EFI_EXIT();
   file_proto *f = bs->find_protocol();
   file_foo *g = f->bar();
   if (!g) return EFI_AMAZING_ERROR;
   ...
   EFI_ENTER();

That way the fs loading code that really only deals with UEFI calls 
would be self-contained. We do something similar already with 
efi_signal_event() today.

Alex