Hi Heinrich,

On 4 December 2017 at 15:15, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 12/04/2017 10:28 PM, Simon Glass wrote:

...

This function can fail but gives no indication of failure. Update it to return an error when something goes wrong.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Update return type of efi_smbios_register() to efi_status_t

• Use return value of efi_install_configuration_table

  include/efi_loader.h | 9 ++++++++- lib/efi_loader/efi_smbios.c | 7 ++++--- 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 1b92edbd77..35f8f84401 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -164,7 +164,14 @@ int efi_gop_register(void); /* Called by bootefi to make the network interface available */ int efi_net_register(void); /* Called by bootefi to make SMBIOS tables available */ -void efi_smbios_register(void); +/**

• • efi_smbios_register() - write out SMBIOS tables
• • Called by bootefi to make SMBIOS tables available
• • @return 0 if OK, -ENOMEM if no memory is available for the tables
• */

+efi_status_t efi_smbios_register(void); struct efi_simple_file_system_protocol * efi_fs_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index ac412e7362..67f71892ca 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -13,7 +13,7 @@ static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; -void efi_smbios_register(void) +efi_status_t efi_smbios_register(void) { /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */ uint64_t dmi = 0xffffffff; @@ -22,11 +22,12 @@ void efi_smbios_register(void) int memtype = EFI_RUNTIME_SERVICES_DATA; if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS)

Please, return the value returned from efi_allocate_pages(). The function has a lot of different failure modes.

OK, will do. But looking at efi_allocate_pages() there is no documentation as to the return value.

...

•           return;
  

•           return EFI_OUT_OF_RESOURCES;
    /* Generate SMBIOS tables */
    write_smbios_table(dmi);
  

We should add a comment explaining why we do not use the return value of write_smbios_table(). My understanding is: write_smbios_table returns dmi rounded up to a multiple of 16. efi_allocate_pages returns a 4096 aligned address. So we do expect that the return value equals the parameter apssed to write_smbios_table().

OK will do.

Regards, Simon

On 12/04/2017 10:28 PM, Simon Glass wrote:

Rather than having the caller check this variable and the callee set it, move all access to the variable inside the function. This reduces the logic needed to call efi_init_obj_list().

Signed-off-by: Simon Glass sjg@chromium.org Reviewed: Heinrich Schuchardt xypron.glpk@gmx.de

---

Changes in v2: None

cmd/bootefi.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 478bc116e2..17b26e6f4e 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -28,6 +28,8 @@ static struct efi_device_path *bootefi_device_path; /* Initialize and populate EFI object list */ static void efi_init_obj_list(void) {

• if (efi_obj_list_initalized)

• return;
  

  efi_obj_list_initalized = 1;

  efi_console_register();

@@ -208,6 +210,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", "{ro,boot}(blob)0000000000000000");

• /* Initialize and populate EFI object list */
• efi_init_obj_list();

If you add it here where do you remove the old call? We do not want to do the assignments twice.

• /* Call our payload! */ debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);

@@ -310,6 +315,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) /* Initialize and populate EFI object list */ if (!efi_obj_list_initalized) efi_init_obj_list();

• loaded_image_info.device_handle = bootefi_device_path;
  

• loaded_image_info.file_path = bootefi_image_path;
  

Same.

Best regards

Heinrich

return efi_selftest(&loaded_image_info, &systab);

} else #endif

On 12/04/2017 10:28 PM, Simon Glass wrote:

This function calls a function which can fail. Print a message in this case and abort the boot, rather than silently continuing to boot, which will certainly fail.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Change return type of efi_init_obj_list() to efi_status_t

  cmd/bootefi.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 17b26e6f4e..a2138f6075 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized; static struct efi_device_path *bootefi_image_path; static struct efi_device_path *bootefi_device_path;

-/* Initialize and populate EFI object list */ -static void efi_init_obj_list(void) +/**

• • efi_init_obj_list() - Initialize and populate EFI object list
• • @return 0 if OK, -ve on error (in which case it prints a message)
• */

