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

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

[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 <robdclark@gmail.com>
---
 lib/efi_loader/efi_boottime.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 8735b1f418..a113124708 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -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
 
-- 
2.13.0

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

[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 <robdclark@gmail.com>

So there are 2 ways to do this:

   1) Keep a refcount, only transition when passing the 0 level
   2) Explicitly split functions with ENTRY/EXIT from their core functions

So far we used approach 2, splitting functions that are used by both 
internal and external code into _ext (for externally called) and normal 
functions. You can see this pattern quite a few times throughout efi_loader.

I definitely did try the refcount approach back when I decided for 
approach 2 and it failed on me in some case, but I can't remember where.

Either way, we should definitely be consistent. Either we always use 
refcounting or we shouldn't bother with it. So if you can make a version 
work where all _ext variants disappear and we're magically reentrant, 
I'll be happy to take that. I'm fairly sure it'll break somewhere though :).


Alex

> ---
>   lib/efi_loader/efi_boottime.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 8735b1f418..a113124708 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -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
>   
> 

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

[Rob Clark]

On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.07.17 01:47, Rob Clark wrote:
>>
>> To implement efi_load_image() for the case of loading an image from a
>> device path rather than image already loaded into source_buffer, it is
>> convenient to be able to re-use simple-file-system and efi-file
>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().  Allow
>> entry into EFI interfaces to be recursive, since this greatly simplifies
>> things.
>>
>> (And hypothetically this would be needed anyways to allow grub to call
>> into interfaces installed by something outside of grub.)
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
>
> So there are 2 ways to do this:
>
>   1) Keep a refcount, only transition when passing the 0 level
>   2) Explicitly split functions with ENTRY/EXIT from their core functions
>
> So far we used approach 2, splitting functions that are used by both
> internal and external code into _ext (for externally called) and normal
> functions. You can see this pattern quite a few times throughout efi_loader.
>
> I definitely did try the refcount approach back when I decided for approach
> 2 and it failed on me in some case, but I can't remember where.
>
> Either way, we should definitely be consistent. Either we always use
> refcounting or we shouldn't bother with it. So if you can make a version
> work where all _ext variants disappear and we're magically reentrant, I'll
> be happy to take that. I'm fairly sure it'll break somewhere though :).
>

for load_image via file-path, we end up needing a *bunch* of functions
normally called via EFI.. so it is going to be a lot more _ext
variants.

BR,
-R

>
> Alex
>
>
>> ---
>>   lib/efi_loader/efi_boottime.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 8735b1f418..a113124708 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -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
>>

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

[Alexander Graf]

On 25.07.17 13:10, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 25.07.17 01:47, Rob Clark wrote:
>>>
>>> To implement efi_load_image() for the case of loading an image from a
>>> device path rather than image already loaded into source_buffer, it is
>>> convenient to be able to re-use simple-file-system and efi-file
>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().  Allow
>>> entry into EFI interfaces to be recursive, since this greatly simplifies
>>> things.
>>>
>>> (And hypothetically this would be needed anyways to allow grub to call
>>> into interfaces installed by something outside of grub.)
>>>
>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>
>>
>> So there are 2 ways to do this:
>>
>>    1) Keep a refcount, only transition when passing the 0 level
>>    2) Explicitly split functions with ENTRY/EXIT from their core functions
>>
>> So far we used approach 2, splitting functions that are used by both
>> internal and external code into _ext (for externally called) and normal
>> functions. You can see this pattern quite a few times throughout efi_loader.
>>
>> I definitely did try the refcount approach back when I decided for approach
>> 2 and it failed on me in some case, but I can't remember where.
>>
>> Either way, we should definitely be consistent. Either we always use
>> refcounting or we shouldn't bother with it. So if you can make a version
>> work where all _ext variants disappear and we're magically reentrant, I'll
>> be happy to take that. I'm fairly sure it'll break somewhere though :).
>>
> 
> for load_image via file-path, we end up needing a *bunch* of functions
> normally called via EFI.. so it is going to be a lot more _ext
> variants.

Yes, but imagine a case like this:

open_protocol
   -> open()
     -> random_junk()
       -> efi_timer_check()
         -> timer notifier
           -> console print()

Here the refcounting will simply fail on us, as the timer notifier needs 
to be run in UEFI context while efi_timer_check() as well as console 
print() need to be run in U-Boot context.

And if we start to mix and mesh the 2 approaches, I can promise you that 
2 weeks down some corner case nobody thought of falls apart because 
people don't manage to fully grasp the code flow anymore.


Alex

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

[Rob Clark]

On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.07.17 13:10, Rob Clark wrote:
>>
>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>
>>>>
>>>> To implement efi_load_image() for the case of loading an image from a
>>>> device path rather than image already loaded into source_buffer, it is
>>>> convenient to be able to re-use simple-file-system and efi-file
>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().  Allow
>>>> entry into EFI interfaces to be recursive, since this greatly simplifies
>>>> things.
>>>>
>>>> (And hypothetically this would be needed anyways to allow grub to call
>>>> into interfaces installed by something outside of grub.)
>>>>
>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>
>>>
>>>
>>> So there are 2 ways to do this:
>>>
>>>    1) Keep a refcount, only transition when passing the 0 level
>>>    2) Explicitly split functions with ENTRY/EXIT from their core
>>> functions
>>>
>>> So far we used approach 2, splitting functions that are used by both
>>> internal and external code into _ext (for externally called) and normal
>>> functions. You can see this pattern quite a few times throughout
>>> efi_loader.
>>>
>>> I definitely did try the refcount approach back when I decided for
>>> approach
>>> 2 and it failed on me in some case, but I can't remember where.
>>>
>>> Either way, we should definitely be consistent. Either we always use
>>> refcounting or we shouldn't bother with it. So if you can make a version
>>> work where all _ext variants disappear and we're magically reentrant,
>>> I'll
>>> be happy to take that. I'm fairly sure it'll break somewhere though :).
>>>
>>
>> for load_image via file-path, we end up needing a *bunch* of functions
>> normally called via EFI.. so it is going to be a lot more _ext
>> variants.
>
>
> Yes, but imagine a case like this:
>
> open_protocol
>   -> open()
>     -> random_junk()
>       -> efi_timer_check()
>         -> timer notifier
>           -> console print()
>
> Here the refcounting will simply fail on us, as the timer notifier needs to
> be run in UEFI context while efi_timer_check() as well as console print()
> need to be run in U-Boot context.
>
> And if we start to mix and mesh the 2 approaches, I can promise you that 2
> weeks down some corner case nobody thought of falls apart because people
> don't manage to fully grasp the code flow anymore.
>

ugg.. gd is a gd pita ;-)

how many places do we have callbacks into UEFI context like this?  If
just one or two maybe we can suppress them while in u-boot context and
handle in efi_exit_func() when entry_count drops to zero?

BR,
-R

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

[Alexander Graf]

On 25.07.17 14:08, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 25.07.17 13:10, Rob Clark wrote:
>>>
>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>
>>>>>
>>>>> To implement efi_load_image() for the case of loading an image from a
>>>>> device path rather than image already loaded into source_buffer, it is
>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().  Allow
>>>>> entry into EFI interfaces to be recursive, since this greatly simplifies
>>>>> things.
>>>>>
>>>>> (And hypothetically this would be needed anyways to allow grub to call
>>>>> into interfaces installed by something outside of grub.)
>>>>>
>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>
>>>>
>>>>
>>>> So there are 2 ways to do this:
>>>>
>>>>     1) Keep a refcount, only transition when passing the 0 level
>>>>     2) Explicitly split functions with ENTRY/EXIT from their core
>>>> functions
>>>>
>>>> So far we used approach 2, splitting functions that are used by both
>>>> internal and external code into _ext (for externally called) and normal
>>>> functions. You can see this pattern quite a few times throughout
>>>> efi_loader.
>>>>
>>>> I definitely did try the refcount approach back when I decided for
>>>> approach
>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>
>>>> Either way, we should definitely be consistent. Either we always use
>>>> refcounting or we shouldn't bother with it. So if you can make a version
>>>> work where all _ext variants disappear and we're magically reentrant,
>>>> I'll
>>>> be happy to take that. I'm fairly sure it'll break somewhere though :).
>>>>
>>>
>>> for load_image via file-path, we end up needing a *bunch* of functions
>>> normally called via EFI.. so it is going to be a lot more _ext
>>> variants.
>>
>>
>> Yes, but imagine a case like this:
>>
>> open_protocol
>>    -> open()
>>      -> random_junk()
>>        -> efi_timer_check()
>>          -> timer notifier
>>            -> console print()
>>
>> Here the refcounting will simply fail on us, as the timer notifier needs to
>> be run in UEFI context while efi_timer_check() as well as console print()
>> need to be run in U-Boot context.
>>
>> And if we start to mix and mesh the 2 approaches, I can promise you that 2
>> weeks down some corner case nobody thought of falls apart because people
>> don't manage to fully grasp the code flow anymore.
>>
> 
> ugg.. gd is a gd pita ;-)
> 
> how many places do we have callbacks into UEFI context like this?  If
> just one or two maybe we can suppress them while in u-boot context and
> handle in efi_exit_func() when entry_count drops to zero?

What do you mean by suppressing? We need to call those helpers 
synchronously. And yes, we can probably hand massage the refcount on the 
ones that we remember, but I'm just afraid that the whole thing will be 
so complicated eventually that nobody understands what's going on.

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


