[U-Boot] [PATCH 18/48] efi: Add start-up library code

Simon Glass sjg at chromium.org
Fri Jul 31 17:45:24 CEST 2015


Hi Bin,

On 23 July 2015 at 02:09, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass <sjg at chromium.org> wrote:
>> When running as an EFI application, U-Boot must request memory from EFI,
>> and provide access to the boot services U-Boot needs.
>>
>> Add library code to perform these tasks. This includes efi_main() which is
>> the entry point from EFI. U-Boot is built as a shared library.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>>  arch/x86/include/asm/fsp/fsp_hob.h |  59 +-----
>>  include/efi.h                      | 356 +++++++++++++++++++++++++++++++++++++
>>  include/efi_api.h                  | 252 ++++++++++++++++++++++++++
>>  include/part_efi.h                 |   9 +-
>>  lib/Kconfig                        |   2 +
>>  lib/Makefile                       |   1 +
>>  lib/efi/Kconfig                    |  33 ++++
>>  lib/efi/Makefile                   |   7 +
>>  lib/efi/efi.c                      |  91 ++++++++++
>>  lib/efi/efi_app.c                  | 125 +++++++++++++
>>  10 files changed, 871 insertions(+), 64 deletions(-)
>>  create mode 100644 include/efi.h
>>  create mode 100644 include/efi_api.h
>>  create mode 100644 lib/efi/Kconfig
>>  create mode 100644 lib/efi/Makefile
>>  create mode 100644 lib/efi/efi.c
>>  create mode 100644 lib/efi/efi_app.c
>>
>> diff --git a/arch/x86/include/asm/fsp/fsp_hob.h b/arch/x86/include/asm/fsp/fsp_hob.h
>> index 6cca7f5..3fb3546 100644
>> --- a/arch/x86/include/asm/fsp/fsp_hob.h
>> +++ b/arch/x86/include/asm/fsp/fsp_hob.h
>> @@ -8,6 +8,8 @@
>>  #ifndef __FSP_HOB_H__
>>  #define __FSP_HOB_H__
>>
>> +#include <efi.h>
>> +
>>  /* Type of HOB Header */
>>  #define HOB_TYPE_MEM_ALLOC     0x0002
>>  #define HOB_TYPE_RES_DESC      0x0003
>> @@ -25,63 +27,6 @@ struct hob_header {
>>         u32     reserved;       /* always zero */
>>  };
>>
>> -/* Enumeration of memory types introduced in UEFI */
>> -enum efi_mem_type {
>> -       EFI_RESERVED_MEMORY_TYPE,
>> -       /*
>> -        * The code portions of a loaded application.
>> -        * (Note that UEFI OS loaders are UEFI applications.)
>> -        */
>> -       EFI_LOADER_CODE,
>> -       /*
>> -        * The data portions of a loaded application and
>> -        * the default data allocation type used by an application
>> -        * to allocate pool memory.
>> -        */
>> -       EFI_LOADER_DATA,
>> -       /* The code portions of a loaded Boot Services Driver */
>> -       EFI_BOOT_SERVICES_CODE,
>> -       /*
>> -        * The data portions of a loaded Boot Serves Driver and
>> -        * the default data allocation type used by a Boot Services
>> -        * Driver to allocate pool memory.
>> -        */
>> -       EFI_BOOT_SERVICES_DATA,
>> -       /* The code portions of a loaded Runtime Services Driver */
>> -       EFI_RUNTIME_SERVICES_CODE,
>> -       /*
>> -        * The data portions of a loaded Runtime Services Driver and
>> -        * the default data allocation type used by a Runtime Services
>> -        * Driver to allocate pool memory.
>> -        */
>> -       EFI_RUNTIME_SERVICES_DATA,
>> -       /* Free (unallocated) memory */
>> -       EFI_CONVENTIONAL_MEMORY,
>> -       /* Memory in which errors have been detected */
>> -       EFI_UNUSABLE_MEMORY,
>> -       /* Memory that holds the ACPI tables */
>> -       EFI_ACPI_RECLAIM_MEMORY,
>> -       /* Address space reserved for use by the firmware */
>> -       EFI_ACPI_MEMORY_NVS,
>> -       /*
>> -        * Used by system firmware to request that a memory-mapped IO region
>> -        * be mapped by the OS to a virtual address so it can be accessed by
>> -        * EFI runtime services.
>> -        */
>> -       EFI_MMAP_IO,
>> -       /*
>> -        * System memory-mapped IO region that is used to translate
>> -        * memory cycles to IO cycles by the processor.
>> -        */
>> -       EFI_MMAP_IO_PORT,
>> -       /*
>> -        * Address space reserved by the firmware for code that is
>> -        * part of the processor.
>> -        */
>> -       EFI_PAL_CODE,
>> -       EFI_MAX_MEMORY_TYPE
>> -};
>> -
>>  /*
>>   * Describes all memory ranges used during the HOB producer phase that
>>   * exist outside the HOB list. This HOB type describes how memory is used,
>> diff --git a/include/efi.h b/include/efi.h
>> new file mode 100644
>> index 0000000..66ef6c3
>> --- /dev/null
>> +++ b/include/efi.h
>> @@ -0,0 +1,356 @@
>> +/*
>> + * Extensible Firmware Interface
>> + * Based on 'Extensible Firmware Interface Specification' version 0.9,
>> + * April 30, 1999
>> + *
>> + * Copyright (C) 1999 VA Linux Systems
>> + * Copyright (C) 1999 Walt Drummond <drummond at valinux.com>
>> + * Copyright (C) 1999, 2002-2003 Hewlett-Packard Co.
>> + *     David Mosberger-Tang <davidm at hpl.hp.com>
>> + *     Stephane Eranian <eranian at hpl.hp.com>
>> + *
>> + * From include/linux/efi.h in kernel 4.1 with some additions/subtractions
>> + */
>> +
>> +#ifndef _EFI_H
>> +#define _EFI_H
>> +
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +#ifdef CONFIG_EFI_STUB_64BIT
>
> I believe this CONFIG_EFI_STUB_64BIT is introduced in later patches,
> can we remove this to later patch where you add this
> CONFIG_EFI_STUB_64BIT?
>
>> +#define EFIAPI __attribute__((ms_abi))
>
> This is worth a comment block to describe the fact that EFI 64-bit is
> using M$ ABI which is different from ours.
>
>> +#else
>> +#define EFIAPI
>> +#endif
>> +
>> +struct efi_device_path;
>> +
>> +#define EFI_SUCCESS            0
>> +#define EFI_LOAD_ERROR         (1 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_INVALID_PARAMETER  (2 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_UNSUPPORTED                (3 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_BAD_BUFFER_SIZE    (4 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_BUFFER_TOO_SMALL   (5 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_NOT_READY          (6 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_DEVICE_ERROR       (7 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_WRITE_PROTECTED    (8 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_OUT_OF_RESOURCES   (9 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_NOT_FOUND          (14 | (1UL << (BITS_PER_LONG - 1)))
>> +#define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG - 1)))
>> +
>
> Can we also remove those duplicated defines in
> arch/x86/include/asm/fsp/fsp_types.h so that FSP codes can use the one
> in efi.h?

