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

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

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


Alex