Alex

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

[Rob Clark]

On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.07.17 14:08, Rob Clark wrote:
>>
>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>
>>>>
>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> To implement efi_load_image() for the case of loading an image from a
>>>>>> device path rather than image already loaded into source_buffer, it is
>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>> Allow
>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>> simplifies
>>>>>> things.
>>>>>>
>>>>>> (And hypothetically this would be needed anyways to allow grub to call
>>>>>> into interfaces installed by something outside of grub.)
>>>>>>
>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> So there are 2 ways to do this:
>>>>>
>>>>>     1) Keep a refcount, only transition when passing the 0 level
>>>>>     2) Explicitly split functions with ENTRY/EXIT from their core
>>>>> functions
>>>>>
>>>>> So far we used approach 2, splitting functions that are used by both
>>>>> internal and external code into _ext (for externally called) and normal
>>>>> functions. You can see this pattern quite a few times throughout
>>>>> efi_loader.
>>>>>
>>>>> I definitely did try the refcount approach back when I decided for
>>>>> approach
>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>
>>>>> Either way, we should definitely be consistent. Either we always use
>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>> version
>>>>> work where all _ext variants disappear and we're magically reentrant,
>>>>> I'll
>>>>> be happy to take that. I'm fairly sure it'll break somewhere though :).
>>>>>
>>>>
>>>> for load_image via file-path, we end up needing a *bunch* of functions
>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>> variants.
>>>
>>>
>>>
>>> Yes, but imagine a case like this:
>>>
>>> open_protocol
>>>    -> open()
>>>      -> random_junk()
>>>        -> efi_timer_check()
>>>          -> timer notifier
>>>            -> console print()
>>>
>>> Here the refcounting will simply fail on us, as the timer notifier needs
>>> to
>>> be run in UEFI context while efi_timer_check() as well as console print()
>>> need to be run in U-Boot context.
>>>
>>> And if we start to mix and mesh the 2 approaches, I can promise you that
>>> 2
>>> weeks down some corner case nobody thought of falls apart because people
>>> don't manage to fully grasp the code flow anymore.
>>>
>>
>> ugg.. gd is a gd pita ;-)
>>
>> how many places do we have callbacks into UEFI context like this?  If
>> just one or two maybe we can suppress them while in u-boot context and
>> handle in efi_exit_func() when entry_count drops to zero?
>
>
> What do you mean by suppressing? We need to call those helpers
> synchronously. And yes, we can probably hand massage the refcount on the
> ones that we remember, but I'm just afraid that the whole thing will be so
> complicated eventually that nobody understands what's going on.

Maybe suppress isn't the right word.. I was thinking of delaying the
callback until EFI_EXIT() that transitions back to the UEFI world.  So
from the PoV of the UEFI world, it is still synchronous.

I haven't looked at the efi_event stuff until just now, but from a
30sec look, maybe efi_signal_event() could just put the event on a
list of signalled events and not immediately call
event->notify_function(), and handle the calling of notify_function()
in the last EFI_EXIT()??

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

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

[Alexander Graf]

On 25.07.17 14:28, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 25.07.17 14:08, Rob Clark wrote:
>>>
>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>
>>>>>
>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> To implement efi_load_image() for the case of loading an image from a
>>>>>>> device path rather than image already loaded into source_buffer, it is
>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>> Allow
>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>> simplifies
>>>>>>> things.
>>>>>>>
>>>>>>> (And hypothetically this would be needed anyways to allow grub to call
>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>
>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> So there are 2 ways to do this:
>>>>>>
>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>> functions
>>>>>>
>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>> internal and external code into _ext (for externally called) and normal
>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>> efi_loader.
>>>>>>
>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>> approach
>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>
>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>> version
>>>>>> work where all _ext variants disappear and we're magically reentrant,
>>>>>> I'll
>>>>>> be happy to take that. I'm fairly sure it'll break somewhere though :).
>>>>>>
>>>>>
>>>>> for load_image via file-path, we end up needing a *bunch* of functions
>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>> variants.
>>>>
>>>>
>>>>
>>>> Yes, but imagine a case like this:
>>>>
>>>> open_protocol
>>>>     -> open()
>>>>       -> random_junk()
>>>>         -> efi_timer_check()
>>>>           -> timer notifier
>>>>             -> console print()
>>>>
>>>> Here the refcounting will simply fail on us, as the timer notifier needs
>>>> to
>>>> be run in UEFI context while efi_timer_check() as well as console print()
>>>> need to be run in U-Boot context.
>>>>
>>>> And if we start to mix and mesh the 2 approaches, I can promise you that
>>>> 2
>>>> weeks down some corner case nobody thought of falls apart because people
>>>> don't manage to fully grasp the code flow anymore.
>>>>
>>>
>>> ugg.. gd is a gd pita ;-)
>>>
>>> how many places do we have callbacks into UEFI context like this?  If
>>> just one or two maybe we can suppress them while in u-boot context and
>>> handle in efi_exit_func() when entry_count drops to zero?
>>
>>
>> What do you mean by suppressing? We need to call those helpers
>> synchronously. And yes, we can probably hand massage the refcount on the
>> ones that we remember, but I'm just afraid that the whole thing will be so
>> complicated eventually that nobody understands what's going on.
> 
> Maybe suppress isn't the right word.. I was thinking of delaying the
> callback until EFI_EXIT() that transitions back to the UEFI world.  So
> from the PoV of the UEFI world, it is still synchronous.
> 
> I haven't looked at the efi_event stuff until just now, but from a
> 30sec look, maybe efi_signal_event() could just put the event on a
> list of signalled events and not immediately call
> event->notify_function(), and handle the calling of notify_function()
> in the last EFI_EXIT()??

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

> 
>> I personally think having _ext functions in anything exposed to UEFI
>> payloads makes more sense.
>>
> 
> having 2x all the interfaces for file related protocols would be

Why 2x the interfaces?

> unfortunate.  (Also currently they can stay static in efi_file.c and
> just be called via same efi_file_handle fxn ptrs that the UEFI world
> would be using, which is kinda nice.)

Ah, that indeed is nice.

So, there is a third option if you want to tackle it:

   3) Remove register variable need for gd

If gd is a simple global variable rather than a register variable, we 
wouldn't have that problem. On x86 it's just a variable for example. I 
don't know why arm and aarch64 have it as register variable, but I can't 
see an immediate need for it.

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


Alex

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

[Rob Clark]

On Tue, Jul 25, 2017 at 10:28 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.07.17 14:28, Rob Clark wrote:
>>
>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>> 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.

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


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.

BR,
-R

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

[Alexander Graf]

On 25.07.17 18:18, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 10:28 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 25.07.17 14:28, Rob Clark wrote:
>>>
>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>> 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.
> 
>>> 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 ;).
> 
> 
> 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

Yeah, that was my plan too when I introduced efi_loader. Maybe I'll find 
time to tackle it one day. But it's certainly nothing short-term :(.

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

No worries, we're all in the same boat :)

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

If you want to macrorize things to make them more readable, give it a 
try :).

I really don't mind which path to take, but I want to ensure we're 
consistent and only have a single way of dealing with entry/exit to 
worry about.


Alex

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

[Heinrich Schuchardt]

On 07/25/2017 04:28 PM, Alexander Graf wrote:
> 
> 
> On 25.07.17 14:28, Rob Clark wrote:
>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>
>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>> from a
>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>> it is
>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>> Allow
>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>> simplifies
>>>>>>>> things.
>>>>>>>>
>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>> to call
>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>
>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> So there are 2 ways to do this:
>>>>>>>
>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>> functions
>>>>>>>
>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>> normal
>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>> efi_loader.
>>>>>>>
>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>> approach
>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>
>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>> version
>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>> reentrant,
>>>>>>> I'll
>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>> though :).
>>>>>>>
>>>>>>
>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>> functions
>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>> variants.
>>>>>
>>>>>
>>>>>
>>>>> Yes, but imagine a case like this:
>>>>>
>>>>> open_protocol
>>>>>     -> open()
>>>>>       -> random_junk()
>>>>>         -> efi_timer_check()
>>>>>           -> timer notifier
>>>>>             -> console print()
>>>>>
>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>> needs
>>>>> to
>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>> print()
>>>>> need to be run in U-Boot context.
>>>>>
>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>> that
>>>>> 2
>>>>> weeks down some corner case nobody thought of falls apart because
>>>>> people
>>>>> don't manage to fully grasp the code flow anymore.
>>>>>
>>>>
>>>> ugg.. gd is a gd pita ;-)
>>>>
>>>> how many places do we have callbacks into UEFI context like this?  If
>>>> just one or two maybe we can suppress them while in u-boot context and
>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>
>>>
>>> What do you mean by suppressing? We need to call those helpers
>>> synchronously. And yes, we can probably hand massage the refcount on the
>>> ones that we remember, but I'm just afraid that the whole thing will
>>> be so
>>> complicated eventually that nobody understands what's going on.
>>
>> Maybe suppress isn't the right word.. I was thinking of delaying the
>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>> from the PoV of the UEFI world, it is still synchronous.
>>
>> I haven't looked at the efi_event stuff until just now, but from a
>> 30sec look, maybe efi_signal_event() could just put the event on a
>> list of signalled events and not immediately call
>> event->notify_function(), and handle the calling of notify_function()
>> in the last EFI_EXIT()??
> 
> Maybe, I'll leave Heinrich to comment on that.

