On 26.07.17 00:01, Rob Clark wrote:
On Tue, Jul 25, 2017 at 5:42 PM, Alexander Graf <ag...@suse.de> wrote:
On 25.07.17 23:26, Rob Clark wrote:
On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.g...@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.g...@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.g...@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 <ag...@suse.de>
wrote:
On 25.07.17 14:08, Rob Clark wrote:
On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <ag...@suse.de>
wrote:
On 25.07.17 13:10, Rob Clark wrote:
On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <ag...@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 <robdcl...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
