efi_loader: allow efi_loader code to call EFI interfaces

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 <[email protected]>

---
  lib/efi_loader/efi_boottime.c | 5 +++++
  1 file changed, 5 insertions(+)

lib/efi_loader/efi_boottime.c +5 -0

index 8735b1f418..a113124708 100644
@@ -47,6 +47,7 @@ static struct efi_configuration_table __efi_runtime_data efi_conf_table[2];
   * EFI callback entry/exit.
   */
  static volatile void *efi_gd, *app_gd;
+static int entry_level = 0;
  #endif
 
  /* Called from do_bootefi_exec() */

@@ -61,6 +62,8 @@ void efi_save_gd(void)
  void efi_restore_gd(void)
  {
  #ifdef CONFIG_ARM
+	if (entry_level++)
+		return;
  	/* Only restore if we're already in EFI context */
  	if (!efi_gd)
  		return;

@@ -75,6 +78,8 @@ void efi_restore_gd(void)
  efi_status_t efi_exit_func(efi_status_t ret)
  {
  #ifdef CONFIG_ARM
+	if (--entry_level)
+		return ret;
  	gd = app_gd;
  #endif
 

Quoted 8 lines · Rob Clark

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.

Quoted 4 lines

(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 <[email protected]>

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

Alex

Quoted 34 lines

---
  lib/efi_loader/efi_boottime.c | 5 +++++
  1 file changed, 5 insertions(+)

lib/efi_loader/efi_boottime.c +5 -0

index 8735b1f418..a113124708 100644
@@ -47,6 +47,7 @@ static struct efi_configuration_table __efi_runtime_data
 efi_conf_table[2];
   * EFI callback entry/exit.
   */
  static volatile void *efi_gd, *app_gd;
+static int entry_level = 0;
  #endif
    /* Called from do_bootefi_exec() */

@@ -61,6 +62,8 @@ void efi_save_gd(void)
  void efi_restore_gd(void)
  {
  #ifdef CONFIG_ARM
+       if (entry_level++)
+               return;
        /* Only restore if we're already in EFI context */
        if (!efi_gd)
                return;

@@ -75,6 +78,8 @@ void efi_restore_gd(void)
  efi_status_t efi_exit_func(efi_status_t ret)
  {
  #ifdef CONFIG_ARM
+       if (--entry_level)
+               return ret;
        gd = app_gd;
  #endif
 

Quoted 26 lines · Alexander Graf

Quoted 8 lines · Rob Clark

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.

Quoted 4 lines

(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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 44 lines · Rob Clark

Quoted 35 lines · Alexander Graf

Quoted 9 lines · Rob Clark

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.

Quoted 4 lines

(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 <[email protected]>

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.

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 61 lines · Alexander Graf

Quoted 44 lines · Rob Clark

Quoted 35 lines · Alexander Graf

Quoted 9 lines · Rob Clark

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.

Quoted 4 lines

(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 <[email protected]>

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.

Quoted 6 lines

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

Quoted 3 lines

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

Quoted 8 lines

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?

Quoted 83 lines · Rob Clark

Quoted 75 lines · Alexander Graf

Quoted 51 lines · Rob Clark

Quoted 40 lines · Alexander Graf

Quoted 12 lines · Rob Clark

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.

Quoted 4 lines

(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 <[email protected]>

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.

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

I personally think having _ext functions in anything exposed to UEFI payloads makes more sense.

Quoted 93 lines · Alexander Graf

Quoted 83 lines · Rob Clark

Quoted 75 lines · Alexander Graf

Quoted 51 lines · Rob Clark

Quoted 40 lines · Alexander Graf

Quoted 12 lines · Rob Clark

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.

Quoted 4 lines

(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 <[email protected]>

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.

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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()??

Quoted 4 lines

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

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

Quoted 3 lines · Rob Clark

On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <[email protected]> wrote:

Quoted 5 lines

Quoted 3 lines

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?

Quoted 3 lines

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

Quoted 6 lines · Alexander Graf

Quoted 3 lines · Rob Clark

On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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?

well, maybe not all, but for efi_load_image() from a device-path to work, we need to be able to use simple-filesystem-protocol and a bunch of the file-protocol interfaces.. so it is definitely more messy.

Quoted 16 lines

Quoted 3 lines

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.

Quoted 2 lines

If you manage to solve that, we don't need any ext functions or reference counters ;).

It would be nice to not have this gd mess.. although I'm going to leave it to someone else who understands better why we do that in the first place to solve. I have a big pile of display-handover

(clk/powerdomain/display-controller-state) issues on my TODO list, and I'm just trying to get the bootloader sorted to the point where I can tackle that. Not to mention a big pile of things todo in mesa, and my actual $day_job.

I think with the EFI interfaces I need to use for efi_load_image, it should be easy enough to keep any of those from calling back into the UEFI world, and we could do some sort of EFI_CALLBACK() macro with some extra error checking to safeguard instead of the EFI_EXIT()+callback+EFI_ENTER() pattern.. and while it is a bit annoying to have to be careful about that, I think it would work ok if no one has a solution for the gd mess in sight.

Quoted 116 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 44 lines · Alexander Graf

Quoted 14 lines · Rob Clark

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.

Quoted 5 lines

(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 <[email protected]>

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.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines

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?

Quoted 3 lines

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

Quoted 125 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 44 lines · Alexander Graf

Quoted 14 lines · Rob Clark

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.

Quoted 5 lines

(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 <[email protected]>

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.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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()??

Quoted 1 line

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.

Quoted 126 lines · Heinrich Schuchardt

Quoted 125 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 44 lines · Alexander Graf

Quoted 14 lines · Rob Clark

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.

Quoted 5 lines

(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 <[email protected]>

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.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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()??

Quoted 9 lines

Quoted 1 line

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

Quoted 143 lines · Rob Clark

Quoted 126 lines · Heinrich Schuchardt

Quoted 125 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 44 lines · Alexander Graf

Quoted 14 lines · Rob Clark

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.

Quoted 5 lines

(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 <[email protected]>

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.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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()??

Quoted 9 lines

Quoted 1 line

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.

Quoted 5 lines

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.

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

Best regards

Heinrich

Quoted 154 lines · Heinrich Schuchardt

Quoted 143 lines · Rob Clark

Quoted 126 lines · Heinrich Schuchardt

Quoted 125 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 44 lines · Alexander Graf

Quoted 14 lines · Rob Clark

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.

Quoted 5 lines

(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 <[email protected]>

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.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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()??

Quoted 9 lines

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 4 lines

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

Quoted 6 lines

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

Best regards

Heinrich

Quoted 160 lines · Rob Clark

Quoted 158 lines · Heinrich Schuchardt

Quoted 143 lines · Rob Clark

Quoted 126 lines · Heinrich Schuchardt

Quoted 125 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 44 lines · Alexander Graf

Quoted 19 lines · Rob Clark

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 <[email protected]>

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.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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()??

Quoted 9 lines

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

I still do not understand why you need to change anything here.

Quoted 4 lines

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.

Quoted 161 lines · Heinrich Schuchardt

Quoted 160 lines · Rob Clark

Quoted 158 lines · Heinrich Schuchardt

Quoted 143 lines · Rob Clark

Quoted 126 lines · Heinrich Schuchardt

Quoted 125 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 105 lines · Alexander Graf

Quoted 95 lines · Rob Clark

Quoted 85 lines · Alexander Graf

Quoted 58 lines · Rob Clark

Quoted 53 lines · Alexander Graf

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 <[email protected]>

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

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 5 lines

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()??

Quoted 9 lines

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

I still do not understand why you need to change anything here.

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 257 lines · Rob Clark

Quoted 215 lines · Heinrich Schuchardt

Quoted 203 lines · Rob Clark

Quoted 200 lines · Heinrich Schuchardt

Quoted 187 lines · Rob Clark

Quoted 170 lines · Heinrich Schuchardt

Quoted 165 lines · Alexander Graf

Quoted 153 lines · Rob Clark

Quoted 135 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 107 lines · Alexander Graf

Quoted 80 lines · Rob Clark

On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <[email protected]>
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 <[email protected]>

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.

Quoted 9 lines

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.

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 6 lines

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()??

Quoted 1 line

Maybe, I'll leave Heinrich to comment on that.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 8 lines

I still do not understand why you need to change anything here.

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 284 lines · Alexander Graf

Quoted 257 lines · Rob Clark

Quoted 215 lines · Heinrich Schuchardt

Quoted 203 lines · Rob Clark

Quoted 200 lines · Heinrich Schuchardt

Quoted 187 lines · Rob Clark

Quoted 170 lines · Heinrich Schuchardt

Quoted 165 lines · Alexander Graf

Quoted 153 lines · Rob Clark

Quoted 135 lines · Alexander Graf

Quoted 121 lines · Rob Clark

Quoted 116 lines · Alexander Graf

On 25.07.17 13:10, Rob Clark wrote:

On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <[email protected]>
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 <[email protected]>

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.

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 6 lines

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()??

Quoted 1 line

Maybe, I'll leave Heinrich to comment on that.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 8 lines

I still do not understand why you need to change anything here.

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 9 lines

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();

Quoted 3 lines

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.

Quoted 339 lines · Rob Clark

Quoted 335 lines · Alexander Graf

Quoted 300 lines · Rob Clark

Quoted 253 lines · Heinrich Schuchardt

Quoted 245 lines · Rob Clark

Quoted 234 lines · Heinrich Schuchardt

Quoted 217 lines · Rob Clark

Quoted 198 lines · Heinrich Schuchardt

Quoted 191 lines · Alexander Graf

Quoted 175 lines · Rob Clark

Quoted 153 lines · Alexander Graf

Quoted 144 lines · Rob Clark

On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <[email protected]>
wrote:

On 25.07.17 13:10, Rob Clark wrote:

On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf
<[email protected]>
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 <[email protected]>

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?

Quoted 8 lines

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.

Quoted 8 lines

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()??

Quoted 1 line

Maybe, I'll leave Heinrich to comment on that.

Quoted 9 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

sure, one way or another I don't want to call a notifier from nested EFI_ENTER()'s..

Quoted 5 lines

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.

Quoted 9 lines

I still do not understand why you need to change anything here.

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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

Quoted 26 lines

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, ...));
     }

     ...

}

Quoted 8 lines

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.

Quoted 2 lines

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