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

Rob Clark
2017-07-25T22:01:45Z

On Tue, Jul 25, 2017 at 5:42 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.07.17 23:26, Rob Clark wrote:
>>
>> On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.glpk@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@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@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@suse.de>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@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@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.

I guess that would mean no malloc/printf/etc.. seems like just
*asking* for someone to screw up ;-)

But I suppose we could do it at a finer granularity.. ie.

efi_load_image(...)
{
EFI_ENTER(..);

if (!source_buffer) {
UEFI(ret = fs->open_volume(fs, &f));

while (fp) {
UEFI(ret = f->open(f, &f2, fp->str));
fp = efi_dp_next(fp);
UEFI(ret = f->close(f));
f = f2;
}

UEFI(ret = f->getinfo(f, &efi_file_info_guid, &bs, info));
...

UEFI(ret = f->read(f, ...));
}

...
}

And ofc, convert existing callbacks to use UEFI() macro to make it it
consistent in the code when things are going back to UEFI world.. ie.
basically treat these just like callbacks.

There is more malloc, etc (and when debugging, printf()) in between
this all, so I'm more a fan of a one-line macro rather than individual
EFI_EXIT/EFI_ENTER. Plus it makes it harder to screw up early returns
and that sorta thing.

and I'm still all for adding some error checking to
EFI_ENTER/EFI_EXIT/UEFI macros to debug misuse more easily.

BR,
-R