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

[Alexander Graf]

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.

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