On 09/18/2017 12:59 AM, 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

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

diff --git a/include/efi_loader.h b/include/efi_loader.h index 2051fc994e..79d2dad22c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -150,8 +150,14 @@ int efi_disk_register(void); int efi_gop_register(void); /* Called by bootefi to make the network interface available */ int efi_net_register(void **handle); -/* 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
• */

+int efi_smbios_register(void);

/* Called by networking code to memorize the dhcp ack package */ void efi_net_set_dhcp_ack(void *pkt, int len); diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index ac412e7362..3b87294dc3 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) +int efi_smbios_register(void)

Please, use efi_status_t as return type.

{ /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */ uint64_t dmi = 0xffffffff; @@ -22,11 +22,13 @@ void efi_smbios_register(void) int memtype = EFI_RUNTIME_SERVICES_DATA;

if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS)

• return;
  

• return -ENOMEM;
  

Use return EFI_OUT_OF_RESOURCES This matches the value returned by efi_install_configuration_table.

/* Generate SMBIOS tables */ write_smbios_table(dmi);

/* And expose them to our EFI payload */ efi_install_configuration_table(&smbios_guid, (void*)(uintptr_t)dmi);

This function can return EFI_OUT_OF_RESOURCES.

• return 0;

Use return EFI_SUCCESS;

Regards Heinrich

}

On 09/18/2017 12:59 AM, 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

cmd/bootefi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index abfab8fa98..2c9d31c5eb 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -109,6 +109,8 @@ static struct efi_object bootefi_device_obj = { /* Initialize and populate EFI object list */ static void efi_init_obj_list(void) {

• if (efi_obj_list_initalized)

• return;
  

  efi_obj_list_initalized = 1;

  list_add_tail(&loaded_image_info_obj.link, &efi_obj_list);

@@ -256,8 +258,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt) return -ENOENT;

/* Initialize and populate EFI object list */

• if (!efi_obj_list_initalized)

• efi_init_obj_list();
  

• efi_init_obj_list();

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

@@ -311,8 +312,7 @@ 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();
  

• efi_init_obj_list();
  

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

Reviewed: Heinrich Schuchardt xypron.glpk@gmx.de

On 09/18/2017 12:59 AM, 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

cmd/bootefi.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 2c9d31c5eb..9aa588eb1b 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -106,11 +106,17 @@ static struct efi_object bootefi_device_obj = { }, };

-/* 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 int efi_init_obj_list(void)

Use efi_status_t as return type.

{

• int ret;
• if (efi_obj_list_initalized)

• return;
  

• return 0;
  

  efi_obj_list_initalized = 1;

  list_add_tail(&loaded_image_info_obj.link, &efi_obj_list);

@@ -132,12 +138,19 @@ static void efi_init_obj_list(void) loaded_image_info.device_handle = bootefi_device_path; #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 0;

+error:

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

}

static void *copy_fdt(void *fdt) @@ -219,6 +232,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;

  /*

  • gd lives in a fixed register which may get clobbered while we execute

@@ -258,7 +272,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt) return -ENOENT;

/* 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);

@@ -312,7 +328,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) */ efi_save_gd(); /* Initialize and populate EFI object list */

• efi_init_obj_list();
  

• if (efi_init_obj_list())
  

• 	return CMD_RET_FAILURE;
  

We are duplicating code here.

efi_save_gd and efi_init_obj_list should have been moved to the start of do_bootefi_exec in my patch efi_selftest: provide unit test for event services

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

Hi Heinrich,

On 17 September 2017 at 22:47, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 09/18/2017 12:59 AM, Simon Glass wrote:

...

This function repeats data structures provided by driver model. They are only created once so can be stale if the EFI loader is called twice (e.g. for testing or on boot failure).