Let the application call WaitForEvent for a keyboard event.
Rob suggests that neither timer events nor any other events are served
until the user possibly presses a key.

No, we have to call notification functions immediately to serve time
critical communication like network traffic.

Best regards

Heinrich

> 
>>
>>> I personally think having _ext functions in anything exposed to UEFI
>>> payloads makes more sense.
>>>
>>
>> having 2x all the interfaces for file related protocols would be
> 
> Why 2x the interfaces?
> 
>> unfortunate.  (Also currently they can stay static in efi_file.c and
>> just be called via same efi_file_handle fxn ptrs that the UEFI world
>> would be using, which is kinda nice.)
> 
> Ah, that indeed is nice.
> 
> So, there is a third option if you want to tackle it:
> 
>   3) Remove register variable need for gd
> 
> If gd is a simple global variable rather than a register variable, we
> wouldn't have that problem. On x86 it's just a variable for example. I
> don't know why arm and aarch64 have it as register variable, but I can't
> see an immediate need for it.
> 
> If you manage to solve that, we don't need any ext functions or
> reference counters ;).
> 
> 
> Alex
> 

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

[Rob Clark]

On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
<xypron.glpk@gmx.de> wrote:
> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>
>>
>> On 25.07.17 14:28, Rob Clark wrote:
>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>
>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>>> from a
>>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>>> it is
>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>>> Allow
>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>> simplifies
>>>>>>>>> things.
>>>>>>>>>
>>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>>> to call
>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>
>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> So there are 2 ways to do this:
>>>>>>>>
>>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>>> functions
>>>>>>>>
>>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>>> normal
>>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>>> efi_loader.
>>>>>>>>
>>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>>> approach
>>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>>
>>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>>> version
>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>> reentrant,
>>>>>>>> I'll
>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>> though :).
>>>>>>>>
>>>>>>>
>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>> functions
>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>> variants.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Yes, but imagine a case like this:
>>>>>>
>>>>>> open_protocol
>>>>>>     -> open()
>>>>>>       -> random_junk()
>>>>>>         -> efi_timer_check()
>>>>>>           -> timer notifier
>>>>>>             -> console print()
>>>>>>
>>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>>> needs
>>>>>> to
>>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>>> print()
>>>>>> need to be run in U-Boot context.
>>>>>>
>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>>> that
>>>>>> 2
>>>>>> weeks down some corner case nobody thought of falls apart because
>>>>>> people
>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>
>>>>>
>>>>> ugg.. gd is a gd pita ;-)
>>>>>
>>>>> how many places do we have callbacks into UEFI context like this?  If
>>>>> just one or two maybe we can suppress them while in u-boot context and
>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>
>>>>
>>>> What do you mean by suppressing? We need to call those helpers
>>>> synchronously. And yes, we can probably hand massage the refcount on the
>>>> ones that we remember, but I'm just afraid that the whole thing will
>>>> be so
>>>> complicated eventually that nobody understands what's going on.
>>>
>>> Maybe suppress isn't the right word.. I was thinking of delaying the
>>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>>> from the PoV of the UEFI world, it is still synchronous.
>>>
>>> I haven't looked at the efi_event stuff until just now, but from a
>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>> list of signalled events and not immediately call
>>> event->notify_function(), and handle the calling of notify_function()
>>> in the last EFI_EXIT()??
>>
>> Maybe, I'll leave Heinrich to comment on that.
>
> Let the application call WaitForEvent for a keyboard event.
> Rob suggests that neither timer events nor any other events are served
> until the user possibly presses a key.
>
> No, we have to call notification functions immediately to serve time
> critical communication like network traffic.
>

Does WaitForEvent or anything else that dispatches events dispatch
more than one event, or is it just one event and then return to UEFI
world to poll again?  We aren't really multi-threaded so there can't
really be async callbacks.

Either way, I think we can make this work by changing
EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
non-nested EFI_ENTER() case we don't have to do anything special (and
the nested case probably should never happen in places where we need
EFI_CALLBACK())

BR,
-R

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

[Heinrich Schuchardt]

On 07/25/2017 07:52 PM, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
> <xypron.glpk@gmx.de> wrote:
>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>
>>>
>>> On 25.07.17 14:28, Rob Clark wrote:
>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>
>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>>>> from a
>>>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>>>> it is
>>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>>>> Allow
>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>> simplifies
>>>>>>>>>> things.
>>>>>>>>>>
>>>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>>>> to call
>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>
>>>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>>>> functions
>>>>>>>>>
>>>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>>>> normal
>>>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>>>> efi_loader.
>>>>>>>>>
>>>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>>>> approach
>>>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>>>
>>>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>>>> version
>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>> reentrant,
>>>>>>>>> I'll
>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>> though :).
>>>>>>>>>
>>>>>>>>
>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>> functions
>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>> variants.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes, but imagine a case like this:
>>>>>>>
>>>>>>> open_protocol
>>>>>>>     -> open()
>>>>>>>       -> random_junk()
>>>>>>>         -> efi_timer_check()
>>>>>>>           -> timer notifier
>>>>>>>             -> console print()
>>>>>>>
>>>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>>>> needs
>>>>>>> to
>>>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>>>> print()
>>>>>>> need to be run in U-Boot context.
>>>>>>>
>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>>>> that
>>>>>>> 2
>>>>>>> weeks down some corner case nobody thought of falls apart because
>>>>>>> people
>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>
>>>>>>
>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>
>>>>>> how many places do we have callbacks into UEFI context like this?  If
>>>>>> just one or two maybe we can suppress them while in u-boot context and
>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>
>>>>>
>>>>> What do you mean by suppressing? We need to call those helpers
>>>>> synchronously. And yes, we can probably hand massage the refcount on the
>>>>> ones that we remember, but I'm just afraid that the whole thing will
>>>>> be so
>>>>> complicated eventually that nobody understands what's going on.
>>>>
>>>> Maybe suppress isn't the right word.. I was thinking of delaying the
>>>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>
>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>> list of signalled events and not immediately call
>>>> event->notify_function(), and handle the calling of notify_function()
>>>> in the last EFI_EXIT()??
>>>
>>> Maybe, I'll leave Heinrich to comment on that.
>>
>> Let the application call WaitForEvent for a keyboard event.
>> Rob suggests that neither timer events nor any other events are served
>> until the user possibly presses a key.
>>
>> No, we have to call notification functions immediately to serve time
>> critical communication like network traffic.
>>
> 
> Does WaitForEvent or anything else that dispatches events dispatch
> more than one event, or is it just one event and then return to UEFI
> world to poll again?  We aren't really multi-threaded so there can't
> really be async callbacks.
> 
> Either way, I think we can make this work by changing
> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
> non-nested EFI_ENTER() case we don't have to do anything special (and
> the nested case probably should never happen in places where we need
> EFI_CALLBACK())

WaitForEvent and CheckEvent calls will serve all notify_functions of
timer events this includes a notify_function checking the keyboard and
in future I want to check the watchdog in a notify_function.

Please, consider that in the notify functions any number of UEFI API
functions will be called by the EFI application which themselves may
signal events which in turn call notify_functions in which again UEFI
API functions may be called.

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

Best regards

Heinrich

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

[Rob Clark]

On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 07/25/2017 07:52 PM, Rob Clark wrote:
>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>> <xypron.glpk@gmx.de> wrote:
>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>
>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>>>>> from a
>>>>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>>>>> it is
>>>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>> Allow
>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>>> simplifies
>>>>>>>>>>> things.
>>>>>>>>>>>
>>>>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>>>>> to call
>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>
>>>>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>>>>> functions
>>>>>>>>>>
>>>>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>>>>> normal
>>>>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>>>>> efi_loader.
>>>>>>>>>>
>>>>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>>>>> approach
>>>>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>>>>
>>>>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>>>>> version
>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>> reentrant,
>>>>>>>>>> I'll
>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>> though :).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>> functions
>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>>> variants.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>
>>>>>>>> open_protocol
>>>>>>>>     -> open()
>>>>>>>>       -> random_junk()
>>>>>>>>         -> efi_timer_check()
>>>>>>>>           -> timer notifier
>>>>>>>>             -> console print()
>>>>>>>>
>>>>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>>>>> needs
>>>>>>>> to
>>>>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>>>>> print()
>>>>>>>> need to be run in U-Boot context.
>>>>>>>>
>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>>>>> that
>>>>>>>> 2
>>>>>>>> weeks down some corner case nobody thought of falls apart because
>>>>>>>> people
>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>
>>>>>>>
>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>
>>>>>>> how many places do we have callbacks into UEFI context like this?  If
>>>>>>> just one or two maybe we can suppress them while in u-boot context and
>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>
>>>>>>
>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>> synchronously. And yes, we can probably hand massage the refcount on the
>>>>>> ones that we remember, but I'm just afraid that the whole thing will
>>>>>> be so
>>>>>> complicated eventually that nobody understands what's going on.
>>>>>
>>>>> Maybe suppress isn't the right word.. I was thinking of delaying the
>>>>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>
>>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>>> list of signalled events and not immediately call
>>>>> event->notify_function(), and handle the calling of notify_function()
>>>>> in the last EFI_EXIT()??
>>>>
>>>> Maybe, I'll leave Heinrich to comment on that.
>>>
>>> Let the application call WaitForEvent for a keyboard event.
>>> Rob suggests that neither timer events nor any other events are served
>>> until the user possibly presses a key.
>>>
>>> No, we have to call notification functions immediately to serve time
>>> critical communication like network traffic.
>>>
>>
>> Does WaitForEvent or anything else that dispatches events dispatch
>> more than one event, or is it just one event and then return to UEFI
>> world to poll again?  We aren't really multi-threaded so there can't
>> really be async callbacks.
>>
>> Either way, I think we can make this work by changing
>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>> non-nested EFI_ENTER() case we don't have to do anything special (and
>> the nested case probably should never happen in places where we need
>> EFI_CALLBACK())
>
> WaitForEvent and CheckEvent calls will serve all notify_functions of
> timer events this includes a notify_function checking the keyboard and
> in future I want to check the watchdog in a notify_function.
>
> Please, consider that in the notify functions any number of UEFI API
> functions will be called by the EFI application which themselves may
> signal events which in turn call notify_functions in which again UEFI
> API functions may be called.

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

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

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

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