+static efi_status_t efi_init_obj_list(void) {

• efi_status_t ret;
• if (efi_obj_list_initalized)

• return;
  

• return 0;
  

  efi_obj_list_initalized = 1;

  efi_console_register();

@@ -43,12 +49,19 @@ static void efi_init_obj_list(void) efi_net_register();

This function can also fail. Same is true for efi_gop_register.

#endif #ifdef CONFIG_GENERATE_SMBIOS_TABLE

• efi_smbios_register();

• ret = efi_smbios_register();
• if (ret)

if (ret != EFI_SUCCESS)

• goto error;
  

  #endif

  /* Initialize EFI runtime services */ efi_reset_system_init(); efi_get_time_init();

• return EFI_SUCCESS;

+error:

• printf("Error: Cannot set up EFI object list (err=%d)\n", ret);

Now we have some objects initialized and others not. Before returning we should clean up. We should free all allocated objects.

Best regards

Heinrich

• return ret; }

  static void *copy_fdt(void *fdt)

@@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, ulong fdt_pages, fdt_size, fdt_start, fdt_end; const efi_guid_t fdt_guid = EFI_FDT_GUID; bootm_headers_t img = { 0 };

• int ret;

  /*

  • Special case for efi payload not loaded from disk, such as

@@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, "{ro,boot}(blob)0000000000000000");

/* Initialize and populate EFI object list */

• efi_init_obj_list();

• ret = efi_init_obj_list();

• if (ret)

• return ret;
  
  

  /* Call our payload! */ debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);

@@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) */ efi_save_gd(); /* Initialize and populate EFI object list */

• if (!efi_obj_list_initalized)
  

• 	efi_init_obj_list();
  

• if (!efi_obj_list_initalized && efi_init_obj_list())
  

• 	return CMD_RET_FAILURE;
  

• loaded_image_info.device_handle = bootefi_device_path; loaded_image_info.file_path = bootefi_image_path;
• return efi_selftest(&loaded_image_info, &systab); } else #endif

On 12/04/2017 10:28 PM, Simon Glass wrote:

This function calls a function which can fail. Print a message in this case and abort the boot, rather than silently continuing to boot, which will certainly fail.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Change return type of efi_init_obj_list() to efi_status_t

cmd/bootefi.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 17b26e6f4e..a2138f6075 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized; static struct efi_device_path *bootefi_image_path; static struct efi_device_path *bootefi_device_path;

-/* Initialize and populate EFI object list */ -static void efi_init_obj_list(void) +/**

• • efi_init_obj_list() - Initialize and populate EFI object list
• • @return 0 if OK, -ve on error (in which case it prints a message)
• */

+static efi_status_t efi_init_obj_list(void) {

• efi_status_t ret;
• if (efi_obj_list_initalized)

• return;
  

• return 0;
  

  efi_obj_list_initalized = 1;

  efi_console_register();

@@ -43,12 +49,19 @@ static void efi_init_obj_list(void) efi_net_register(); #endif #ifdef CONFIG_GENERATE_SMBIOS_TABLE

• efi_smbios_register();

• ret = efi_smbios_register();
• if (ret)

• goto error;
  

#endif

/* Initialize EFI runtime services */ efi_reset_system_init(); efi_get_time_init();

• return EFI_SUCCESS;

+error:

• printf("Error: Cannot set up EFI object list (err=%d)\n", ret);

warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘efi_status_t {aka long unsigned int}’ [-Wformat=]

Please, use %lu.

Regards

Heinrich

• return ret;

}

static void *copy_fdt(void *fdt) @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, ulong fdt_pages, fdt_size, fdt_start, fdt_end; const efi_guid_t fdt_guid = EFI_FDT_GUID; bootm_headers_t img = { 0 };