Add a TODO to address this. It should be possible to attach EFI devices and data structures to driver-model devices and avoid having a parallel set of data structures.

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 9aa588eb1b..ee07733e3e 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -109,6 +109,10 @@ static struct efi_object bootefi_device_obj = { /**

• efi_init_obj_list() - Initialize and populate EFI object list

• • TODO(sjg@chromium.org): Move this to a dynamic list based on driver model,
• • so that it does not need to be created before running EFI applications
• • and updates when devices change.

I am not quite sure if by dynamic list you refer to linker generated lists or to hot plugging.

The UEFI spec 2.7 has a chapter on "Hot-Plug Events". This would add a lot of complexity. I do not think that we currently need it.

The object list also gets new entries created by the EFI application via calling InstallMultipleProtocolInterfaces of InstallProtocolInterface with *handle == NULL.

Here I am referring to what I see as duplicate device tables, brought in from driver model. I am wondering if we can make the EFI info come from the driver-model directly.

Regards, Simon

On 09/18/2017 12:59 AM, Simon Glass wrote:

The headers are not in the correct order. Fix this.

The commit message should tell that and explain why you are dropping #include <libfdt_env.h>

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

lib/efi_loader/efi_memory.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9e079f1fa3..ad3d277be6 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -8,12 +8,11 @@

#include <common.h> #include <efi_loader.h> +#include <inttypes.h> #include <malloc.h> +#include <watchdog.h> #include <asm/global_data.h> -#include <libfdt_env.h> #include <linux/list_sort.h> -#include <inttypes.h> -#include <watchdog.h>

DECLARE_GLOBAL_DATA_PTR;

On 09/18/2017 12:59 AM, 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.

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

lib/efi_loader/efi_memory.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ad3d277be6..c1a080e2e9 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -459,18 +459,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)) {

We need the memory maps for: GetMemoryMap ExitBootServices

In efi_init_memory_init you define 8 MB memory starting at 0x0000000000000000 as available: ram_start 0 start 0 pages 8000

I suggest you override efi_init_memory_init using malloc to assign at least 128 MB contiguous memory with alignment EFI_PAGE_SIZE.

Regards

Heinrich

• /* 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 */

On 09/18/2017 12:59 AM, Simon Glass wrote:

Add an implementation of setjmp() and longjmp() which rely on the underlying host C library. Since we cannot know how large the jump buffer needs to be, pick something that should be suitable and check it at runtime. At present we need access to the underlying struct as well.

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

arch/sandbox/cpu/cpu.c | 13 +++++++++++++ arch/sandbox/cpu/os.c | 17 +++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++++++++ include/os.h | 21 +++++++++++++++++++++ 4 files changed, 72 insertions(+) create mode 100644 arch/sandbox/include/asm/setjmp.h

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index 01991049cc..de5862a53b 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -9,6 +9,7 @@ #include <libfdt.h> #include <os.h> #include <asm/io.h> +#include <asm/setjmp.h> #include <asm/state.h> #include <dm/root.h>

@@ -154,3 +155,15 @@ ulong timer_get_boot_us(void)

return (count - base_count) / 1000; }

+int setjmp(jmp_buf jmp) +{

• return os_setjmp((ulong *)jmp, sizeof(jmp));

+}

+void longjmp(jmp_buf jmp, int ret) +{

• os_longjmp((ulong *)jmp, ret);
• while (1)

• ;
  

+} diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 22d6aab534..909034fa4b 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -7,6 +7,7 @@ #include <errno.h> #include <fcntl.h> #include <getopt.h> +#include <setjmp.h> #include <stdio.h> #include <stdint.h> #include <stdlib.h> @@ -609,3 +610,19 @@ void os_localtime(struct rtc_time *rt) rt->tm_yday = tm->tm_yday; rt->tm_isdst = tm->tm_isdst; }

+int os_setjmp(ulong *jmp, int size) +{

• if (size < sizeof(jmp_buf)) {

• printf("setjmp: jmpbuf is too small (%d bytes, need %d)\n",
  

•        size, sizeof(jmp_buf));
  

• return -ENOSPC;
  

• }
• return setjmp((struct __jmp_buf_tag *)jmp);

+}

+void os_longjmp(ulong *jmp, int ret) +{

• longjmp((struct __jmp_buf_tag *)jmp, ret);

+} diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h new file mode 100644 index 0000000000..e25f50107c --- /dev/null +++ b/arch/sandbox/include/asm/setjmp.h @@ -0,0 +1,21 @@ +/*

• • (C) Copyright 2016
• • Alexander Graf agraf@suse.de
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _SETJMP_H_ +#define _SETJMP_H_ 1

+struct jmp_buf_data {

• /* We're not sure how long this should be */
• ulong data[32];

+};

+typedef struct jmp_buf_data jmp_buf[1];

+int setjmp(jmp_buf jmp); +__noreturn void longjmp(jmp_buf jmp, int ret);

+#endif /* _SETJMP_H_ */ diff --git a/include/os.h b/include/os.h index 2bf4bdb1b8..ad1836ac9f 100644 --- a/include/os.h +++ b/include/os.h @@ -310,4 +310,25 @@ int os_spl_to_uboot(const char *fname); */ void os_localtime(struct rtc_time *rt);

+/**

• • os_setjmp() - Call setjmp()
• • Call the host system's setjmp() function.
• • @jmp: Buffer to store current execution state
• • @size: Size of buffer
• • @return normal setjmp() value if OK, -ENOSPC if @size is too small
• */

+int os_setjmp(ulong *jmp, int size);

+/**

• • os_longjmp() - Call longjmp()
• • Call the host system's longjmp() function.
• • @jmp: Buffer where previous execution state was stored
• • @ret: Value to pass to longjmp()
• */

+void os_longjmp(ulong *jmp, int ret);

#endif

Please, fix this warning:

arch/sandbox/cpu/cpu.c: In function ‘setjmp’: arch/sandbox/cpu/cpu.c:161:39: warning: ‘sizeof’ on array function parameter ‘jmp’ will return size of ‘struct jmp_buf_data *’ [-Wsizeof-array-argument] return os_setjmp((ulong *)jmp, sizeof(jmp)); ^ arch/sandbox/cpu/cpu.c:159:20: note: declared here int setjmp(jmp_buf jmp)

Regards

Heinrich

Hi,

On 18 September 2017 at 13:10, Rob Clark robdclark@gmail.com wrote:

On Mon, Sep 18, 2017 at 2:01 AM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/18/2017 12:59 AM, Simon Glass wrote:

...

Add an implementation of setjmp() and longjmp() which rely on the underlying host C library. Since we cannot know how large the jump buffer needs to be, pick something that should be suitable and check it at runtime. At present we need access to the underlying struct as well.

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

arch/sandbox/cpu/cpu.c | 13 +++++++++++++ arch/sandbox/cpu/os.c | 17 +++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++++++++ include/os.h | 21 +++++++++++++++++++++ 4 files changed, 72 insertions(+) create mode 100644 arch/sandbox/include/asm/setjmp.h

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index 01991049cc..de5862a53b 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -9,6 +9,7 @@ #include <libfdt.h> #include <os.h> #include <asm/io.h> +#include <asm/setjmp.h> #include <asm/state.h> #include <dm/root.h>

@@ -154,3 +155,15 @@ ulong timer_get_boot_us(void)

  return (count - base_count) / 1000;

}

+int setjmp(jmp_buf jmp) +{

• return os_setjmp((ulong *)jmp, sizeof(jmp));
  

+}

+void longjmp(jmp_buf jmp, int ret) +{

• os_longjmp((ulong *)jmp, ret);
  

• while (1)
  

•         ;
  

+} diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 22d6aab534..909034fa4b 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -7,6 +7,7 @@ #include <errno.h> #include <fcntl.h> #include <getopt.h> +#include <setjmp.h> #include <stdio.h> #include <stdint.h> #include <stdlib.h> @@ -609,3 +610,19 @@ void os_localtime(struct rtc_time *rt) rt->tm_yday = tm->tm_yday; rt->tm_isdst = tm->tm_isdst; }

+int os_setjmp(ulong *jmp, int size) +{

• if (size < sizeof(jmp_buf)) {
  

•         printf("setjmp: jmpbuf is too small (%d bytes, need %d)\n",
  

•                size, sizeof(jmp_buf));
  

•         return -ENOSPC;
  

• }
  

• return setjmp((struct __jmp_buf_tag *)jmp);
  

+}

+void os_longjmp(ulong *jmp, int ret) +{

• longjmp((struct __jmp_buf_tag *)jmp, ret);
  

+} diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h new file mode 100644 index 0000000000..e25f50107c --- /dev/null +++ b/arch/sandbox/include/asm/setjmp.h @@ -0,0 +1,21 @@ +/*

• • (C) Copyright 2016
• • Alexander Graf agraf@suse.de
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _SETJMP_H_ +#define _SETJMP_H_ 1

+struct jmp_buf_data {

• /* We're not sure how long this should be */
  

• ulong data[32];
  

+};

+typedef struct jmp_buf_data jmp_buf[1];

+int setjmp(jmp_buf jmp); +__noreturn void longjmp(jmp_buf jmp, int ret);

+#endif /* _SETJMP_H_ */ diff --git a/include/os.h b/include/os.h index 2bf4bdb1b8..ad1836ac9f 100644 --- a/include/os.h +++ b/include/os.h @@ -310,4 +310,25 @@ int os_spl_to_uboot(const char *fname); */ void os_localtime(struct rtc_time *rt);

+/**

• • os_setjmp() - Call setjmp()
• • Call the host system's setjmp() function.
• • @jmp: Buffer to store current execution state
• • @size: Size of buffer
• • @return normal setjmp() value if OK, -ENOSPC if @size is too small
• */

+int os_setjmp(ulong *jmp, int size);

+/**

• • os_longjmp() - Call longjmp()
• • Call the host system's longjmp() function.
• • @jmp: Buffer where previous execution state was stored
• • @ret: Value to pass to longjmp()
• */

+void os_longjmp(ulong *jmp, int ret);

#endif

Please, fix this warning:

arch/sandbox/cpu/cpu.c: In function ‘setjmp’: arch/sandbox/cpu/cpu.c:161:39: warning: ‘sizeof’ on array function parameter ‘jmp’ will return size of ‘struct jmp_buf_data *’ [-Wsizeof-array-argument] return os_setjmp((ulong *)jmp, sizeof(jmp)); ^ arch/sandbox/cpu/cpu.c:159:20: note: declared here int setjmp(jmp_buf jmp)

with the sizeof changed to sizeof(jmp_buf), I'm getting:

... EFI: Entry efi_exit(00007fffffffc198, 2147483662, 0, 0000000000000000)

