Hi Bin,
On 23 July 2015 at 02:09, Bin Meng <bmeng...@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass <s...@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 <s...@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 <drumm...@valinux.com>
>> + * Copyright (C) 1999, 2002-2003 Hewlett-Packard Co.
>> + * David Mosberger-Tang <dav...@hpl.hp.com>
>> + * Stephane Eranian <eran...@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 <drumm...@valinux.com>
>> + * Copyright (C) 1999, 2002-2003 Hewlett-Packard Co.
>> + * David Mosberger-Tang <dav...@hpl.hp.com>
>> + * Stephane Eranian <eran...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
