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

Simon Glass
2016-01-31T15:19:45Z

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.

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

> +};
> +
> +struct efi_object {
> + struct list_head link;
> + struct efi_handler protocols[4];
> + void *handle;
> +};

Can you add comments on the above structs?

> +extern struct list_head efi_obj_list;
> +
> efi_status_t efi_return_handle(void *handle,
> efi_guid_t *protocol, void **protocol_interface,
> void *agent_handle, void *controller_handle,
> uint32_t attributes);
> +void efi_timer_check(void);
> void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info);
> +void efi_save_gd(void);
> +void efi_restore_gd(void);
> +efi_status_t efi_exit_func(efi_status_t ret);
>
> #define EFI_LOADER_POOL_SIZE (128 * 1024 * 1024)
> void *efi_loader_alloc(uint64_t len);
> +
> +#else /* defined(EFI_LOADER) */
> +
> +static inline void efi_restore_gd(void) { }

And the above functions

[snip]

Regards,
Simon