But these have an FSP prefix, right? If you are sure about this, I'll
do it as a separate clean-up patch.

>
>> +typedef unsigned long efi_status_t;
>> +typedef u64 efi_physical_addr_t;
>> +typedef u64 efi_virtual_addr_t;
>> +typedef void *efi_handle_t;
>> +
>> +#define EFI_GUID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
>> +       ((efi_guid_t) \
>> +       {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, \
>> +               ((a) >> 24) & 0xff, \
>> +               (b) & 0xff, ((b) >> 8) & 0xff, \
>> +               (c) & 0xff, ((c) >> 8) & 0xff, \
>> +               (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
>> +
>> +/*
>> + * Generic EFI table header
>> + */
>
> Please use one-line comment.
>
>> +struct efi_table_hdr {
>> +       u64 signature;
>> +       u32 revision;
>> +       u32 headersize;
>> +       u32 crc32;
>> +       u32 reserved;
>> +};
>> +
>> +/* Enumeration of memory types introduced in UEFI */
>> +enum efi_mem_type {
>> +       EFI_RESERVED_MEMORY_TYPE,
>> +       /*
>> +        * The code portions of a loaded application.
>> +        * (Note that UEFI OS loaders are UEFI applications.)
>> +        */
>> +       EFI_LOADER_CODE,
>> +       /*
>> +        * The data portions of a loaded application and
>> +        * the default data allocation type used by an application
>> +        * to allocate pool memory.
>> +        */
>> +       EFI_LOADER_DATA,
>> +       /* The code portions of a loaded Boot Services Driver */
>> +       EFI_BOOT_SERVICES_CODE,
>> +       /*
>> +        * The data portions of a loaded Boot Serves Driver and
>> +        * the default data allocation type used by a Boot Services
>> +        * Driver to allocate pool memory.
>> +        */
>> +       EFI_BOOT_SERVICES_DATA,
>> +       /* The code portions of a loaded Runtime Services Driver */
>> +       EFI_RUNTIME_SERVICES_CODE,
>> +       /*
>> +        * The data portions of a loaded Runtime Services Driver and
>> +        * the default data allocation type used by a Runtime Services
>> +        * Driver to allocate pool memory.
>> +        */
>> +       EFI_RUNTIME_SERVICES_DATA,
>> +       /* Free (unallocated) memory */
>> +       EFI_CONVENTIONAL_MEMORY,
>> +       /* Memory in which errors have been detected */
>> +       EFI_UNUSABLE_MEMORY,
>> +       /* Memory that holds the ACPI tables */
>> +       EFI_ACPI_RECLAIM_MEMORY,
>> +       /* Address space reserved for use by the firmware */
>> +       EFI_ACPI_MEMORY_NVS,
>> +       /*
>> +        * Used by system firmware to request that a memory-mapped IO region
>> +        * be mapped by the OS to a virtual address so it can be accessed by
>> +        * EFI runtime services.
>> +        */
>> +       EFI_MMAP_IO,
>> +       /*
>> +        * System memory-mapped IO region that is used to translate
>> +        * memory cycles to IO cycles by the processor.
>> +        */
>> +       EFI_MMAP_IO_PORT,
>> +       /*
>> +        * Address space reserved by the firmware for code that is
>> +        * part of the processor.
>> +        */
>> +       EFI_PAL_CODE,
>> +
>> +       EFI_MAX_MEMORY_TYPE,
>> +       EFI_TABLE_END,  /* For efi_build_mem_table() */
>> +};
>> +
>> +/* Attribute values */
>> +enum {
>> +       EFI_MEMORY_UC_SHIFT     = 0,    /* uncached */
>> +       EFI_MEMORY_WC_SHIFT     = 1,    /* write-coalescing */
>> +       EFI_MEMORY_WT_SHIFT     = 2,    /* write-through */
>> +       EFI_MEMORY_WB_SHIFT     = 3,    /* write-back */
>> +       EFI_MEMORY_UCE_SHIFT    = 4,    /* uncached, exported */
>> +       EFI_MEMORY_WP_SHIFT     = 12,   /* write-protect */
>> +       EFI_MEMORY_RP_SHIFT     = 13,   /* read-protect */
>> +       EFI_MEMORY_XP_SHIFT     = 14,   /* execute-protect */
>> +       EFI_MEMORY_RUNTIME_SHIFT = 63,  /* range requires runtime mapping */
>> +
>> +       EFI_MEMORY_RUNTIME = 1ULL << EFI_MEMORY_RUNTIME_SHIFT,
>> +       efi_mem_desc_VERSION    = 1,
>
> What is efi_mem_desc_VERSION? The name should be all capital letters.
>
>> +};
>> +
>> +#define EFI_PAGE_SHIFT         12
>> +#define EFI_PAGE_SIZE          (1UL << EFI_PAGE_SHIFT)
>> +
>> +struct efi_mem_desc {
>> +       u32 type;
>> +       u32 reserved;
>> +       efi_physical_addr_t physical_start;
>> +       efi_virtual_addr_t virtual_start;
>> +       u64 num_pages;
>> +       u64 attribute;
>> +};
>> +
>> +/*
>> + * Allocation types for calls to boottime->allocate_pages.
>> + */
>
> Please use one-line comment.
>
>> +#define EFI_ALLOCATE_ANY_PAGES         0
>> +#define EFI_ALLOCATE_MAX_ADDRESS       1
>> +#define EFI_ALLOCATE_ADDRESS           2
>> +#define EFI_MAX_ALLOCATE_TYPE          3
>> +
>> +/*
>> + * Types and defines for Time Services
>> + */
>
> Please use one-line comment.
>
>> +#define EFI_TIME_ADJUST_DAYLIGHT 0x1
>> +#define EFI_TIME_IN_DAYLIGHT     0x2
>> +#define EFI_UNSPECIFIED_TIMEZONE 0x07ff
>> +
>> +struct efi_time {
>> +       u16 year;
>> +       u8 month;
>> +       u8 day;
>> +       u8 hour;
>> +       u8 minute;
>> +       u8 second;
>> +       u8 pad1;
>> +       u32 nanosecond;
>> +       s16 timezone;
>> +       u8 daylight;
>> +       u8 pad2;
>> +};
>> +
>> +struct efi_time_cap {
>> +       u32 resolution;
>> +       u32 accuracy;
>> +       u8 sets_to_zero;
>> +};
>> +
>> +enum efi_locate_search_type {
>> +       all_handles,
>> +       by_register_notify,
>> +       by_protocol
>> +};
>> +
>> +struct efi_open_protocol_info_entry {
>> +       efi_handle_t agent_handle;
>> +       efi_handle_t controller_handle;
>> +       u32 attributes;
>> +       u32 open_count;
>> +};
>> +
>> +enum efi_entry_t {
>> +       EFIET_END,      /* Signals this is the last (empty) entry */
>> +       EFIET_MEMORY_MAP,
>> +
>
> Please remove this blank line.
>
>> +       EFIET_MEMORY_COUNT,
>> +};
>> +
>> +#define EFI_TABLE_VERSION      1
>> +
>> +/**
>> + * struct efi_info_hdr - Header for the EFI info table
>> + *
>> + * @version:   EFI_TABLE_VERSION
>> + * @hdr_size:  Size of this struct in bytes
>
> Missing description for total_size and spare[5]
>
>> + */
>> +struct efi_info_hdr {
>> +       u32 version;
>> +       u32 hdr_size;
>> +       u32 total_size;
>> +       u32 spare[5];
>> +};
>> +
>> +/**
>> + * struct efi_entry_hdr - Header for a table entry
>> + *
>> + * @type:      enum eft_entry_t
>> + * @size       size of entry bytes excluding header and padding
>> + * @addr:      address of this entry (0 if it follows the header )
>> + * @link:      size of entry including header and padding
>> + * @
>
> Missing description for spare1 and spare2.
>
>> + */
>> +struct efi_entry_hdr {
>> +       u32 type;
>> +       u32 size;
>> +       u64 addr;
>> +       u32 link;
>> +       u32 spare1;
>> +       u64 spare22;
>
> Why 22? Should it be spare2?
>
>> +};
>> +
>> +struct efi_entry_memmap {
>
> Can we add comment block for this structure too?
>
>> +       u32 version;
>> +       u32 desc_size;
>> +       u64 spare;
>> +       struct efi_mem_desc desc[];
>
> Nits: should it be "struct efi_mem_desc *desc"?

No, the data follows immediately here.

>
>> +};
>> +
>> +static inline struct efi_mem_desc *efi_get_next_mem_desc(
>> +               struct efi_entry_memmap *map, struct efi_mem_desc *desc)
>> +{
>> +       return (struct efi_mem_desc *)((ulong)desc + map->desc_size);
>> +}
>> +
>> +struct efi_priv {
>> +       efi_handle_t parent_image;
>> +       struct efi_device_path *device_path;
>> +       struct efi_system_table *sys_table;
>> +       struct efi_boot_services *boot;
>> +       struct efi_runtime_services *run;
>> +       bool use_pool_for_malloc;
>> +       unsigned long ram_base;
>> +       unsigned int image_data_type;
>> +       struct efi_info_hdr *info;
>> +       unsigned int info_size;
>> +       void *next_hdr;
>> +};
>> +
>> +/* Base address of the EFI image */
>> +extern char ImageBase[];
>
> Can we avoid CamelCase?
>
>> +
>> +/**
>> + * efi_get_sys_table() - Get access to the main EFI system table
>> + *
>> + * @return pointer to EFI system table
>> + */
>> +struct efi_system_table *efi_get_sys_table(void);
>> +
>> +/**
>> + * efi_get_ram_base() - Find the base of RAM
>> + *
>> + * This is used when U-Boot is built as an EFI application.
>> + *
>> + * @return the base of RAM as know to U-Boot
>
> as 'known'?
>
>> + */
>> +unsigned long efi_get_ram_base(void);
>> +
>> +/**
>> + * efi_init() - Set up ready for use of EFI boot services
>> + *
>> + * @priv:      Pointer to our private EFI structure to fill in
>> + * @banner:    Banner to display when starting
>> + * @image:     The image handle passed to efi_main()
>> + * @sys_table: The EFI system table pointer passed to efi_main()
>> + */
>> +int efi_init(struct efi_priv *priv, const char *banner, efi_handle_t image,
>> +            struct efi_system_table *sys_table);
>> +
>> +/**
>> + * efi_malloc() - Allocate some memory from EFI
>> + *
>> + * @priv:      Pointer to private EFI structure
>> + * @size:      Number of bytes to allocate
>> + * @retp:      Return EFI status resuilt
>
> Typo of 'result'
>
>> + * @return pointer to memory allocated, or NULL on error
>> + */
>> +void *efi_malloc(struct efi_priv *priv, int size, efi_status_t *retp);
>> +
>> +/**
>> + * efi_free() - Free memory allocated from EFI
>> + *
>> + * @priv:      Pointer to private EFI structure
>> + * @ptr:       Pointer to memory to free
>> + */
>> +void efi_free(struct efi_priv *priv, void *ptr);
>> +
>> +/**
>> + * efi_puts() - Write out a string to the EFI console
>> + *
>> + * @priv:      Pointer to private EFI structure
>> + * @str:       String to write (note this is ASCII, not unicode)
>> + */
>> +void efi_puts(struct efi_priv *priv, const char *str);
>> +
>> +/**
>> + * efi_putc() - Write out a character to the EFI console
>> + *
>> + * @priv:      Pointer to private EFI structure
>> + * @ch:                Character to write (note this is ASCII, not unicode)
>
> A character can only be ASCII.
>
>> + */
>> +void efi_putc(struct efi_priv *priv, const char ch);
>> +
>> +/**
>> + * efi_info_get() - get an entry from an EFI table
>> + *
>> + * @type:      Entry type to search for
>> + * @datap:     Returns pointer to entry data
>> + * @sizep:     Returns pointer to entry size
>> + * @return 0 if OK, -ENODATA if there is no table, -ENOENT if there is no entry
>> + * of the requested type, -EPROTONOSUPPORT if the table has the wrong version
>> + */
>> +int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);
>> +
>> +/**
>> + * efi_build_mem_table() - make a sorted copy of the memory table
>> + *
>> + * @map:       Pointer to EFI memory map table
>> + * @size:      Size of table in bytes
>> + * @skip_bs:   True to skip boot-time memory and merge it with conventional
>> + *             memory. This will significantly reduce the number of table
>> + *             entries.
>> + * @return pointer to the new table. It should be freed with free() by the
>> + *        caller
>> + */
>> +void *efi_build_mem_table(struct efi_entry_memmap *map, int size, bool skip_bs);
>> +
>> +#endif /* _LINUX_EFI_H */
>> diff --git a/include/efi_api.h b/include/efi_api.h
>> new file mode 100644
>> index 0000000..d4528f6
>> --- /dev/null
>> +++ b/include/efi_api.h
>> @@ -0,0 +1,252 @@
>> +/*
>> + * Extensible Firmware Interface
>> + * Based on 'Extensible Firmware Interface Specification' version 0.9,
>> + * April 30, 1999
>> + *
>> + * Copyright (C) 1999 VA Linux Systems
>> + * Copyright (C) 1999 Walt Drummond <drummond at valinux.com>
>> + * Copyright (C) 1999, 2002-2003 Hewlett-Packard Co.
>> + *     David Mosberger-Tang <davidm at hpl.hp.com>
>> + *     Stephane Eranian <eranian at hpl.hp.com>
>> + *
>> + * From include/linux/efi.h in kernel 4.1 with some additions/subtractions
>> + */
>> +
>> +#ifndef _EFI_API_H
>> +#define _EFI_API_H
>> +
>> +#include <efi.h>
>> +
>> +/*
>> + * EFI Boot Services table
>> + */
>
> Please use one-line comment.
>
>> +struct efi_boot_services {
>> +       struct efi_table_hdr hdr;
>> +       void *raise_tpl;
>> +       void *restore_tpl;
>> +
>> +       efi_status_t (EFIAPI *allocate_pages)(int, int, unsigned long,
>> +                                             efi_physical_addr_t *);
>> +       efi_status_t (EFIAPI *free_pages)(efi_physical_addr_t, unsigned long);
>> +       efi_status_t (EFIAPI *get_memory_map)(unsigned long *memory_map_size,
>> +                       struct efi_mem_desc *desc, unsigned long *key,
>> +                       unsigned long *desc_size, u32 *desc_version);
>> +       efi_status_t (EFIAPI *allocate_pool)(int, unsigned long, void **);
>> +       efi_status_t (EFIAPI *free_pool)(void *);
>> +
>> +       void *create_event;
>> +       void *set_timer;
>> +       efi_status_t(EFIAPI *wait_for_event)(unsigned long number_of_events,
>> +                                            void *event, unsigned long *index);
>> +       void *signal_event;
>> +       void *close_event;
>> +       void *check_event;
>> +
>> +       void *install_protocol_interface;
>> +       void *reinstall_protocol_interface;
>> +       void *uninstall_protocol_interface;
>> +       efi_status_t (EFIAPI *handle_protocol)(efi_handle_t, efi_guid_t *,
>> +                                              void **);
>> +       void *__reserved;
>
> Can it be just 'reserved'?
>
>> +       void *register_protocol_notify;
>> +       efi_status_t (EFIAPI *locate_handle)(
>> +                       enum efi_locate_search_type search_type,
>> +                       efi_guid_t *protocol, void *search_key,
>> +                       unsigned long *buffer_size, efi_handle_t *buffer);
>> +       efi_status_t (EFIAPI *locate_device_path)(efi_guid_t *protocol,
>> +                       struct efi_device_path **device_path,
>> +                       efi_handle_t *device);
>> +       void *install_configuration_table;
>> +
>> +       efi_status_t (EFIAPI *load_image)(bool boot_policiy,
>> +                       efi_handle_t parent_image,
>> +                       struct efi_device_path *file_path, void *source_buffer,
>> +                       unsigned long source_size, efi_handle_t *image);
>> +       efi_status_t (EFIAPI *start_image)(efi_handle_t handle,
>> +                                          unsigned long *exitdata_size,
>> +                                          s16 **exitdata);
>> +       efi_status_t (EFIAPI *exit)(efi_handle_t handle,
>> +                                   efi_status_t exit_status,
>> +                                   unsigned long exitdata_size, s16 *exitdata);
>> +       void *unload_image;
>> +       efi_status_t (EFIAPI *exit_boot_services)(efi_handle_t, unsigned long);
>> +
>> +       efi_status_t (EFIAPI *get_next_monotonic_count)(u64 *count);
>> +       efi_status_t (EFIAPI *stall)(unsigned long usecs);
>> +       void *set_watchdog_timer;
>> +       efi_status_t(EFIAPI *connect_controller)(efi_handle_t controller_handle,
>> +                       efi_handle_t *driver_image_handle,
>> +                       struct efi_device_path *remaining_device_path,
>> +                       bool recursive);
>> +       void *disconnect_controller;
>> +#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL  0x00000001
>> +#define EFI_OPEN_PROTOCOL_GET_PROTOCOL        0x00000002
>> +#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL       0x00000004
>> +#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008
>> +#define EFI_OPEN_PROTOCOL_BY_DRIVER           0x00000010
>> +#define EFI_OPEN_PROTOCOL_EXCLUSIVE           0x00000020
>
> Can we move these to efi.h?

Well they relate to a specific API method so I think they are better here.

>
>> +       efi_status_t (EFIAPI *open_protocol)(efi_handle_t handle,
>> +                       efi_guid_t *protocol, void **interface,
>> +                       efi_handle_t agent_handle,
>> +                       efi_handle_t controller_handle, u32 attributes);
>> +       void *close_protocol;
>> +       efi_status_t(EFIAPI *open_protocol_information)(efi_handle_t handle,
>> +                       efi_guid_t *protocol,
>> +                       struct efi_open_protocol_info_entry **entry_buffer,
>> +                       unsigned long *entry_count);
>> +       efi_status_t (EFIAPI *protocols_per_handle)(efi_handle_t handle,
>> +                       efi_guid_t ***protocol_buffer,
>> +                       unsigned long *protocols_buffer_count);
>> +       efi_status_t (EFIAPI *locate_handle_buffer) (
>> +                       enum efi_locate_search_type search_type,
>> +                       efi_guid_t *protocol, void *search_key,
>> +                       unsigned long *no_handles, efi_handle_t **buffer);
>> +       void *locate_protocol;
>> +       void *install_multiple_protocol_interfaces;
>> +       void *uninstall_multiple_protocol_interfaces;
>> +       void *calculate_crc32;
>> +       void *copy_mem;
>> +       void *set_mem;
>> +       void *create_event_ex;
>> +};
>> +
>> +/*
>> + * Types and defines for EFI ResetSystem
>> + */
>
> Please use one-line comment.
>
>> +enum efi_reset_type {
>> +       EFI_RESET_COLD = 0,
>> +       EFI_RESET_WARM = 1,
>> +       EFI_RESET_SHUTDOWN = 2
>> +};
>
> Nits: can we move this to efi.h?

I think it relates to the reset API so is best here. What do you think?

>
>> +
>> +/*
>> + * EFI Runtime Services table
>> + */
>
> Please use one-line comment.
>
>> +#define EFI_RUNTIME_SERVICES_SIGNATURE ((u64)0x5652453544e5552ULL)
>
> Is the (u64) cast necessary?
>
>> +#define EFI_RUNTIME_SERVICES_REVISION  0x00010000
>
> Nits: can we move the above two to efi.h?

Again I think this is API-related.

>
>> +struct efi_runtime_services {
>> +       struct efi_table_hdr hdr;
>> +       void *get_time;
>> +       void *set_time;
>> +       void *get_wakeup_time;
>> +       void *set_wakeup_time;
>> +       void *set_virtual_address_map;
>> +       void *convert_pointer;
>> +       efi_status_t (EFIAPI *get_variable)(s16 *variable_name,
>> +                       efi_guid_t *vendor, u32 *attributes,
>> +                       unsigned long *data_size, void *data);
>> +       efi_status_t (EFIAPI *get_next_variable)(
>> +                       unsigned long *variable_name_size,
>> +                       s16 *variable_name, efi_guid_t *vendor);
>> +       efi_status_t (EFIAPI *set_variable)(s16 *variable_name,
>> +                       efi_guid_t *vendor, u32 attributes,
>> +                       unsigned long data_size, void *data);
>> +       void *get_next_high_mono_count;
>> +       void (EFIAPI *reset_system)(enum efi_reset_type reset_type,
>> +                                   efi_status_t reset_status,
>> +                                   unsigned long data_size, void *reset_data);
>> +       void *update_capsule;
>> +       void *query_capsule_caps;
>> +       void *query_variable_info;
>> +};
>> +
>> +/*
>> + *  EFI Configuration Table and GUID definitions
>> + */
>
> Please use one-line comment.
>
>> +#define NULL_GUID \
>> +       EFI_GUID(0x00000000, 0x0000, 0x0000, 0x00, 0x00, \
>> +                0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
>> +
>> +#define LOADED_IMAGE_PROTOCOL_GUID \
>> +       EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, 0x8e, 0x3f, \
>> +                0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
>> +
>
> Nits: can we move the above two to efi.h?
>
>> +struct efi_system_table {
>> +       struct efi_table_hdr hdr;
>> +       unsigned long fw_vendor;   /* physical addr of CHAR16 vendor string */
>
> What's CHAR16? Is it wchar_t?
>
>> +       u32 fw_revision;
>> +       unsigned long con_in_handle;
>> +       struct efi_simple_input_interface *con_in;
>> +       unsigned long con_out_handle;
>> +       struct efi_simple_text_output_protocol *con_out;
>> +       unsigned long stderr_handle;
>> +       unsigned long std_err;
>> +       struct efi_runtime_services *runtime;
>> +       struct efi_boot_services *boottime;
>> +       unsigned long nr_tables;
>> +       unsigned long tables;
>> +};
>> +
>> +struct efi_loaded_image {
>> +       u32 revision;
>> +       void *parent_handle;
>> +       struct efi_system_table *system_table;
>> +       void *device_handle;
>> +       void *file_path;
>> +       void *reserved;
>> +       u32 load_options_size;
>> +       void *load_options;
>> +       void *image_base;
>> +       aligned_u64 image_size;
>> +       unsigned int image_code_type;
>> +       unsigned int image_data_type;
>> +       unsigned long unload;
>> +};
>> +
>> +struct __packed efi_device_path {
>
> __packed is not needed here.
>
>> +       u8 type;
>> +       u8 sub_type;
>> +       u16 length;
>> +};
>> +
>> +struct simple_text_output_mode {
>> +       s32 max_mode;
>> +       s32 mode;
>> +       s32 attribute;
>> +       s32 cursor_column;
>> +       s32 cursor_row;
>> +       bool cursor_visible;
>> +};
>> +
>> +struct efi_simple_text_output_protocol {
>> +       void *reset;
>> +       efi_status_t (EFIAPI *output_string)(
>> +                       struct efi_simple_text_output_protocol *this,
>> +                       const unsigned short *str);
>> +       void *test_string;
>> +
>> +       efi_status_t(EFIAPI *query_mode)(
>> +                       struct efi_simple_text_output_protocol *this,
>> +                       unsigned long mode_number, unsigned long *columns,
>> +                       unsigned long *rows);
>> +       efi_status_t(EFIAPI *set_mode)(
>> +                       struct efi_simple_text_output_protocol *this,
>> +                       unsigned long mode_number);
>> +       efi_status_t(EFIAPI *set_attribute)(
>> +                       struct efi_simple_text_output_protocol *this,
>> +                       unsigned long attribute);
>> +       efi_status_t(EFIAPI *clear_screen) (
>> +                       struct efi_simple_text_output_protocol *this);
>> +       efi_status_t(EFIAPI *set_cursor_position) (
>> +                       struct efi_simple_text_output_protocol *this,
>> +                       unsigned long column, unsigned long row);
>> +       efi_status_t(EFIAPI *enable_cursor)(void *, bool enable);
>> +       struct simple_text_output_mode *mode;
>> +};
>> +
>> +struct efi_input_key {
>> +       u16 scan_code;
>> +       s16 unicode_char;
>> +};
>> +
>> +struct efi_simple_input_interface {
>> +       efi_status_t(EFIAPI *reset)(struct efi_simple_input_interface *this,
>> +                       bool ExtendedVerification);
>> +       efi_status_t(EFIAPI *read_key_stroke)(
>> +                       struct efi_simple_input_interface *this,
>> +                       struct efi_input_key *key);
>> +       void *wait_for_key;
>> +};
>> +
>> +#endif
>> diff --git a/include/part_efi.h b/include/part_efi.h
>> index d68ef3b..3012b91 100644
>> --- a/include/part_efi.h
>> +++ b/include/part_efi.h
>> @@ -18,6 +18,8 @@
>>  #ifndef _DISK_PART_EFI_H
>>  #define _DISK_PART_EFI_H
>>
>> +#include <efi.h>
>> +
>>  #define MSDOS_MBR_SIGNATURE 0xAA55
>>  #define EFI_PMBR_OSTYPE_EFI 0xEF
>>  #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
>> @@ -29,13 +31,6 @@
>>  #define GPT_ENTRY_NUMBERS              128
>>  #define GPT_ENTRY_SIZE                 128
>>
>> -#define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
>> -       ((efi_guid_t) \
>> -       {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
>> -               (b) & 0xff, ((b) >> 8) & 0xff, \
>> -               (c) & 0xff, ((c) >> 8) & 0xff, \
>> -               (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
>> -
>>  #define PARTITION_SYSTEM_GUID \
>>         EFI_GUID( 0xC12A7328, 0xF81F, 0x11d2, \
>>                 0xBA, 0x4B, 0x00, 0xA0, 0xC9, 0x3E, 0xC9, 0x3B)
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index c98d399..4c8d38e 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -96,4 +96,6 @@ config ERRNO_STR
>>           - if errno is null or positive number - a pointer to "Success" message
>>           - if errno is negative - a pointer to errno related message
>>
>> +source lib/efi/Kconfig
>> +
>>  endmenu
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 97ed398..4821779 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -7,6 +7,7 @@
>>
>>  ifndef CONFIG_SPL_BUILD
>>
>> +obj-$(CONFIG_EFI) += efi/
>>  obj-$(CONFIG_RSA) += rsa/
>>  obj-$(CONFIG_LZMA) += lzma/
>>  obj-$(CONFIG_LZO) += lzo/
>> diff --git a/lib/efi/Kconfig b/lib/efi/Kconfig
>> new file mode 100644
>> index 0000000..2b3dbd4
>> --- /dev/null
>> +++ b/lib/efi/Kconfig
>> @@ -0,0 +1,33 @@
>> +config EFI
>> +       bool "Support running U-Boot from EFI"
>> +       depends on X86
>> +       help
>> +         U-Boot can be started from EFI on certain platforms. This allows
>> +         EFI to perform most of the system init and then jump to U-Boot for
>> +         final system boot. Another option is to run U-Boot as an EFI
>> +         application, with U-Boot using EFI's drivers instead of its own.
>> +
>> +choice
>> +       prompt "Select EFI mode to use"
>> +       depends on X86 && EFI
>> +
>> +config ARCH_EFI
>
> Can we change this to EFI_APP?
>
>> +       bool "Support running U-Boot from EFI"
>
> Nits: better to be "Support running as an EFI application".
>
>> +       help
>> +         Build U-Boot as an application which can be started from EFI. This
>> +         is useful for examining a platform in the early stages of porting
>> +         U-Boot to it. It allows only very basic functionality, such as a
>> +         command problem and memory and I/O functions. Use 'reset' to return
>
> I don't understand what is 'a command problem'.
>
>> +         to EFI.
>> +
>> +config EFI_RAM_SIZE
>> +       hex "Amount of EFI RAM for U-Boot"
>> +       depends on ARCH_EFI
>> +       default 0x2000000
>> +       help
>> +         Set the amount of EFI RAM which is claimed by U-Boot for its own
>> +         use. U-Boot allocates this from EFI on start-up (along with a few
>> +         other smaller amounts) and it can never be increased after that.
>> +         It is used as the RAM size in withU-Boot.
>
> Missing space between 'with' and 'U-Boot'
>
>> +
>> +endchoice
>> diff --git a/lib/efi/Makefile b/lib/efi/Makefile
>> new file mode 100644
>> index 0000000..137d2f9
>> --- /dev/null
>> +++ b/lib/efi/Makefile
>> @@ -0,0 +1,7 @@
>> +#
>> +# (C) Copyright 2015 Google, Inc
>> +#
>> +# SPDX-License-Identifier:     GPL-2.0+
>> +#
>> +
>> +obj-$(CONFIG_ARCH_EFI) += efi_app.o efi.o
>> diff --git a/lib/efi/efi.c b/lib/efi/efi.c
>> new file mode 100644
>> index 0000000..acd276c
>> --- /dev/null
>> +++ b/lib/efi/efi.c
>> @@ -0,0 +1,91 @@
>> +/*
>> + * Copyright (c) 2015 Google, Inc
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + *
>> + * EFI information obtained here:
>> + * http://wiki.phoenix.com/wiki/index.php/EFI_BOOT_SERVICES
>> + *
>> + * Common EFI functions
>> + */
>> +
>> +#include <common.h>
>> +#include <debug_uart.h>
>> +#include <errno.h>
>> +#include <linux/err.h>
>> +#include <linux/types.h>
>> +#include <efi.h>
>> +#include <efi_api.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static void efi_memset(void *ptr, int ch, int size)
>> +{
>> +       char *dest = ptr;
>> +
>> +       while (size-- > 0)
>> +               *dest++ = ch;
>> +}
>
> Why do we need this? Can we use memset() here?

Unfortunately we cannot access any code outside what be build
especially for the stub. lib/string.c is already being built for the
U-Boot payload so it using the wrong compiler flags.

>
>> +
>> +void efi_putc(struct efi_priv *priv, const char ch)
>> +{
>> +       struct efi_simple_text_output_protocol *con = priv->sys_table->con_out;
>> +       uint16_t ucode[2];
>> +
>> +       ucode[0] = ch;
>> +       ucode[1] = '\0';
>> +       con->output_string(con, ucode);
>> +}
>> +
>> +void efi_puts(struct efi_priv *priv, const char *str)
>> +{
>> +       while (*str)
>> +               efi_putc(priv, *str++);
>> +}
>> +
>> +int efi_init(struct efi_priv *priv, const char *banner, efi_handle_t image,
>> +            struct efi_system_table *sys_table)
>> +{
>> +       efi_guid_t loaded_image_guid = LOADED_IMAGE_PROTOCOL_GUID;
>> +       struct efi_boot_services *boot = sys_table->boottime;
>> +       struct efi_loaded_image *loaded_image;
>> +       int ret;
>> +
>> +       efi_memset(priv, '\0', sizeof(*priv));
>> +       priv->sys_table = sys_table;
>> +       priv->boot = sys_table->boottime;
>> +       priv->parent_image = image;
>> +       priv->run = sys_table->runtime;
>> +
>> +       efi_puts(priv, "U-Boot EFI ");
>> +       efi_puts(priv, banner);
>> +       efi_putc(priv, ' ');
>> +
>> +       ret = boot->open_protocol(priv->parent_image, &loaded_image_guid,
>> +                                 (void **)&loaded_image, &priv->parent_image,
>> +                                 NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>> +       if (ret) {
>> +               efi_puts(priv, "Failed to get loaded image protocol\n");
>> +               return ret;
>> +       }
>> +       priv->image_data_type = loaded_image->image_data_type;
>> +
>> +       return 0;
>> +}
>> +
>> +void *efi_malloc(struct efi_priv *priv, int size, efi_status_t *retp)
>> +{
>> +       struct efi_boot_services *boot = priv->boot;
>> +       void *buf = NULL;
>> +
>> +       *retp = boot->allocate_pool(priv->image_data_type, size, &buf);
>> +
>> +       return buf;
>> +}
>> +
>> +void efi_free(struct efi_priv *priv, void *ptr)
>> +{
>> +       struct efi_boot_services *boot = priv->boot;
>> +
>> +       boot->free_pool(ptr);
>> +}
>> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
>> new file mode 100644
>> index 0000000..b71669c
>> --- /dev/null
>> +++ b/lib/efi/efi_app.c
>> @@ -0,0 +1,125 @@
>> +/*
>> + * Copyright (c) 2015 Google, Inc
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + *
>> + * EFI information obtained here:
>> + * http://wiki.phoenix.com/wiki/index.php/EFI_BOOT_SERVICES
>> + *
>> + * This file implements U-Boot running as an EFI application.
>> + */
>> +
>> +#include <common.h>
>> +#include <debug_uart.h>
>> +#include <errno.h>
>> +#include <linux/err.h>
>> +#include <linux/types.h>
>> +#include <efi.h>
>> +#include <efi_api.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static struct efi_priv *global_priv;
>> +
>> +struct efi_system_table *efi_get_sys_table(void)
>> +{
>> +       return global_priv->sys_table;
>> +}
>> +
>> +unsigned long efi_get_ram_base(void)
>> +{
>> +       return global_priv->ram_base;
>> +}
>> +
>> +static efi_status_t setup_memory(struct efi_priv *priv)
>> +{
>> +       struct efi_boot_services *boot = priv->boot;
>> +       efi_physical_addr_t addr;
>> +       efi_status_t ret;
>> +       int pages;
>> +
>> +       global_data_ptr = efi_malloc(priv, sizeof(struct global_data), &ret);
>
> Where is this global_data_ptr defined? I guess this is introduced in
> later patches. The introduction of global_data_ptr should be moved to
> this patch.
>
>> +       if (!global_data_ptr)
>> +               return ret;
>> +       memset(gd, '\0', sizeof(*gd));
>> +
>
> And gd here? If gd == global_data_ptr, why no just use gd without
> adding global_data_ptr?
>
>> +       gd->malloc_base = (ulong)efi_malloc(priv, CONFIG_SYS_MALLOC_F_LEN,
>> +                                           &ret);
>> +       if (!gd->malloc_base)
>> +               return ret;
>> +       pages = CONFIG_EFI_RAM_SIZE >> 12;
>> +       addr = 1ULL << 32;
>
> Please add a comment to describe what we are trying to do here, eg:
> addr points to 4GiB, that means the allocated address should not
> exceed that.
>
>> +       ret = boot->allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>> +                                  priv->image_data_type, pages, &addr);
>> +       if (ret) {
>> +               printf("(using pool %lx) ", ret);
>> +               priv->ram_base = (ulong)efi_malloc(priv, CONFIG_EFI_RAM_SIZE,
>> +                                                  &ret);
>> +               if (!priv->ram_base)
>> +                       return ret;
>> +               priv->use_pool_for_malloc = true;
>> +       } else {
>> +               priv->ram_base = addr;
>> +       }
>> +       gd->ram_size = pages << 12;
>> +
>> +       return 0;
>> +}
>> +
>> +static void free_memory(struct efi_priv *priv)
>> +{
>> +       struct efi_boot_services *boot = priv->boot;
>> +
>> +       if (priv->use_pool_for_malloc)
>> +               efi_free(priv, (void *)priv->ram_base);
>> +       else
>> +               boot->free_pages(priv->ram_base, gd->ram_size >> 12);
>> +
>> +       efi_free(priv, (void *)gd->malloc_base);
>> +       efi_free(priv, gd);
>> +       global_data_ptr = NULL;
>> +}
>> +
>> +/**
>> + * efi_main() - Start an EFI image
>> + *
>> + * This function is called by our EFI start-up code. It handles running
>> + * U-Boot. If it returns, EFI will continue. Another way to get back to EFI
>> + * is via reset_cpu().
>> + */
>> +efi_status_t efi_main(efi_handle_t image, struct efi_system_table *sys_table)
>> +{
>> +       struct efi_priv local_priv, *priv = &local_priv;
>> +       efi_status_t ret;
>> +
>> +       /* Set up access to EFI data structures */
>> +       efi_init(priv, "App", image, sys_table);
>> +
>> +       global_priv = priv;
>> +
>> +       /* Set up the EFI debug UART so that printf() works */
>> +       debug_uart_init();
>
> Where is this implemented for efi app?
>
>> +
>> +       ret = setup_memory(priv);
>> +       if (ret) {
>> +               printf("Failed to set up memory: ret=%lx\n", ret);
>
> Is printf usable here? If yes, why do we create efi_puts() and efi_putc()?
>
>> +               return ret;
>> +       }
>> +
>> +       printf("starting\n");
>> +
>> +       board_init_f(GD_FLG_SKIP_RELOC);
>> +       board_init_r(NULL, 0);
>> +       free_memory(priv);
>> +
>> +       return EFI_SUCCESS;
>> +}
>> +
>> +void reset_cpu(ulong addr)
>> +{
>> +       struct efi_priv *priv = global_priv;
>> +
>> +       free_memory(priv);
>> +       printf("U-Boot EFI exiting\n");
>> +       priv->boot->exit(priv->parent_image, EFI_SUCCESS, 0, NULL);
>> +}
>> --
>
> Regards,
> Bin

Regards,
Simon



More information about the U-Boot mailing list