[Heinrich Schuchardt]

On 07/25/2017 09:42 PM, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>
>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>>>>>> from a
>>>>>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>>>>>> it is
>>>>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>>> Allow
>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>>>> simplifies
>>>>>>>>>>>> things.
>>>>>>>>>>>>
>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>>>>>> to call
>>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>>
>>>>>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>>>>>> functions
>>>>>>>>>>>
>>>>>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>>>>>> normal
>>>>>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>>>>>> efi_loader.
>>>>>>>>>>>
>>>>>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>>>>>> approach
>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>>>>>
>>>>>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>>>>>> version
>>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>>> reentrant,
>>>>>>>>>>> I'll
>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>>> though :).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>>> functions
>>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>>>> variants.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>>
>>>>>>>>> open_protocol
>>>>>>>>>     -> open()
>>>>>>>>>       -> random_junk()
>>>>>>>>>         -> efi_timer_check()
>>>>>>>>>           -> timer notifier
>>>>>>>>>             -> console print()
>>>>>>>>>
>>>>>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>>>>>> needs
>>>>>>>>> to
>>>>>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>>>>>> print()
>>>>>>>>> need to be run in U-Boot context.
>>>>>>>>>
>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>>>>>> that
>>>>>>>>> 2
>>>>>>>>> weeks down some corner case nobody thought of falls apart because
>>>>>>>>> people
>>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>>
>>>>>>>>
>>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>>
>>>>>>>> how many places do we have callbacks into UEFI context like this?  If
>>>>>>>> just one or two maybe we can suppress them while in u-boot context and
>>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>>
>>>>>>>
>>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>>> synchronously. And yes, we can probably hand massage the refcount on the
>>>>>>> ones that we remember, but I'm just afraid that the whole thing will
>>>>>>> be so
>>>>>>> complicated eventually that nobody understands what's going on.
>>>>>>
>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying the
>>>>>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>>
>>>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>>>> list of signalled events and not immediately call
>>>>>> event->notify_function(), and handle the calling of notify_function()
>>>>>> in the last EFI_EXIT()??
>>>>>
>>>>> Maybe, I'll leave Heinrich to comment on that.
>>>>
>>>> Let the application call WaitForEvent for a keyboard event.
>>>> Rob suggests that neither timer events nor any other events are served
>>>> until the user possibly presses a key.
>>>>
>>>> No, we have to call notification functions immediately to serve time
>>>> critical communication like network traffic.
>>>>
>>>
>>> Does WaitForEvent or anything else that dispatches events dispatch
>>> more than one event, or is it just one event and then return to UEFI
>>> world to poll again?  We aren't really multi-threaded so there can't
>>> really be async callbacks.
>>>
>>> Either way, I think we can make this work by changing
>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>>> non-nested EFI_ENTER() case we don't have to do anything special (and
>>> the nested case probably should never happen in places where we need
>>> EFI_CALLBACK())
>>
>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>> timer events this includes a notify_function checking the keyboard and
>> in future I want to check the watchdog in a notify_function.
>>
>> Please, consider that in the notify functions any number of UEFI API
>> functions will be called by the EFI application which themselves may
>> signal events which in turn call notify_functions in which again UEFI
>> API functions may be called.
> 
> sure, one way or another I don't want to call a notifier from nested
> EFI_ENTER()'s..

We call EFI_EXIT before calling the notifier function and
call EFI_ENTER after returning form the notifier function.

So currently we always switch gd correctly. An nested EFI_ENTER simply
does not occur.

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.

Best regards

Heinrich

> 
> 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
> 
>> So make no assumptions whatsoever about what is going to happen in a
>> notify function or about the level of nesting.
>>
>> Best regards
>>
>> Heinrich
> 

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

[Rob Clark]

On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 07/25/2017 09:42 PM, Rob Clark wrote:
>> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>>>> <xypron.glpk@gmx.de> wrote:
>>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>>>
>>>>>>
>>>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>>>>>>> from a
>>>>>>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>>>>>>> it is
>>>>>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>>>> Allow
>>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>>>>> simplifies
>>>>>>>>>>>>> things.
>>>>>>>>>>>>>
>>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>>>>>>> to call
>>>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>>>
>>>>>>>>>>>>      1) Keep a refcount, only transition when passing the 0 level
>>>>>>>>>>>>      2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>>>>>>> functions
>>>>>>>>>>>>
>>>>>>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>>>>>>> normal
>>>>>>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>>>>>>> efi_loader.
>>>>>>>>>>>>
>>>>>>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>>>>>>> approach
>>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>>>>>>
>>>>>>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>>>>>>> version
>>>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>>>> reentrant,
>>>>>>>>>>>> I'll
>>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>>>> though :).
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>>>> functions
>>>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>>>>> variants.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>>>
>>>>>>>>>> open_protocol
>>>>>>>>>>     -> open()
>>>>>>>>>>       -> random_junk()
>>>>>>>>>>         -> efi_timer_check()
>>>>>>>>>>           -> timer notifier
>>>>>>>>>>             -> console print()
>>>>>>>>>>
>>>>>>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>>>>>>> needs
>>>>>>>>>> to
>>>>>>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>>>>>>> print()
>>>>>>>>>> need to be run in U-Boot context.
>>>>>>>>>>
>>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>>>>>>> that
>>>>>>>>>> 2
>>>>>>>>>> weeks down some corner case nobody thought of falls apart because
>>>>>>>>>> people
>>>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>>>
>>>>>>>>> how many places do we have callbacks into UEFI context like this?  If
>>>>>>>>> just one or two maybe we can suppress them while in u-boot context and
>>>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>>>
>>>>>>>>
>>>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>>>> synchronously. And yes, we can probably hand massage the refcount on the
>>>>>>>> ones that we remember, but I'm just afraid that the whole thing will
>>>>>>>> be so
>>>>>>>> complicated eventually that nobody understands what's going on.
>>>>>>>
>>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying the
>>>>>>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>>>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>>>
>>>>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>>>>> list of signalled events and not immediately call
>>>>>>> event->notify_function(), and handle the calling of notify_function()
>>>>>>> in the last EFI_EXIT()??
>>>>>>
>>>>>> Maybe, I'll leave Heinrich to comment on that.
>>>>>
>>>>> Let the application call WaitForEvent for a keyboard event.
>>>>> Rob suggests that neither timer events nor any other events are served
>>>>> until the user possibly presses a key.
>>>>>
>>>>> No, we have to call notification functions immediately to serve time
>>>>> critical communication like network traffic.
>>>>>
>>>>
>>>> Does WaitForEvent or anything else that dispatches events dispatch
>>>> more than one event, or is it just one event and then return to UEFI
>>>> world to poll again?  We aren't really multi-threaded so there can't
>>>> really be async callbacks.
>>>>
>>>> Either way, I think we can make this work by changing
>>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>>>> non-nested EFI_ENTER() case we don't have to do anything special (and
>>>> the nested case probably should never happen in places where we need
>>>> EFI_CALLBACK())
>>>
>>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>>> timer events this includes a notify_function checking the keyboard and
>>> in future I want to check the watchdog in a notify_function.
>>>
>>> Please, consider that in the notify functions any number of UEFI API
>>> functions will be called by the EFI application which themselves may
>>> signal events which in turn call notify_functions in which again UEFI
>>> API functions may be called.
>>
>> sure, one way or another I don't want to call a notifier from nested
>> EFI_ENTER()'s..
>
> We call EFI_EXIT before calling the notifier function and
> call EFI_ENTER after returning form the notifier function.
>
> So currently we always switch gd correctly. An nested EFI_ENTER simply
> does not occur.

yes, I understand how it currently works.. although I think an
EFI_CALLBACK() macro that did the equiv (with extra error checking if
we allow re-entrant EFI_ENTER()) would be clearer.  It would make the
callbacks to UEFI world easy to spot and make the code a bit more
concise.

> I still do not understand why you need to change anything here.
>
>>
>> so either EFI_CALLBACK() should detect the condition and yell loudly,
>> or it should detect it and queue up callbacks until we are not
>> nested..
>
> Your suggestions are getting more and more complicated.
>
> I would always go for easily maintainable code and not for
> sophistication. Please, stick to the KISS principle.
>
> If you want to call functions internally as you described in the first
> mail in this thread, do not call the UEFI functions but supply separate
> entry points.

