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

[Rob Clark]

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..

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..

I think the first option is probably ok.  In the special cases where
we might need to dispatch events, _ext() fxns should still be used
until gd goes away.

BR,
-R

> So make no assumptions whatsoever about what is going to happen in a
> notify function or about the level of nesting.
>
> Best regards
>
> Heinrich