Program received signal SIGSEGV, Segmentation fault. os_longjmp (jmp=0x7fffffffc200, jmp@entry=<error reading variable: DWARF-2 expression error: Loop detected (257).>, ret=1, ret@entry=<error reading variable: DWARF-2 expression error: Loop detected (257).>) at ../arch/sandbox/cpu/os.c:627 627 longjmp((struct __jmp_buf_tag *)jmp, ret);

So it seems like something not quite right still..

Hmm I'm not sure what, yet.

Regards, Simon

On 09/18/2017 12:59 AM, Simon Glass wrote:

Add these so that we can build the EFI loader for sandbox. The values are for x86_64 so potentially bogus. But we don't support relocation within sandbox anyway.

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

lib/efi_loader/efi_runtime.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index ad7f3754bd..8ad1d2fc99 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -48,6 +48,9 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void); #include <asm/elf.h> #define R_RELATIVE R_386_RELATIVE #define R_MASK 0xffULL +#elif defined(CONFIG_SANDBOX)

For the sandbox you should make your choice based on the host architecture. I want to be able to work on my armv8 machine.

+#define R_RELATIVE 8

You mean R_X86_64_RELATIVE (defined in arch/x86/include/asm/elf.h)?

Best regards

Heinrich

+#define R_MASK 0xffffffffULL #else #error Need to add relocation awareness #endif

Hi Heinrich,

On 18 September 2017 at 05:02, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 09/18/2017 12:59 AM, 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:

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