• int ret;

  /*

  • Special case for efi payload not loaded from disk, such as

@@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, "{ro,boot}(blob)0000000000000000");

/* Initialize and populate EFI object list */

• efi_init_obj_list();

• ret = efi_init_obj_list();

• if (ret)

• return ret;
  
  

  /* Call our payload! */ debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);

@@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) */ efi_save_gd(); /* Initialize and populate EFI object list */

• if (!efi_obj_list_initalized)
  

• 	efi_init_obj_list();
  

• if (!efi_obj_list_initalized && efi_init_obj_list())
  

• 	return CMD_RET_FAILURE;
  

• loaded_image_info.device_handle = bootefi_device_path; loaded_image_info.file_path = bootefi_image_path;
• return efi_selftest(&loaded_image_info, &systab); } else

#endif

On 12/04/2017 10:28 PM, Simon Glass wrote:

With sandbox the U-Boot code is not mapped into the sandbox memory range so does not need to be excluded when allocating EFI memory. Update the EFI memory init code to take account of that.

Also use mapmem instead of a cast to convert a memory address to a pointer.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Update to use mapmem instead of a cast

lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e95896ca0a..3ad58d8930 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -10,6 +10,7 @@ #include <efi_loader.h> #include <inttypes.h> #include <malloc.h> +#include <mapmem.h> #include <watchdog.h> #include <asm/global_data.h> #include <linux/list_sort.h> @@ -366,7 +367,7 @@ efi_status_t efi_allocate_pool(int pool_type, unsigned long size, r = efi_allocate_pages(0, pool_type, num_pages, &t);

if (r == EFI_SUCCESS) {

• struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
  

• struct efi_pool_allocation *alloc = map_sysmem(t, size);
  

  alloc->num_pages = num_pages; *buffer = alloc->data; }

@@ -460,18 +461,22 @@ int efi_memory_init(void)

efi_add_known_memory();

• /* Add U-Boot */
• uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
• uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
• efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
• /* Add Runtime Services */
• runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
• runtime_end = (ulong)&__efi_runtime_stop;
• runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
• runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
• efi_add_memory_map(runtime_start, runtime_pages,

• 	   EFI_RUNTIME_SERVICES_CODE, false);
  

• if (!IS_ENABLED(CONFIG_SANDBOX)) {

• /* Add U-Boot */
  

• uboot_start = (gd->start_addr_sp - uboot_stack_size) &
  

• 		~EFI_PAGE_MASK;
  

• uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
  

• efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA,
  

• 		   false);
  

• /* Add Runtime Services */
  

• runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
  

• runtime_end = (ulong)&__efi_runtime_stop;
  

• runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
  

• runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
  

• efi_add_memory_map(runtime_start, runtime_pages,
  

• 		   EFI_RUNTIME_SERVICES_CODE, false);
  

• }

#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER /* Request a 32bit 64MB bounce buffer region */

bootefi selftest shows an error. You should setup the EFI memory for CONFIG_SANDBOX=y too.

Setting up 'ExitBootServices' Setting up 'ExitBootServices' succeeded lib/efi_selftest/efi_selftest.c(53): ERROR: GetMemoryMap did not return EFI_SUCCESS

Best regards

Heinrich

On 12/04/2017 10:28 PM, Simon Glass wrote:

At present this code casts addresses to pointers so cannot be used with sandbox. Update it to use mapmem instead.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2: None

lib/smbios.c | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c index 8f19ad89c1..56481d448d 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -7,6 +7,7 @@ */

#include <common.h> +#include <mapmem.h> #include <smbios.h> #include <tables_csum.h> #include <version.h> @@ -75,9 +76,10 @@ static int smbios_string_table_len(char *start)

static int smbios_write_type0(ulong *current, int handle) {

• struct smbios_type0 *t = (struct smbios_type0 *)*current;

• struct smbios_type0 *t; int len = sizeof(struct smbios_type0);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type0)); fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); t->vendor = smbios_add_string(t->eos, "U-Boot");