The issue that I am trying to address at the moment is that
efi_load_image(), to load an image specified by file_path (as opposed
to already loaded into source_buffer, ie. the source_buffer==NULL
case, which UEFI spec allows) requires using a handful of APIs from
the EFI file interface, and also requires the
simple-file-system-protocol, to actually read the image specified by
file_path into a buffer.

We could go down the path of continuing to add _ext functions.. but it
is a bit ugly, and seems like it would keep growing worse.  And it
requires exposing functions from different modules which could
otherwise remain static.  All for a case that won't happen (for these
particular APIs at least) or at least is easy to detect.  Keeping the
_ext functions for only the special cases that could lead to a
callback into UEFI world, and avoiding them in the normal case, seems
cleaner.  It isn't terribly complicated to implement, and it is easy
to detect programming errors.

At a *minimum* I'd say that the current EFI_ENTRY()/EFI_EXIT() should
detect nesting (since the result if you screw that up is a less than
obvious crash).  And an EFI_CALLBACK() macro would make the callback
into UEFI world more obvious via grep.  Once you have that, it is
trivial to allow nesting and detect problematic EFI_CALLBACK().


BR,
-R

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

[Alexander Graf]

On 25.07.17 23:26, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 07/25/2017 09:42 PM, Rob Clark wrote:
>>> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> To implement efi_load_image() for the case of loading an image
>>>>>>>>>>>>>> from a
>>>>>>>>>>>>>> device path rather than image already loaded into source_buffer,
>>>>>>>>>>>>>> it is
>>>>>>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file
>>>>>>>>>>>>>> interfaces.  But these are already using EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>>>>> Allow
>>>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>>>>>> simplifies
>>>>>>>>>>>>>> things.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow grub
>>>>>>>>>>>>>> to call
>>>>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>>>>
>>>>>>>>>>>>>       1) Keep a refcount, only transition when passing the 0 level
>>>>>>>>>>>>>       2) Explicitly split functions with ENTRY/EXIT from their core
>>>>>>>>>>>>> functions
>>>>>>>>>>>>>
>>>>>>>>>>>>> So far we used approach 2, splitting functions that are used by both
>>>>>>>>>>>>> internal and external code into _ext (for externally called) and
>>>>>>>>>>>>> normal
>>>>>>>>>>>>> functions. You can see this pattern quite a few times throughout
>>>>>>>>>>>>> efi_loader.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I definitely did try the refcount approach back when I decided for
>>>>>>>>>>>>> approach
>>>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember where.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Either way, we should definitely be consistent. Either we always use
>>>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a
>>>>>>>>>>>>> version
>>>>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>>>>> reentrant,
>>>>>>>>>>>>> I'll
>>>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>>>>> though :).
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>>>>> functions
>>>>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>>>>>> variants.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>>>>
>>>>>>>>>>> open_protocol
>>>>>>>>>>>      -> open()
>>>>>>>>>>>        -> random_junk()
>>>>>>>>>>>          -> efi_timer_check()
>>>>>>>>>>>            -> timer notifier
>>>>>>>>>>>              -> console print()
>>>>>>>>>>>
>>>>>>>>>>> Here the refcounting will simply fail on us, as the timer notifier
>>>>>>>>>>> needs
>>>>>>>>>>> to
>>>>>>>>>>> be run in UEFI context while efi_timer_check() as well as console
>>>>>>>>>>> print()
>>>>>>>>>>> need to be run in U-Boot context.
>>>>>>>>>>>
>>>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you
>>>>>>>>>>> that
>>>>>>>>>>> 2
>>>>>>>>>>> weeks down some corner case nobody thought of falls apart because
>>>>>>>>>>> people
>>>>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>>>>
>>>>>>>>>> how many places do we have callbacks into UEFI context like this?  If
>>>>>>>>>> just one or two maybe we can suppress them while in u-boot context and
>>>>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>>>>> synchronously. And yes, we can probably hand massage the refcount on the
>>>>>>>>> ones that we remember, but I'm just afraid that the whole thing will
>>>>>>>>> be so
>>>>>>>>> complicated eventually that nobody understands what's going on.
>>>>>>>>
>>>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying the
>>>>>>>> callback until EFI_EXIT() that transitions back to the UEFI world.  So
>>>>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>>>>
>>>>>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>>>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>>>>>> list of signalled events and not immediately call
>>>>>>>> event->notify_function(), and handle the calling of notify_function()
>>>>>>>> in the last EFI_EXIT()??
>>>>>>>
>>>>>>> Maybe, I'll leave Heinrich to comment on that.
>>>>>>
>>>>>> Let the application call WaitForEvent for a keyboard event.
>>>>>> Rob suggests that neither timer events nor any other events are served
>>>>>> until the user possibly presses a key.
>>>>>>
>>>>>> No, we have to call notification functions immediately to serve time
>>>>>> critical communication like network traffic.
>>>>>>
>>>>>
>>>>> Does WaitForEvent or anything else that dispatches events dispatch
>>>>> more than one event, or is it just one event and then return to UEFI
>>>>> world to poll again?  We aren't really multi-threaded so there can't
>>>>> really be async callbacks.
>>>>>
>>>>> Either way, I think we can make this work by changing
>>>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>>>>> non-nested EFI_ENTER() case we don't have to do anything special (and
>>>>> the nested case probably should never happen in places where we need
>>>>> EFI_CALLBACK())
>>>>
>>>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>>>> timer events this includes a notify_function checking the keyboard and
>>>> in future I want to check the watchdog in a notify_function.
>>>>
>>>> Please, consider that in the notify functions any number of UEFI API
>>>> functions will be called by the EFI application which themselves may
>>>> signal events which in turn call notify_functions in which again UEFI
>>>> API functions may be called.
>>>
>>> sure, one way or another I don't want to call a notifier from nested
>>> EFI_ENTER()'s..
>>
>> We call EFI_EXIT before calling the notifier function and
>> call EFI_ENTER after returning form the notifier function.
>>
>> So currently we always switch gd correctly. An nested EFI_ENTER simply
>> does not occur.
> 
> yes, I understand how it currently works.. although I think an
> EFI_CALLBACK() macro that did the equiv (with extra error checking if
> we allow re-entrant EFI_ENTER()) would be clearer.  It would make the
> callbacks to UEFI world easy to spot and make the code a bit more
> concise.
> 
>> I still do not understand why you need to change anything here.
>>
>>>
>>> so either EFI_CALLBACK() should detect the condition and yell loudly,
>>> or it should detect it and queue up callbacks until we are not
>>> nested..
>>
>> Your suggestions are getting more and more complicated.
>>
>> I would always go for easily maintainable code and not for
>> sophistication. Please, stick to the KISS principle.
>>
>> If you want to call functions internally as you described in the first
>> mail in this thread, do not call the UEFI functions but supply separate
>> entry points.
> 
> The issue that I am trying to address at the moment is that
> efi_load_image(), to load an image specified by file_path (as opposed
> to already loaded into source_buffer, ie. the source_buffer==NULL
> case, which UEFI spec allows) requires using a handful of APIs from
> the EFI file interface, and also requires the
> simple-file-system-protocol, to actually read the image specified by
> file_path into a buffer.
> 
> We could go down the path of continuing to add _ext functions.. but it
> is a bit ugly, and seems like it would keep growing worse.  And it
> requires exposing functions from different modules which could
> otherwise remain static.  All for a case that won't happen (for these
> particular APIs at least) or at least is easy to detect.  Keeping the
> _ext functions for only the special cases that could lead to a
> callback into UEFI world, and avoiding them in the normal case, seems
> cleaner.  It isn't terribly complicated to implement, and it is easy
> to detect programming errors.

Maybe we could turn all of this around and you just call EFI_EXIT() 
before calling all the callbacks? That way you're back in UEFI context 
and are free to call anything that doesn't need gd.

So something like

   EFI_EXIT();
   file_proto *f = bs->find_protocol();
   file_foo *g = f->bar();
   if (!g) return EFI_AMAZING_ERROR;
   ...
   EFI_ENTER();

That way the fs loading code that really only deals with UEFI calls 
would be self-contained. We do something similar already with 
efi_signal_event() today.


Alex

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

[Rob Clark]

