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

[Simon Glass]

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.

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