[U-Boot] [PATCH] efi_loader: allow efi_loader code to call EFI interfaces
Alexander Graf
agraf at suse.de
Tue Jul 25 22:10:56 UTC 2017
On 26.07.17 00:01, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 5:42 PM, Alexander Graf <agraf at 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 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.
>
> I guess that would mean no malloc/printf/etc.. seems like just
> *asking* for someone to screw up ;-)
Well, you have to choose a world to be in. You still have the EFI alloc
and print methods ;).
>
> 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.
Yup, that works too.
> and I'm still all for adding some error checking to
> EFI_ENTER/EFI_EXIT/UEFI macros to debug misuse more easily.
Sure, but that's an orthogonal discussion :).
Alex
More information about the U-Boot
mailing list