On Tue, Jul 25, 2017 at 5:42 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.07.17 23:26, Rob Clark wrote:
>>
>> On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de>
>> wrote:
>>>
>>> On 07/25/2017 09:42 PM, Rob Clark wrote:
>>>>
>>>> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt
>>>> <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>>>>>
>>>>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>
>>>>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> To implement efi_load_image() for the case of loading an
>>>>>>>>>>>>>>> image
>>>>>>>>>>>>>>> from a
>>>>>>>>>>>>>>> device path rather than image already loaded into
>>>>>>>>>>>>>>> source_buffer,
>>>>>>>>>>>>>>> it is
>>>>>>>>>>>>>>> convenient to be able to re-use simple-file-system and
>>>>>>>>>>>>>>> efi-file
>>>>>>>>>>>>>>> interfaces.  But these are already using
>>>>>>>>>>>>>>> EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>>>>>> Allow
>>>>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>>>>>>> simplifies
>>>>>>>>>>>>>>> things.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow
>>>>>>>>>>>>>>> grub
>>>>>>>>>>>>>>> to call
>>>>>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>       1) Keep a refcount, only transition when passing the 0
>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>       2) Explicitly split functions with ENTRY/EXIT from their
>>>>>>>>>>>>>> core
>>>>>>>>>>>>>> functions
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So far we used approach 2, splitting functions that are used
>>>>>>>>>>>>>> by both
>>>>>>>>>>>>>> internal and external code into _ext (for externally called)
>>>>>>>>>>>>>> and
>>>>>>>>>>>>>> normal
>>>>>>>>>>>>>> functions. You can see this pattern quite a few times
>>>>>>>>>>>>>> throughout
>>>>>>>>>>>>>> efi_loader.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I definitely did try the refcount approach back when I decided
>>>>>>>>>>>>>> for
>>>>>>>>>>>>>> approach
>>>>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember
>>>>>>>>>>>>>> where.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Either way, we should definitely be consistent. Either we
>>>>>>>>>>>>>> always use
>>>>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make
>>>>>>>>>>>>>> a
>>>>>>>>>>>>>> version
>>>>>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>>>>>> reentrant,
>>>>>>>>>>>>>> I'll
>>>>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>>>>>> though :).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>>>>>> functions
>>>>>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>>>>>>> variants.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>>>>>
>>>>>>>>>>>> open_protocol
>>>>>>>>>>>>      -> open()
>>>>>>>>>>>>        -> random_junk()
>>>>>>>>>>>>          -> efi_timer_check()
>>>>>>>>>>>>            -> timer notifier
>>>>>>>>>>>>              -> console print()
>>>>>>>>>>>>
>>>>>>>>>>>> Here the refcounting will simply fail on us, as the timer
>>>>>>>>>>>> notifier
>>>>>>>>>>>> needs
>>>>>>>>>>>> to
>>>>>>>>>>>> be run in UEFI context while efi_timer_check() as well as
>>>>>>>>>>>> console
>>>>>>>>>>>> print()
>>>>>>>>>>>> need to be run in U-Boot context.
>>>>>>>>>>>>
>>>>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise
>>>>>>>>>>>> you
>>>>>>>>>>>> that
>>>>>>>>>>>> 2
>>>>>>>>>>>> weeks down some corner case nobody thought of falls apart
>>>>>>>>>>>> because
>>>>>>>>>>>> people
>>>>>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>>>>>
>>>>>>>>>>> how many places do we have callbacks into UEFI context like this?
>>>>>>>>>>> If
>>>>>>>>>>> just one or two maybe we can suppress them while in u-boot
>>>>>>>>>>> context and
>>>>>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>>>>>> synchronously. And yes, we can probably hand massage the refcount
>>>>>>>>>> on the
>>>>>>>>>> ones that we remember, but I'm just afraid that the whole thing
>>>>>>>>>> will
>>>>>>>>>> be so
>>>>>>>>>> complicated eventually that nobody understands what's going on.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying
>>>>>>>>> the
>>>>>>>>> callback until EFI_EXIT() that transitions back to the UEFI world.
>>>>>>>>> So
>>>>>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>>>>>
>>>>>>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>>>>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>>>>>>> list of signalled events and not immediately call
>>>>>>>>> event->notify_function(), and handle the calling of
>>>>>>>>> notify_function()
>>>>>>>>> in the last EFI_EXIT()??
>>>>>>>>
>>>>>>>>
>>>>>>>> Maybe, I'll leave Heinrich to comment on that.
>>>>>>>
>>>>>>>
>>>>>>> Let the application call WaitForEvent for a keyboard event.
>>>>>>> Rob suggests that neither timer events nor any other events are
>>>>>>> served
>>>>>>> until the user possibly presses a key.
>>>>>>>
>>>>>>> No, we have to call notification functions immediately to serve time
>>>>>>> critical communication like network traffic.
>>>>>>>
>>>>>>
>>>>>> Does WaitForEvent or anything else that dispatches events dispatch
>>>>>> more than one event, or is it just one event and then return to UEFI
>>>>>> world to poll again?  We aren't really multi-threaded so there can't
>>>>>> really be async callbacks.
>>>>>>
>>>>>> Either way, I think we can make this work by changing
>>>>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>>>>>> non-nested EFI_ENTER() case we don't have to do anything special (and
>>>>>> the nested case probably should never happen in places where we need
>>>>>> EFI_CALLBACK())
>>>>>
>>>>>
>>>>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>>>>> timer events this includes a notify_function checking the keyboard and
>>>>> in future I want to check the watchdog in a notify_function.
>>>>>
>>>>> Please, consider that in the notify functions any number of UEFI API
>>>>> functions will be called by the EFI application which themselves may
>>>>> signal events which in turn call notify_functions in which again UEFI
>>>>> API functions may be called.
>>>>
>>>>
>>>> sure, one way or another I don't want to call a notifier from nested
>>>> EFI_ENTER()'s..
>>>
>>>
>>> We call EFI_EXIT before calling the notifier function and
>>> call EFI_ENTER after returning form the notifier function.
>>>
>>> So currently we always switch gd correctly. An nested EFI_ENTER simply
>>> does not occur.
>>
>>
>> yes, I understand how it currently works.. although I think an
>> EFI_CALLBACK() macro that did the equiv (with extra error checking if
>> we allow re-entrant EFI_ENTER()) would be clearer.  It would make the
>> callbacks to UEFI world easy to spot and make the code a bit more
>> concise.
>>
>>> I still do not understand why you need to change anything here.
>>>
>>>>
>>>> so either EFI_CALLBACK() should detect the condition and yell loudly,
>>>> or it should detect it and queue up callbacks until we are not
>>>> nested..
>>>
>>>
>>> Your suggestions are getting more and more complicated.
>>>
>>> I would always go for easily maintainable code and not for
>>> sophistication. Please, stick to the KISS principle.
>>>
>>> If you want to call functions internally as you described in the first
>>> mail in this thread, do not call the UEFI functions but supply separate
>>> entry points.
>>
>>
>> The issue that I am trying to address at the moment is that
>> efi_load_image(), to load an image specified by file_path (as opposed
>> to already loaded into source_buffer, ie. the source_buffer==NULL
>> case, which UEFI spec allows) requires using a handful of APIs from
>> the EFI file interface, and also requires the
>> simple-file-system-protocol, to actually read the image specified by
>> file_path into a buffer.
>>
>> We could go down the path of continuing to add _ext functions.. but it
>> is a bit ugly, and seems like it would keep growing worse.  And it
>> requires exposing functions from different modules which could
>> otherwise remain static.  All for a case that won't happen (for these
>> particular APIs at least) or at least is easy to detect.  Keeping the
>> _ext functions for only the special cases that could lead to a
>> callback into UEFI world, and avoiding them in the normal case, seems
>> cleaner.  It isn't terribly complicated to implement, and it is easy
>> to detect programming errors.
>
>
> Maybe we could turn all of this around and you just call EFI_EXIT() before
> calling all the callbacks? That way you're back in UEFI context and are free
> to call anything that doesn't need gd.
>
> So something like
>
>   EFI_EXIT();
>   file_proto *f = bs->find_protocol();
>   file_foo *g = f->bar();
>   if (!g) return EFI_AMAZING_ERROR;
>   ...
>   EFI_ENTER();
>
> That way the fs loading code that really only deals with UEFI calls would be
> self-contained. We do something similar already with efi_signal_event()
> today.

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

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

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

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

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

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

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

    ...
  }

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

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

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

BR,
-R

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

[Alexander Graf]