@@ -104,16 +106,18 @@ static int smbios_write_type0(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len;

}

static int smbios_write_type1(ulong *current, int handle) {

• struct smbios_type1 *t = (struct smbios_type1 *)*current;

• struct smbios_type1 *t; int len = sizeof(struct smbios_type1); char *serial_str = env_get("serial#");

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type1)); fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);

@@ -125,15 +129,17 @@ static int smbios_write_type1(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len;

}

static int smbios_write_type2(ulong *current, int handle) {

• struct smbios_type2 *t = (struct smbios_type2 *)*current;

• struct smbios_type2 *t; int len = sizeof(struct smbios_type2);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type2)); fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);

@@ -143,15 +149,17 @@ static int smbios_write_type2(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len;

}

static int smbios_write_type3(ulong *current, int handle) {

• struct smbios_type3 *t = (struct smbios_type3 *)*current;

• struct smbios_type3 *t; int len = sizeof(struct smbios_type3);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type3)); fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);

@@ -163,6 +171,7 @@ static int smbios_write_type3(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len;

} @@ -201,9 +210,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t)

static int smbios_write_type4(ulong *current, int handle) {

• struct smbios_type4 *t = (struct smbios_type4 *)*current;

• struct smbios_type4 *t; int len = sizeof(struct smbios_type4);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type4)); fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle); t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;

@@ -217,32 +227,37 @@ static int smbios_write_type4(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len;

}

static int smbios_write_type32(ulong *current, int handle) {

• struct smbios_type32 *t = (struct smbios_type32 *)*current;

• struct smbios_type32 *t; int len = sizeof(struct smbios_type32);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type32)); fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle);

  *current += len;

• unmap_sysmem(t);

  return len;

}

static int smbios_write_type127(ulong *current, int handle) {

• struct smbios_type127 *t = (struct smbios_type127 *)*current;

• struct smbios_type127 *t; int len = sizeof(struct smbios_type127);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type127)); fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle);

  *current += len;

• unmap_sysmem(t);

  return len;

} @@ -271,7 +286,7 @@ ulong write_smbios_table(ulong addr) /* 16 byte align the table address */ addr = ALIGN(addr, 16);

• se = (struct smbios_entry *)(uintptr_t)addr;

• se = map_sysmem(addr, sizeof(struct smbios_entry)); memset(se, 0, sizeof(struct smbios_entry));

  addr += sizeof(struct smbios_entry);

@@ -280,9 +295,13 @@ ulong write_smbios_table(ulong addr)

/* populate minimum required tables */ for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {

• int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
  

• ulong *ptr = map_sysmem(addr, 0);
  

This replaces &addr by addr and breaks booting on ARM64.

Did you mean: ulong *ptr = map_sysmem((phys_addr_t)&addr, 0);

The coding would get much easier to maintain if the map_sysmem() function would be eliminated from U-Boot.

Why don't you simply use the address range that mmap() has provided? This would avoid double book keeping.

Regards

Heinrich

• int tmp;
  

• tmp = smbios_write_funcs[i](ptr, handle++);
  

  max_struct_size = max(max_struct_size, tmp); len += tmp;

• unmap_sysmem(ptr);
  

  }

  memcpy(se->anchor, "_SM_", 4);

@@ -300,6 +319,7 @@ ulong write_smbios_table(ulong addr) isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET; se->intermediate_checksum = table_compute_checksum(istart, isize); se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry));

• unmap_sysmem(se);

  return addr;

}

On 12/04/2017 10:28 PM, Simon Glass wrote:

With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.

How would you run this on a Pinebook (arm64)?

Shouldn't we strive to make sandbox work on any architecture?

Best regards

Heinrich

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2: None

include/config_distro_bootcmd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 5c469a23fa..6772d2d404 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -242,7 +242,7 @@ #elif defined(CONFIG_ARM) #define BOOTENV_EFI_PXE_ARCH "0xa" #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000" -#elif defined(CONFIG_X86) +#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX) /* Always assume we're running 64bit */ #define BOOTENV_EFI_PXE_ARCH "0x7" #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"