cmd/bootefi.c | 18 ++++++++++++++++++ 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, 50 insertions(+) create mode 100644 lib/efi_loader/efi_test.c

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index ee07733e3e..f499103d23 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -323,6 +323,24 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) memcpy((char *)addr, __efi_hello_world_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;
  

•         loaded_image_info.device_handle = bootefi_device_path;
  

•         loaded_image_info.file_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")) { /* diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 72600afea8..ab63f639de 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -196,3 +196,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 79d2dad22c..43919137b0 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -263,6 +263,9 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle, struct efi_system_table *systab); #endif

+/* 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 30bf343a36..69eb93518d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -21,3 +21,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;
  

+}

Hello Simon,

why do we need this patch? You could just call bootefi selftest to demonstrate the same (after fixing the memory map initialization for the sandbox).

Yes, but I'd like to use sandbox for testing where possible.

Could you, please, review the framework that I have setup for bootefi selftest. Does the design make sense to you?

Yes it looks good, now reviewed. I think we need both:

- sandbox test for ease of use (but will not provide 100% coverage, e.g. I'm not sure we can implement exit boot services) - selftest to actually test the API calls properly (should be able to test nearly everything here)

Regards, Simon

On Mon, Sep 18, 2017 at 9:18 AM, Rob Clark robdclark@gmail.com wrote:

On Sun, Sep 17, 2017 at 11:48 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/18/2017 12:59 AM, Simon Glass wrote:

...

A limitation of the EFI loader at present is that it does not build with sandbox. This makes it hard to write tests, since sandbox is used for most testing in U-Boot.

This series enables the EFI loader feature. It allows sandbox to build and run a trivial function which calls the EFI API to output a message.

Much work remains but this should serve as a basis for adding tests more easily for EFI loader.

This series sits on top of Heinrich's recent EFI test series. It is available at u-boot-dm/efi-working

Simon Glass (16): efi: Update efi_smbios_register() to return error code efi: Move the init check inside efi_init_obj_list() efi: Add error checking for efi_init_obj_list() efi: Add a TODO to efi_init_obj_list() efi: Correct header order in efi_memory efi: sandbox: Adjust memory setup for sandbox sandbox: smbios: Update to support sandbox sandbox: Add a setjmp() implementation efi: sandbox: Add required linker sections efi: sandbox: Add distroboot support Define board_quiesce_devices() in a shared location Add a comment for board_quiesce_devices() efi: sandbox: Add relocation constants efi: Add a comment about duplicated ELF constants efi: sandbox: Enable EFI loader builder for sandbox efi: sandbox: Add a simple 'bootefi test' command

arch/arm/include/asm/u-boot-arm.h | 1 - arch/sandbox/cpu/cpu.c | 13 ++++++++++ arch/sandbox/cpu/os.c | 17 ++++++++++++ arch/sandbox/cpu/u-boot.lds | 29 +++++++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++ arch/sandbox/lib/Makefile | 2 +- arch/sandbox/lib/sections.c | 12 +++++++++ arch/x86/include/asm/u-boot-x86.h | 1 - arch/x86/lib/bootm.c | 4 --- cmd/bootefi.c | 54 ++++++++++++++++++++++++++++++++++----- common/bootm.c | 4 +++ configs/sandbox_defconfig | 1 + include/bootm.h | 8 ++++++ include/config_distro_bootcmd.h | 2 +- include/efi_loader.h | 13 ++++++++-- include/os.h | 21 +++++++++++++++ lib/efi_loader/Kconfig | 12 ++++++++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_boottime.c | 4 +++ lib/efi_loader/efi_memory.c | 33 +++++++++++++----------- lib/efi_loader/efi_runtime.c | 7 +++++ lib/efi_loader/efi_smbios.c | 6 +++-- lib/efi_loader/efi_test.c | 17 ++++++++++++ lib/smbios.c | 38 ++++++++++++++++++++------- 24 files changed, 277 insertions(+), 44 deletions(-) create mode 100644 arch/sandbox/include/asm/setjmp.h create mode 100644 arch/sandbox/lib/sections.c create mode 100644 lib/efi_loader/efi_test.c

Thanks for enabling efi_loader on sandbox. That will make many things easier.

Unfortunately efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle, struct efi_system_table *systab) { ... boottime = systable->boottime; ... ret = boottime->allocate_pool(EFI_BOOT_SERVICES_DATA, map_size, (void **)&memory_map); leads to a segmentation fault:

I'm seeing something similar, because:

(gdb) print gd->bd->bi_dram[0] $2 = {start = 0, size = 134217728}

u-boot expects 1:1 phys:virt mapping, so that probably won't work.

The following quick hack works.. something similar could probably be smashed in to ""

-------- diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index cddafe2d43..da2079a4b1 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -459,9 +459,10 @@ int efi_memory_init(void) unsigned long uboot_start, uboot_pages; unsigned long uboot_stack_size = 16 * 1024 * 1024;

- efi_add_known_memory();

if (!IS_ENABLED(CONFIG_SANDBOX)) { + efi_add_known_memory(); + /* Add U-Boot */ uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; @@ -476,6 +477,12 @@ int efi_memory_init(void) runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map(runtime_start, runtime_pages, EFI_RUNTIME_SERVICES_CODE, false); + } else { +#define SZ_256M 0x10000000 + size_t sz = SZ_256M; + void *ram = os_malloc(sz); + efi_add_memory_map((uintptr_t)ram, sz >> EFI_PAGE_SHIFT, + EFI_CONVENTIONAL_MEMORY, false); }

#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER --------

With that I'm at least getting further.. efi_allocate_pool() eventually fails, possibly making every small memory allocation page aligned means that 256m isn't enough..

BR, -R

...

=> bootefi selftest

Testing EFI API implementation

Number of tests to execute: 3

<snip> Setting up 'ExitBootServices' Setting up 'ExitBootServices' succeeded Segmentation fault user@workstation:~/workspace/u-boot-odroid-c2/denx$

The problem does not exist with qemu-x86_defconfig without your patches.

fwiw, qemu-x86 still works for me (I can still load Shell.efi) with these patches..

BR, -R

...

qemu-x86_defconfig cannot be built with you patches:

UPD include/generated/asm-offsets.h sh: echo: I/O error Kbuild:47: recipe for target 'include/generated/generic-asm-offsets.h' failed make[1]: *** [include/generated/generic-asm-offsets.h] Error 1 make[1]: *** Waiting for unfinished jobs.... Makefile:1332: recipe for target 'prepare0' failed make: *** [prepare0] Error 2

Best regards

Heinrich

On Mon, Sep 18, 2017 at 10:30 AM, Rob Clark robdclark@gmail.com wrote:

On Mon, Sep 18, 2017 at 9:31 AM, Rob Clark robdclark@gmail.com wrote:

...

On Mon, Sep 18, 2017 at 9:18 AM, Rob Clark robdclark@gmail.com wrote:

...

On Sun, Sep 17, 2017 at 11:48 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/18/2017 12:59 AM, Simon Glass wrote:

...

A limitation of the EFI loader at present is that it does not build with sandbox. This makes it hard to write tests, since sandbox is used for most testing in U-Boot.

This series enables the EFI loader feature. It allows sandbox to build and run a trivial function which calls the EFI API to output a message.

Much work remains but this should serve as a basis for adding tests more easily for EFI loader.

This series sits on top of Heinrich's recent EFI test series. It is available at u-boot-dm/efi-working

Simon Glass (16): efi: Update efi_smbios_register() to return error code efi: Move the init check inside efi_init_obj_list() efi: Add error checking for efi_init_obj_list() efi: Add a TODO to efi_init_obj_list() efi: Correct header order in efi_memory efi: sandbox: Adjust memory setup for sandbox sandbox: smbios: Update to support sandbox sandbox: Add a setjmp() implementation efi: sandbox: Add required linker sections efi: sandbox: Add distroboot support Define board_quiesce_devices() in a shared location Add a comment for board_quiesce_devices() efi: sandbox: Add relocation constants efi: Add a comment about duplicated ELF constants efi: sandbox: Enable EFI loader builder for sandbox efi: sandbox: Add a simple 'bootefi test' command

arch/arm/include/asm/u-boot-arm.h | 1 - arch/sandbox/cpu/cpu.c | 13 ++++++++++ arch/sandbox/cpu/os.c | 17 ++++++++++++ arch/sandbox/cpu/u-boot.lds | 29 +++++++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++ arch/sandbox/lib/Makefile | 2 +- arch/sandbox/lib/sections.c | 12 +++++++++ arch/x86/include/asm/u-boot-x86.h | 1 - arch/x86/lib/bootm.c | 4 --- cmd/bootefi.c | 54 ++++++++++++++++++++++++++++++++++----- common/bootm.c | 4 +++ configs/sandbox_defconfig | 1 + include/bootm.h | 8 ++++++ include/config_distro_bootcmd.h | 2 +- include/efi_loader.h | 13 ++++++++-- include/os.h | 21 +++++++++++++++ lib/efi_loader/Kconfig | 12 ++++++++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_boottime.c | 4 +++ lib/efi_loader/efi_memory.c | 33 +++++++++++++----------- lib/efi_loader/efi_runtime.c | 7 +++++ lib/efi_loader/efi_smbios.c | 6 +++-- lib/efi_loader/efi_test.c | 17 ++++++++++++ lib/smbios.c | 38 ++++++++++++++++++++------- 24 files changed, 277 insertions(+), 44 deletions(-) create mode 100644 arch/sandbox/include/asm/setjmp.h create mode 100644 arch/sandbox/lib/sections.c create mode 100644 lib/efi_loader/efi_test.c

Thanks for enabling efi_loader on sandbox. That will make many things easier.

Unfortunately efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle, struct efi_system_table *systab) { ... boottime = systable->boottime; ... ret = boottime->allocate_pool(EFI_BOOT_SERVICES_DATA, map_size, (void **)&memory_map); leads to a segmentation fault:

I'm seeing something similar, because:

(gdb) print gd->bd->bi_dram[0] $2 = {start = 0, size = 134217728}

u-boot expects 1:1 phys:virt mapping, so that probably won't work.

The following quick hack works.. something similar could probably be smashed in to ""

---

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index cddafe2d43..da2079a4b1 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -459,9 +459,10 @@ int efi_memory_init(void) unsigned long uboot_start, uboot_pages; unsigned long uboot_stack_size = 16 * 1024 * 1024;

• efi_add_known_memory();

  if (!IS_ENABLED(CONFIG_SANDBOX)) {

•    efi_add_known_memory();
  

•    /* Add U-Boot */
     uboot_start = (gd->start_addr_sp - uboot_stack_size) &
             ~EFI_PAGE_MASK;
  

@@ -476,6 +477,12 @@ int efi_memory_init(void) runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map(runtime_start, runtime_pages, EFI_RUNTIME_SERVICES_CODE, false);

• } else {

+#define SZ_256M 0x10000000

•    size_t sz = SZ_256M;
  

•    void *ram = os_malloc(sz);
  

•    efi_add_memory_map((uintptr_t)ram, sz >> EFI_PAGE_SHIFT,
  

•               EFI_CONVENTIONAL_MEMORY, false);
  

  }

#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER

With that I'm at least getting further.. efi_allocate_pool() eventually fails, possibly making every small memory allocation page aligned means that 256m isn't enough..

Ok, still just as hacky, but works a bit better:

---

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index cddafe2d43..b546b5e35d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -14,6 +14,7 @@ #include <linux/list_sort.h> #include <inttypes.h> #include <watchdog.h> +#include <os.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -459,9 +460,9 @@ int efi_memory_init(void) unsigned long uboot_start, uboot_pages; unsigned long uboot_stack_size = 16 * 1024 * 1024;

• efi_add_known_memory();
• if (!IS_ENABLED(CONFIG_SANDBOX)) {

•    efi_add_known_memory();
  

•    /* Add U-Boot */
     uboot_start = (gd->start_addr_sp - uboot_stack_size) &
             ~EFI_PAGE_MASK;
  

@@ -476,6 +477,14 @@ int efi_memory_init(void) runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map(runtime_start, runtime_pages, EFI_RUNTIME_SERVICES_CODE, false);

• } else {

+#define SZ_4K 0x00001000 +#define SZ_256M 0x10000000

•    size_t sz = SZ_256M;
  

•    uintptr_t ram = (uintptr_t)os_malloc(sz + SZ_4K) + SZ_4K;
  

•    efi_add_memory_map(ram & ~EFI_PAGE_MASK, sz >> EFI_PAGE_SHIFT,
  

•               EFI_CONVENTIONAL_MEMORY, false);
  

•    gd->start_addr_sp = ~0;
  

  }

#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER

At this point it crashes in efi_load_pe() when it first tries to dereference the address of the image passed in, ie. I'm running:

host bind 0 x86_64-sct.img load host 0:1 0x01000000 /efi/boot/shell.efi bootefi 0x01000000

Not sure if there is a better way to pick an address to load into. Or maybe just assuming that PA==VA isn't a good idea in sandbox?

Ok, I realized there is map_sysmem().. which gets me further.. efi_loader really expects identity mapping (PA==VA), and iirc this is what UEFI spec expects too so I wouldn't necessarily call it a bug in efi_loader.

BR, -R

Hi Rob,

On 18 September 2017 at 11:03, Rob Clark robdclark@gmail.com wrote:

On Mon, Sep 18, 2017 at 11:07 AM, Rob Clark robdclark@gmail.com wrote:

...

On Mon, Sep 18, 2017 at 10:30 AM, Rob Clark robdclark@gmail.com wrote:

...

On Mon, Sep 18, 2017 at 9:31 AM, Rob Clark robdclark@gmail.com wrote:

...

On Mon, Sep 18, 2017 at 9:18 AM, Rob Clark robdclark@gmail.com wrote:

...

On Sun, Sep 17, 2017 at 11:48 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/18/2017 12:59 AM, Simon Glass wrote: > A limitation of the EFI loader at present is that it does not build with > sandbox. This makes it hard to write tests, since sandbox is used for most > testing in U-Boot. > > This series enables the EFI loader feature. It allows sandbox to build and > run a trivial function which calls the EFI API to output a message. > > Much work remains but this should serve as a basis for adding tests more > easily for EFI loader. > > This series sits on top of Heinrich's recent EFI test series. It is > available at u-boot-dm/efi-working > > > Simon Glass (16): > efi: Update efi_smbios_register() to return error code > efi: Move the init check inside efi_init_obj_list() > efi: Add error checking for efi_init_obj_list() > efi: Add a TODO to efi_init_obj_list() > efi: Correct header order in efi_memory > efi: sandbox: Adjust memory setup for sandbox > sandbox: smbios: Update to support sandbox > sandbox: Add a setjmp() implementation > efi: sandbox: Add required linker sections > efi: sandbox: Add distroboot support > Define board_quiesce_devices() in a shared location > Add a comment for board_quiesce_devices() > efi: sandbox: Add relocation constants > efi: Add a comment about duplicated ELF constants > efi: sandbox: Enable EFI loader builder for sandbox > efi: sandbox: Add a simple 'bootefi test' command > > arch/arm/include/asm/u-boot-arm.h | 1 - > arch/sandbox/cpu/cpu.c | 13 ++++++++++ > arch/sandbox/cpu/os.c | 17 ++++++++++++ > arch/sandbox/cpu/u-boot.lds | 29 +++++++++++++++++++++ > arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++ > arch/sandbox/lib/Makefile | 2 +- > arch/sandbox/lib/sections.c | 12 +++++++++ > arch/x86/include/asm/u-boot-x86.h | 1 - > arch/x86/lib/bootm.c | 4 --- > cmd/bootefi.c | 54 ++++++++++++++++++++++++++++++++++----- > common/bootm.c | 4 +++ > configs/sandbox_defconfig | 1 + > include/bootm.h | 8 ++++++ > include/config_distro_bootcmd.h | 2 +- > include/efi_loader.h | 13 ++++++++-- > include/os.h | 21 +++++++++++++++ > lib/efi_loader/Kconfig | 12 ++++++++- > lib/efi_loader/Makefile | 1 + > lib/efi_loader/efi_boottime.c | 4 +++ > lib/efi_loader/efi_memory.c | 33 +++++++++++++----------- > lib/efi_loader/efi_runtime.c | 7 +++++ > lib/efi_loader/efi_smbios.c | 6 +++-- > lib/efi_loader/efi_test.c | 17 ++++++++++++ > lib/smbios.c | 38 ++++++++++++++++++++------- > 24 files changed, 277 insertions(+), 44 deletions(-) > create mode 100644 arch/sandbox/include/asm/setjmp.h > create mode 100644 arch/sandbox/lib/sections.c > create mode 100644 lib/efi_loader/efi_test.c > Thanks for enabling efi_loader on sandbox. That will make many things easier.

Unfortunately efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle, struct efi_system_table *systab) { ... boottime = systable->boottime; ... ret = boottime->allocate_pool(EFI_BOOT_SERVICES_DATA, map_size, (void **)&memory_map); leads to a segmentation fault:

I'm seeing something similar, because:

(gdb) print gd->bd->bi_dram[0] $2 = {start = 0, size = 134217728}

u-boot expects 1:1 phys:virt mapping, so that probably won't work.

The following quick hack works.. something similar could probably be smashed in to ""

---

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index cddafe2d43..da2079a4b1 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -459,9 +459,10 @@ int efi_memory_init(void) unsigned long uboot_start, uboot_pages; unsigned long uboot_stack_size = 16 * 1024 * 1024;

• efi_add_known_memory();

  if (!IS_ENABLED(CONFIG_SANDBOX)) {

•    efi_add_known_memory();
  

•    /* Add U-Boot */
     uboot_start = (gd->start_addr_sp - uboot_stack_size) &
             ~EFI_PAGE_MASK;
  

@@ -476,6 +477,12 @@ int efi_memory_init(void) runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map(runtime_start, runtime_pages, EFI_RUNTIME_SERVICES_CODE, false);

• } else {

+#define SZ_256M 0x10000000

•    size_t sz = SZ_256M;
  

•    void *ram = os_malloc(sz);
  

•    efi_add_memory_map((uintptr_t)ram, sz >> EFI_PAGE_SHIFT,
  

•               EFI_CONVENTIONAL_MEMORY, false);
  

  }

#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER

With that I'm at least getting further.. efi_allocate_pool() eventually fails, possibly making every small memory allocation page aligned means that 256m isn't enough..

Ok, still just as hacky, but works a bit better:

---

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index cddafe2d43..b546b5e35d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -14,6 +14,7 @@ #include <linux/list_sort.h> #include <inttypes.h> #include <watchdog.h> +#include <os.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -459,9 +460,9 @@ int efi_memory_init(void) unsigned long uboot_start, uboot_pages; unsigned long uboot_stack_size = 16 * 1024 * 1024;

• efi_add_known_memory();
• if (!IS_ENABLED(CONFIG_SANDBOX)) {

•    efi_add_known_memory();
  

•    /* Add U-Boot */
     uboot_start = (gd->start_addr_sp - uboot_stack_size) &
             ~EFI_PAGE_MASK;
  

@@ -476,6 +477,14 @@ int efi_memory_init(void) runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map(runtime_start, runtime_pages, EFI_RUNTIME_SERVICES_CODE, false);

• } else {

+#define SZ_4K 0x00001000 +#define SZ_256M 0x10000000

•    size_t sz = SZ_256M;
  

•    uintptr_t ram = (uintptr_t)os_malloc(sz + SZ_4K) + SZ_4K;
  

•    efi_add_memory_map(ram & ~EFI_PAGE_MASK, sz >> EFI_PAGE_SHIFT,
  

•               EFI_CONVENTIONAL_MEMORY, false);
  

•    gd->start_addr_sp = ~0;
  

  }

#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER

At this point it crashes in efi_load_pe() when it first tries to dereference the address of the image passed in, ie. I'm running:

host bind 0 x86_64-sct.img load host 0:1 0x01000000 /efi/boot/shell.efi bootefi 0x01000000

Not sure if there is a better way to pick an address to load into. Or maybe just assuming that PA==VA isn't a good idea in sandbox?

Ok, I realized there is map_sysmem().. which gets me further.. efi_loader really expects identity mapping (PA==VA), and iirc this is what UEFI spec expects too so I wouldn't necessarily call it a bug in efi_loader.

So, I don't know x86(_64) asm or calling conventions as well as arm.. but I wonder if we are screwing up something long those lines:

0000000000000280 <.text>: 280: 48 89 5c 24 08 mov %rbx,0x8(%rsp) 285: 57 push %rdi 286: 48 83 ec 20 sub $0x20,%rsp 28a: 48 8b f9 mov %rcx,%rdi

...

...

28d: e8 1e 00 00 00 callq 0x2b0

this jump is taken to 0x2b0

 292:       e8 2d 06 00 00          callq  0x8c4
 297:       48 8b cf                mov    %rdi,%rcx
 29a:       48 8b d8                mov    %rax,%rbx
 29d:       e8 ea 01 00 00          callq  0x48c
 2a2:       48 8b c3                mov    %rbx,%rax
 2a5:       48 8b 5c 24 30          mov    0x30(%rsp),%rbx
 2aa:       48 83 c4 20             add    $0x20,%rsp
 2ae:       5f                      pop    %rdi
 2af:       c3                      retq

...

...

2b0: 40 53 rex push %rbx

 2b2:       48 83 ec 20             sub    $0x20,%rsp
 2b6:       48 89 0d e3 b9 05 00    mov    %rcx,0x5b9e3(%rip)

# 0x5bca0 2bd: 4c 8d 05 f4 b9 05 00 lea 0x5b9f4(%rip),%r8 # 0x5bcb8

...

...

2c4: 48 8b 42 60 mov 0x60(%rdx),%rax

and at 0x2c4 %rdx is 0x2.. I always thought x86 asm syntax strange, but I assume that is trying to write to value of %rdx + offset of 0x60?? But this is a register never written, so I assume it is expected to be passed from efi_loader?

From https://en.wikipedia.org/wiki/X86_calling_conventions it seems that MS calling convention expects 2nd arg in %rdx, but linux/gcc calling convention expects 3rd arg in %rdx (there is no 3rd arg)..

I don't know it well either. But so long as functions are properly declared in header files I don't really understand how we can have a mismatch.

Regards, Simon