On 26.07.17 00:01, Rob Clark wrote:
> On Tue, Jul 25, 2017 at 5:42 PM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 25.07.17 23:26, Rob Clark wrote:
>>>
>>> On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> wrote:
>>>>
>>>> On 07/25/2017 09:42 PM, Rob Clark wrote:
>>>>>
>>>>> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt
>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>>>>>>
>>>>>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>>>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>>
>>>>>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> To implement efi_load_image() for the case of loading an
>>>>>>>>>>>>>>>> image
>>>>>>>>>>>>>>>> from a
>>>>>>>>>>>>>>>> device path rather than image already loaded into
>>>>>>>>>>>>>>>> source_buffer,
>>>>>>>>>>>>>>>> it is
>>>>>>>>>>>>>>>> convenient to be able to re-use simple-file-system and
>>>>>>>>>>>>>>>> efi-file
>>>>>>>>>>>>>>>> interfaces.  But these are already using
>>>>>>>>>>>>>>>> EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>>>>>>> Allow
>>>>>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly
>>>>>>>>>>>>>>>> simplifies
>>>>>>>>>>>>>>>> things.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow
>>>>>>>>>>>>>>>> grub
>>>>>>>>>>>>>>>> to call
>>>>>>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>        1) Keep a refcount, only transition when passing the 0
>>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>>        2) Explicitly split functions with ENTRY/EXIT from their
>>>>>>>>>>>>>>> core
>>>>>>>>>>>>>>> functions
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So far we used approach 2, splitting functions that are used
>>>>>>>>>>>>>>> by both
>>>>>>>>>>>>>>> internal and external code into _ext (for externally called)
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> normal
>>>>>>>>>>>>>>> functions. You can see this pattern quite a few times
>>>>>>>>>>>>>>> throughout
>>>>>>>>>>>>>>> efi_loader.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I definitely did try the refcount approach back when I decided
>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>> approach
>>>>>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember
>>>>>>>>>>>>>>> where.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Either way, we should definitely be consistent. Either we
>>>>>>>>>>>>>>> always use
>>>>>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make
>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>> version
>>>>>>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>>>>>>> reentrant,
>>>>>>>>>>>>>>> I'll
>>>>>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>>>>>>> though :).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>>>>>>> functions
>>>>>>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext
>>>>>>>>>>>>>> variants.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> open_protocol
>>>>>>>>>>>>>       -> open()
>>>>>>>>>>>>>         -> random_junk()
>>>>>>>>>>>>>           -> efi_timer_check()
>>>>>>>>>>>>>             -> timer notifier
>>>>>>>>>>>>>               -> console print()
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here the refcounting will simply fail on us, as the timer
>>>>>>>>>>>>> notifier
>>>>>>>>>>>>> needs
>>>>>>>>>>>>> to
>>>>>>>>>>>>> be run in UEFI context while efi_timer_check() as well as
>>>>>>>>>>>>> console
>>>>>>>>>>>>> print()
>>>>>>>>>>>>> need to be run in U-Boot context.
>>>>>>>>>>>>>
>>>>>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise
>>>>>>>>>>>>> you
>>>>>>>>>>>>> that
>>>>>>>>>>>>> 2
>>>>>>>>>>>>> weeks down some corner case nobody thought of falls apart
>>>>>>>>>>>>> because
>>>>>>>>>>>>> people
>>>>>>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>>>>>>
>>>>>>>>>>>> how many places do we have callbacks into UEFI context like this?
>>>>>>>>>>>> If
>>>>>>>>>>>> just one or two maybe we can suppress them while in u-boot
>>>>>>>>>>>> context and
>>>>>>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>>>>>>> synchronously. And yes, we can probably hand massage the refcount
>>>>>>>>>>> on the
>>>>>>>>>>> ones that we remember, but I'm just afraid that the whole thing
>>>>>>>>>>> will
>>>>>>>>>>> be so
>>>>>>>>>>> complicated eventually that nobody understands what's going on.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying
>>>>>>>>>> the
>>>>>>>>>> callback until EFI_EXIT() that transitions back to the UEFI world.
>>>>>>>>>> So
>>>>>>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>>>>>>
>>>>>>>>>> I haven't looked at the efi_event stuff until just now, but from a
>>>>>>>>>> 30sec look, maybe efi_signal_event() could just put the event on a
>>>>>>>>>> list of signalled events and not immediately call
>>>>>>>>>> event->notify_function(), and handle the calling of
>>>>>>>>>> notify_function()
>>>>>>>>>> in the last EFI_EXIT()??
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Maybe, I'll leave Heinrich to comment on that.
>>>>>>>>
>>>>>>>>
>>>>>>>> Let the application call WaitForEvent for a keyboard event.
>>>>>>>> Rob suggests that neither timer events nor any other events are
>>>>>>>> served
>>>>>>>> until the user possibly presses a key.
>>>>>>>>
>>>>>>>> No, we have to call notification functions immediately to serve time
>>>>>>>> critical communication like network traffic.
>>>>>>>>
>>>>>>>
>>>>>>> Does WaitForEvent or anything else that dispatches events dispatch
>>>>>>> more than one event, or is it just one event and then return to UEFI
>>>>>>> world to poll again?  We aren't really multi-threaded so there can't
>>>>>>> really be async callbacks.
>>>>>>>
>>>>>>> Either way, I think we can make this work by changing
>>>>>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>>>>>>> non-nested EFI_ENTER() case we don't have to do anything special (and
>>>>>>> the nested case probably should never happen in places where we need
>>>>>>> EFI_CALLBACK())
>>>>>>
>>>>>>
>>>>>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>>>>>> timer events this includes a notify_function checking the keyboard and
>>>>>> in future I want to check the watchdog in a notify_function.
>>>>>>
>>>>>> Please, consider that in the notify functions any number of UEFI API
>>>>>> functions will be called by the EFI application which themselves may
>>>>>> signal events which in turn call notify_functions in which again UEFI
>>>>>> API functions may be called.
>>>>>
>>>>>
>>>>> sure, one way or another I don't want to call a notifier from nested
>>>>> EFI_ENTER()'s..
>>>>
>>>>
>>>> We call EFI_EXIT before calling the notifier function and
>>>> call EFI_ENTER after returning form the notifier function.
>>>>
>>>> So currently we always switch gd correctly. An nested EFI_ENTER simply
>>>> does not occur.
>>>
>>>
>>> yes, I understand how it currently works.. although I think an
>>> EFI_CALLBACK() macro that did the equiv (with extra error checking if
>>> we allow re-entrant EFI_ENTER()) would be clearer.  It would make the
>>> callbacks to UEFI world easy to spot and make the code a bit more
>>> concise.
>>>
>>>> I still do not understand why you need to change anything here.
>>>>
>>>>>
>>>>> so either EFI_CALLBACK() should detect the condition and yell loudly,
>>>>> or it should detect it and queue up callbacks until we are not
>>>>> nested..
>>>>
>>>>
>>>> Your suggestions are getting more and more complicated.
>>>>
>>>> I would always go for easily maintainable code and not for
>>>> sophistication. Please, stick to the KISS principle.
>>>>
>>>> If you want to call functions internally as you described in the first
>>>> mail in this thread, do not call the UEFI functions but supply separate
>>>> entry points.
>>>
>>>
>>> The issue that I am trying to address at the moment is that
>>> efi_load_image(), to load an image specified by file_path (as opposed
>>> to already loaded into source_buffer, ie. the source_buffer==NULL
>>> case, which UEFI spec allows) requires using a handful of APIs from
>>> the EFI file interface, and also requires the
>>> simple-file-system-protocol, to actually read the image specified by
>>> file_path into a buffer.
>>>
>>> We could go down the path of continuing to add _ext functions.. but it
>>> is a bit ugly, and seems like it would keep growing worse.  And it
>>> requires exposing functions from different modules which could
>>> otherwise remain static.  All for a case that won't happen (for these
>>> particular APIs at least) or at least is easy to detect.  Keeping the
>>> _ext functions for only the special cases that could lead to a
>>> callback into UEFI world, and avoiding them in the normal case, seems
>>> cleaner.  It isn't terribly complicated to implement, and it is easy
>>> to detect programming errors.
>>
>>
>> Maybe we could turn all of this around and you just call EFI_EXIT() before
>> calling all the callbacks? That way you're back in UEFI context and are free
>> to call anything that doesn't need gd.
>>
>> So something like
>>
>>    EFI_EXIT();
>>    file_proto *f = bs->find_protocol();
>>    file_foo *g = f->bar();
>>    if (!g) return EFI_AMAZING_ERROR;
>>    ...
>>    EFI_ENTER();
>>
>> That way the fs loading code that really only deals with UEFI calls would be
>> self-contained. We do something similar already with efi_signal_event()
>> today.
> 
> I guess that would mean no malloc/printf/etc.. seems like just
> *asking* for someone to screw up ;-)

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] [PATCH] efi_loader: allow efi_loader code to call EFI interfaces

[Rob Clark]

