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

[Alexander Graf]

On 01/31/2016 04:19 PM, Simon Glass wrote:
> Hi Alexander,
>
> On 14 January 2016 at 22:06, Alexander Graf <agraf at 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 at 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 at 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.

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


Alex