On 12/04/2017 11:41 PM, Heinrich Schuchardt wrote:

On 12/04/2017 10:28 PM, Simon Glass wrote:

...

This undocumented function relies on arch-specific code to declare a nop

Undocumented code is unsatisfactory. Please, add a comment to the function.

Sorry I missed you put that in a separate patch (12/16).

BR

Best regards

Heinrich

...

weak version. Add the weak function in common code instead to avoid having to duplicate the same function in each arch.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2: None

arch/arm/include/asm/u-boot-arm.h | 1 -   arch/x86/include/asm/u-boot-x86.h | 1 -   arch/x86/lib/bootm.c              | 4 ----   common/bootm.c                    | 4 ++++   include/bootm.h                   | 2 ++   5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/u-boot-arm.h b/arch/arm/include/asm/u-boot-arm.h index ef4fca68ee..73ccf41f8c 100644 --- a/arch/arm/include/asm/u-boot-arm.h +++ b/arch/arm/include/asm/u-boot-arm.h @@ -38,7 +38,6 @@ int    arch_early_init_r(void);   /* board/.../... */   int    board_init(void); -void    board_quiesce_devices(void);   /* cpu/.../interrupt.c */   int    arch_interrupt_init    (void); diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h index 187fe5fd8c..d2e1426042 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -85,7 +85,6 @@ static inline __attribute__((no_instrument_function)) uint64_t rdtsc(void)   /* board/... */   void timer_set_tsc_base(uint64_t new_base);   uint64_t timer_get_tsc(void); -void board_quiesce_devices(void);   void quick_ram_check(void); diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index e548cdbed5..60ae043f24 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -33,10 +33,6 @@ int arch_fixup_fdt(void *blob)       return 0;   } -__weak void board_quiesce_devices(void) -{ -}

void bootm_announce_and_cleanup(void)   {       printf("\nStarting kernel ...\n\n"); diff --git a/common/bootm.c b/common/bootm.c index 9493a306cd..9fba0472ac 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -47,6 +47,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,                      char * const argv[], bootm_headers_t *images,                      ulong *os_data, ulong *os_len); +__weak void board_quiesce_devices(void) +{ +}

#ifdef CONFIG_LMB   static void boot_start_lmb(bootm_headers_t *images)   { diff --git a/include/bootm.h b/include/bootm.h index 49813772ce..76b6ab42e6 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -73,4 +73,6 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,                  void *load_buf, void *image_buf, ulong image_len,                  uint unc_len, ulong *load_end); +void board_quiesce_devices(void);

#endif

On 12/04/2017 10:28 PM, Simon Glass wrote:

This jumps to test code which can call directly into the EFI support. It does not need a separate image so it is easy to write tests with it.

For now the test just outputs a message. To try it:

Hello Simon,

why do we need "bootefi test"? What do you plan to do here which we cannot do in "bootefi selftest? I think we should unify both.

Best regards

Heinrich

./sandbox/u-boot -c "bootefi test" U-Boot 2017.09-00204-g696c9855fe (Sep 17 2017 - 16:43:53 -0600)

DRAM: 128 MiB MMC: Using default environment

In: serial Out: serial Err: serial SCSI: Net: No ethernet found. IDE: Bus 0: not available Found 0 disks Hello, world! Test passed

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Rebase to master

  cmd/bootefi.c | 25 +++++++++++++++++++++++-- configs/sandbox_defconfig | 1 + include/efi_loader.h | 3 +++ lib/efi_loader/Kconfig | 10 ++++++++++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_test.c | 17 +++++++++++++++++ 6 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 lib/efi_loader/efi_test.c

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index a4686f17f0..a77d8b256e 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -148,13 +148,11 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt, struct efi_object loaded_image_info_obj = {}; struct efi_device_path *memdp = NULL; ulong ret;

