[U-Boot] [PATCH 04/14] efi_loader: Add boot time services

[Alexander Graf <agraf@suse.de>]

On 02/02/2016 12:54 AM, Simon Glass wrote:
> Hi Alex,
>
> On 1 February 2016 at 16:45, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 01/31/2016 04:19 PM, Simon Glass wrote:
>>> Hi Alexander,
>>>
>>> On 14 January 2016 at 22:06, Alexander Graf <agraf@suse.de> wrote:
>>>> When an EFI application runs, it has access to a few descriptor and callback
>>>> tables to instruct the EFI compliant firmware to do things for it. The bulk
>>>> of those interfaces are "boot time services". They handle all object management,
>>>> and memory allocation.
>>>>
>>>> This patch adds support for the boot time services and also exposes a system
>>>> table, which is the point of entry descriptor table for EFI payloads.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>
>>>>     - Fix typo s/does now/does not/
>>>>     - Add #ifdefs around header to allow inclusion when efi_loader is disabled
>>>>     - Add stub efi_restore_gd() function when efi_loader is disabled
>>>>     - Disable debug
>>>>     - Mark runtime region as such
>>>>     - Fix up memory map
>>>>     - Allow efi_restore_gd to be called before first efi entry
>>>>     - Add 32bit arm cache workaround
>>>>     - Move memory map to separate patch
>>>>     - Change BTS version to 2.5
>>>>     - Fix return values for a few callbacks to more EFI compliant ones
>>>>     - Change vendor to "Das U-Boot"
>>>>     - Add warning when truncating timer trigger
>>>>     - Move to GPLv2+
>>>> ---
>>>>    include/efi_loader.h          |  51 +++
>>>>    lib/efi_loader/efi_boottime.c | 761 ++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 812 insertions(+)
>>>>    create mode 100644 lib/efi_loader/efi_boottime.c
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> But please see below.
>>>
>>> Also, how does the timer get disabled? For me it seems to be enabled
>>> on boot so it always calls the trigger function.
>>
>> I think I see what you're running into. The ARM code runs set_timer as one of its first actions. On x86 it probably doesn't, so that's why you see the timer firing off, since it's initialized to 0 and now >= 0 always.
>>
>> I'll change the initialization for timer_next to -1ULL. That way you should only get to see events when they're there.
> OK. Or maybe you need a bool to track when it is not active?

Setting the value to -1ULL is almost the same, as it would only ever 
match if timer_get_us() returns exactly 0xffffffffffffffff.

>
>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index bf77573..391459e 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -6,18 +6,69 @@
>>>>     *  SPDX-License-Identifier:     GPL-2.0+
>>>>     */
>>>>
>>>> +#include <common.h>
>>>>    #include <part_efi.h>
>>>>    #include <efi_api.h>
>>>> +
>>>> +#ifdef CONFIG_EFI_LOADER
>>>> +
>>>>    #include <linux/list.h>
>>>>
>>>> +/* #define DEBUG_EFI */
>>>> +
>>>> +#ifdef DEBUG_EFI
>>>> +#define EFI_ENTRY(format, ...) do { \
>>>> +       efi_restore_gd(); \
>>>> +       printf("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
>>>> +       } while(0)
>>>> +#else
>>>> +#define EFI_ENTRY(format, ...) do { \
>>>> +       efi_restore_gd(); \
>>>> +       } while(0)
>>>> +#endif
>>>> +
>>>> +#define EFI_EXIT(ret) efi_exit_func(ret);
>>>> +
>>>> +extern struct efi_system_table systab;
>>>> +
>>>>    extern const efi_guid_t efi_guid_device_path;
>>>>    extern const efi_guid_t efi_guid_loaded_image;
>>>>
>>>> +struct efi_class_map {
>>>> +       const efi_guid_t *guid;
>>>> +       const void *interface;
>>>> +};
>>>> +
>>>> +struct efi_handler {
>>>> +       const efi_guid_t *guid;
>>>> +       efi_status_t (EFIAPI *open)(void *handle,
>>>> +                       efi_guid_t *protocol, void **protocol_interface,
>>>> +                       void *agent_handle, void *controller_handle,
>>>> +                       uint32_t attributes);
>>> Can we add a name in here? It would be good to name each of these
>>> things so that we can print it out for debugging, etc.
>>>
>>> Same also for efi_object. The guids are an unfortunate but effective
>>> obfuscation for people trying to figure out what is going on. I think
>>> we should try to keep things human-friendly.
>>
>> While I agree, I'm not sure this is the correct layer to do this in. As soon as the open callback gets called, you should be able to see which function this is, no?
>>
>> Do you have any concrete case where the current debugging wasn't enough?
> In my case I was trying to work out whether the call was getting
> through, and where it was ending up. It was really hard to know
> whether something was just not getting through. Putting a name in the
> dispatching helped a lot with that. It's all very well to send a
> message when you have arrived, but it's more useful I think to send a
> message when you leave :-)

I can see why it would help for that specific case, but I'd really put 
this into the "apply locally, debug, throw away" patch category. I'm 
sure once we have the support going well on x86, there won't be any need 
to dig out names from the dispatcher.


Alex