On Tue, Jul 25, 2017 at 6:10 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 26.07.17 00:01, Rob Clark wrote:
>>
>> On Tue, Jul 25, 2017 at 5:42 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 25.07.17 23:26, Rob Clark wrote:
>>>>
>>>>
>>>> On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt
>>>> <xypron.glpk@gmx.de>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 07/25/2017 09:42 PM, Rob Clark wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt
>>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 07/25/2017 07:52 PM, Rob Clark wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt
>>>>>>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 25.07.17 14:28, Rob Clark wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 25.07.17 14:08, Rob Clark wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf
>>>>>>>>>>>>>>> <agraf@suse.de>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> To implement efi_load_image() for the case of loading an
>>>>>>>>>>>>>>>>> image
>>>>>>>>>>>>>>>>> from a
>>>>>>>>>>>>>>>>> device path rather than image already loaded into
>>>>>>>>>>>>>>>>> source_buffer,
>>>>>>>>>>>>>>>>> it is
>>>>>>>>>>>>>>>>> convenient to be able to re-use simple-file-system and
>>>>>>>>>>>>>>>>> efi-file
>>>>>>>>>>>>>>>>> interfaces.  But these are already using
>>>>>>>>>>>>>>>>> EFI_ENTER()/EFI_EXIT().
>>>>>>>>>>>>>>>>> Allow
>>>>>>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this
>>>>>>>>>>>>>>>>> greatly
>>>>>>>>>>>>>>>>> simplifies
>>>>>>>>>>>>>>>>> things.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow
>>>>>>>>>>>>>>>>> grub
>>>>>>>>>>>>>>>>> to call
>>>>>>>>>>>>>>>>> into interfaces installed by something outside of grub.)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So there are 2 ways to do this:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>        1) Keep a refcount, only transition when passing the
>>>>>>>>>>>>>>>> 0
>>>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>>>        2) Explicitly split functions with ENTRY/EXIT from
>>>>>>>>>>>>>>>> their
>>>>>>>>>>>>>>>> core
>>>>>>>>>>>>>>>> functions
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So far we used approach 2, splitting functions that are used
>>>>>>>>>>>>>>>> by both
>>>>>>>>>>>>>>>> internal and external code into _ext (for externally called)
>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>> normal
>>>>>>>>>>>>>>>> functions. You can see this pattern quite a few times
>>>>>>>>>>>>>>>> throughout
>>>>>>>>>>>>>>>> efi_loader.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I definitely did try the refcount approach back when I
>>>>>>>>>>>>>>>> decided
>>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>>> approach
>>>>>>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember
>>>>>>>>>>>>>>>> where.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Either way, we should definitely be consistent. Either we
>>>>>>>>>>>>>>>> always use
>>>>>>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can
>>>>>>>>>>>>>>>> make
>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>> version
>>>>>>>>>>>>>>>> work where all _ext variants disappear and we're magically
>>>>>>>>>>>>>>>> reentrant,
>>>>>>>>>>>>>>>> I'll
>>>>>>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere
>>>>>>>>>>>>>>>> though :).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of
>>>>>>>>>>>>>>> functions
>>>>>>>>>>>>>>> normally called via EFI.. so it is going to be a lot more
>>>>>>>>>>>>>>> _ext
>>>>>>>>>>>>>>> variants.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yes, but imagine a case like this:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> open_protocol
>>>>>>>>>>>>>>       -> open()
>>>>>>>>>>>>>>         -> random_junk()
>>>>>>>>>>>>>>           -> efi_timer_check()
>>>>>>>>>>>>>>             -> timer notifier
>>>>>>>>>>>>>>               -> console print()
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Here the refcounting will simply fail on us, as the timer
>>>>>>>>>>>>>> notifier
>>>>>>>>>>>>>> needs
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> be run in UEFI context while efi_timer_check() as well as
>>>>>>>>>>>>>> console
>>>>>>>>>>>>>> print()
>>>>>>>>>>>>>> need to be run in U-Boot context.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can
>>>>>>>>>>>>>> promise
>>>>>>>>>>>>>> you
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>> 2
>>>>>>>>>>>>>> weeks down some corner case nobody thought of falls apart
>>>>>>>>>>>>>> because
>>>>>>>>>>>>>> people
>>>>>>>>>>>>>> don't manage to fully grasp the code flow anymore.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ugg.. gd is a gd pita ;-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> how many places do we have callbacks into UEFI context like
>>>>>>>>>>>>> this?
>>>>>>>>>>>>> If
>>>>>>>>>>>>> just one or two maybe we can suppress them while in u-boot
>>>>>>>>>>>>> context and
>>>>>>>>>>>>> handle in efi_exit_func() when entry_count drops to zero?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> What do you mean by suppressing? We need to call those helpers
>>>>>>>>>>>> synchronously. And yes, we can probably hand massage the
>>>>>>>>>>>> refcount
>>>>>>>>>>>> on the
>>>>>>>>>>>> ones that we remember, but I'm just afraid that the whole thing
>>>>>>>>>>>> will
>>>>>>>>>>>> be so
>>>>>>>>>>>> complicated eventually that nobody understands what's going on.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying
>>>>>>>>>>> the
>>>>>>>>>>> callback until EFI_EXIT() that transitions back to the UEFI
>>>>>>>>>>> world.
>>>>>>>>>>> So
>>>>>>>>>>> from the PoV of the UEFI world, it is still synchronous.
>>>>>>>>>>>
>>>>>>>>>>> I haven't looked at the efi_event stuff until just now, but from
>>>>>>>>>>> a
>>>>>>>>>>> 30sec look, maybe efi_signal_event() could just put the event on
>>>>>>>>>>> a
>>>>>>>>>>> list of signalled events and not immediately call
>>>>>>>>>>> event->notify_function(), and handle the calling of
>>>>>>>>>>> notify_function()
>>>>>>>>>>> in the last EFI_EXIT()??
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Maybe, I'll leave Heinrich to comment on that.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Let the application call WaitForEvent for a keyboard event.
>>>>>>>>> Rob suggests that neither timer events nor any other events are
>>>>>>>>> served
>>>>>>>>> until the user possibly presses a key.
>>>>>>>>>
>>>>>>>>> No, we have to call notification functions immediately to serve
>>>>>>>>> time
>>>>>>>>> critical communication like network traffic.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Does WaitForEvent or anything else that dispatches events dispatch
>>>>>>>> more than one event, or is it just one event and then return to UEFI
>>>>>>>> world to poll again?  We aren't really multi-threaded so there can't
>>>>>>>> really be async callbacks.
>>>>>>>>
>>>>>>>> Either way, I think we can make this work by changing
>>>>>>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the
>>>>>>>> non-nested EFI_ENTER() case we don't have to do anything special
>>>>>>>> (and
>>>>>>>> the nested case probably should never happen in places where we need
>>>>>>>> EFI_CALLBACK())
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> WaitForEvent and CheckEvent calls will serve all notify_functions of
>>>>>>> timer events this includes a notify_function checking the keyboard
>>>>>>> and
>>>>>>> in future I want to check the watchdog in a notify_function.
>>>>>>>
>>>>>>> Please, consider that in the notify functions any number of UEFI API
>>>>>>> functions will be called by the EFI application which themselves may
>>>>>>> signal events which in turn call notify_functions in which again UEFI
>>>>>>> API functions may be called.
>>>>>>
>>>>>>
>>>>>>
>>>>>> sure, one way or another I don't want to call a notifier from nested
>>>>>> EFI_ENTER()'s..
>>>>>
>>>>>
>>>>>
>>>>> We call EFI_EXIT before calling the notifier function and
>>>>> call EFI_ENTER after returning form the notifier function.
>>>>>
>>>>> So currently we always switch gd correctly. An nested EFI_ENTER simply
>>>>> does not occur.
>>>>
>>>>
>>>>
>>>> yes, I understand how it currently works.. although I think an
>>>> EFI_CALLBACK() macro that did the equiv (with extra error checking if
>>>> we allow re-entrant EFI_ENTER()) would be clearer.  It would make the
>>>> callbacks to UEFI world easy to spot and make the code a bit more
>>>> concise.
>>>>
>>>>> I still do not understand why you need to change anything here.
>>>>>
>>>>>>
>>>>>> so either EFI_CALLBACK() should detect the condition and yell loudly,
>>>>>> or it should detect it and queue up callbacks until we are not
>>>>>> nested..
>>>>>
>>>>>
>>>>>
>>>>> Your suggestions are getting more and more complicated.
>>>>>
>>>>> I would always go for easily maintainable code and not for
>>>>> sophistication. Please, stick to the KISS principle.
>>>>>
>>>>> If you want to call functions internally as you described in the first
>>>>> mail in this thread, do not call the UEFI functions but supply separate
>>>>> entry points.
>>>>
>>>>
>>>>
>>>> The issue that I am trying to address at the moment is that
>>>> efi_load_image(), to load an image specified by file_path (as opposed
>>>> to already loaded into source_buffer, ie. the source_buffer==NULL
>>>> case, which UEFI spec allows) requires using a handful of APIs from
>>>> the EFI file interface, and also requires the
>>>> simple-file-system-protocol, to actually read the image specified by
>>>> file_path into a buffer.
>>>>
>>>> We could go down the path of continuing to add _ext functions.. but it
>>>> is a bit ugly, and seems like it would keep growing worse.  And it
>>>> requires exposing functions from different modules which could
>>>> otherwise remain static.  All for a case that won't happen (for these
>>>> particular APIs at least) or at least is easy to detect.  Keeping the
>>>> _ext functions for only the special cases that could lead to a
>>>> callback into UEFI world, and avoiding them in the normal case, seems
>>>> cleaner.  It isn't terribly complicated to implement, and it is easy
>>>> to detect programming errors.
>>>
>>>
>>>
>>> Maybe we could turn all of this around and you just call EFI_EXIT()
>>> before
>>> calling all the callbacks? That way you're back in UEFI context and are
>>> free
>>> to call anything that doesn't need gd.
>>>
>>> So something like
>>>
>>>    EFI_EXIT();
>>>    file_proto *f = bs->find_protocol();
>>>    file_foo *g = f->bar();
>>>    if (!g) return EFI_AMAZING_ERROR;
>>>    ...
>>>    EFI_ENTER();
>>>
>>> That way the fs loading code that really only deals with UEFI calls would
>>> be
>>> self-contained. We do something similar already with efi_signal_event()
>>> today.
>>
>>
>> I guess that would mean no malloc/printf/etc.. seems like just
>> *asking* for someone to screw up ;-)
>
>
> Well, you have to choose a world to be in. You still have the EFI alloc and
> print methods ;).

sure, ofc.. but as long as we have to live with gd it is a good thing
if the associated macros make it hard to inadvertently 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.
>
>
> 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 :).
>

sorta, but not really.. but I'm a big fan of when these sort of things
are designed with debugging checks in mind.  That is something that
the linux kernel does a really good job of (think of the various
lockdep, list debug, etc, features).

There is nothing worse when tearing out your hair debugging some
problem, when adding an innocent looking printf() in the wrong place
makes things blow up in a different and even worse way ;-)

anyways, I'm getting from u-boot -> shim -> fallback -> shim -> grub now :-)

I'll re-work this patch and resend tomorrow.. if any better
suggestions than UEFI() let me know.. maybe we should call it IFE()
since the EFI* macros kinda do the opposite of what they are named :-P

BR,
-R