• ulong (*entry)(void *image_handle, struct efi_system_table *st) asmlinkage; ulong fdt_pages, fdt_size, fdt_start, fdt_end; const efi_guid_t fdt_guid = EFI_FDT_GUID; bootm_headers_t img = { 0 };

• int ret;

  /*

  • Special case for efi payload not loaded from disk, such as

@@ -318,6 +316,29 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) memcpy((char *)addr, __efi_helloworld_begin, size); } else #endif

• if (IS_ENABLED(CONFIG_BOOTEFI_TEST) && !strcmp(argv[1], "test")) {

• int ret;
  

• /* Initialize and populate EFI object list */
  

• if (efi_init_obj_list())
  

• 	return CMD_RET_FAILURE;
  

• struct efi_loaded_image loaded_image_info = {};
  

• struct efi_object loaded_image_info_obj = {};
  

• efi_setup_loaded_image(&loaded_image_info,
  

• 		       &loaded_image_info_obj,
  

• 		       bootefi_device_path, bootefi_image_path);
  

• ret = efi_test(&loaded_image_info, &systab);
  

• if (ret) {
  

• 	printf("Test failed: err=%d\n", ret);
  

• 	return CMD_RET_FAILURE;
  

• }
  

• printf("Test passed\n");
  

• return 0;
  

• } #ifdef CONFIG_CMD_BOOTEFI_SELFTEST if (!strcmp(argv[1], "selftest")) { struct efi_loaded_image loaded_image_info = {};

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 08eec8ecc6..ccf142c51d 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -194,3 +194,4 @@ CONFIG_UT_TIME=y CONFIG_UT_DM=y CONFIG_UT_ENV=y CONFIG_UT_OVERLAY=y +CONFIG_CMD_BOOTEFI_SELFTEST=y diff --git a/include/efi_loader.h b/include/efi_loader.h index 35f8f84401..90018fbe9e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -335,6 +335,9 @@ efi_status_t EFIAPI efi_set_variable(s16 *variable_name, void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path);

+/* Perform EFI tests */ +int efi_test(efi_handle_t image_handle, struct efi_system_table *systab);

• #else /* defined(EFI_LOADER) && !defined(CONFIG_SPL_BUILD) */

  /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index dee0a96a98..659b2b18f4 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -16,3 +16,13 @@ config EFI_LOADER_BOUNCE_BUFFER Some hardware does not support DMA to full 64bit addresses. For this hardware we can create a bounce buffer so that payloads don't have to worry about platform details.

+config BOOTEFI_TEST

• bool "Provide a test for the EFI loader"
• depends on EFI_LOADER && SANDBOX
• default y
• help

•  Provides a test of the EFI loader functionality accessed via the
  

•  command line ('bootefi test'). This runs within U-Boot so does not
  

•  need a separate EFI application to work. It aims to include coverage
  

•  of all EFI code which can be accessed within sandbox.
  

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index ddb978f650..14ef85cc67 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -23,3 +23,4 @@ obj-$(CONFIG_DM_VIDEO) += efi_gop.o obj-$(CONFIG_PARTITIONS) += efi_disk.o obj-$(CONFIG_NET) += efi_net.o obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o +obj-$(CONFIG_BOOTEFI_TEST) += efi_test.o diff --git a/lib/efi_loader/efi_test.c b/lib/efi_loader/efi_test.c new file mode 100644 index 0000000000..3fdce78b05 --- /dev/null +++ b/lib/efi_loader/efi_test.c @@ -0,0 +1,17 @@ +/*

• • Copyright (c) 2017, Google Inc. All rights reserved.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <efi_api.h>

+int efi_test(efi_handle_t image_handle, struct efi_system_table *systable) +{

• struct efi_simple_text_output_protocol *con_out = systable->con_out;
• con_out->output_string(con_out, L"Hello, world!\n");
• return 0;

+}