[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader
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.
This series is at u-boot-dm/efi-working
Changes in v4: - Fix up the sizeof() operations on jmp_buf - Move the fix to query_console_serial() - Rebase to master - Remove code already applied - Update SPDX tags - Update subject
Changes in v3: - Add comments on aligment - Add new patch to init the 'rows' and 'cols' variables - Add new patch to rename bootefi_test_finish() to bootefi_run_finish() - Add new patch to split out test init/uninit into functions - Add patch to create a function to set up for running EFI code - Drop incorrect map_sysmem() in write_smbios_table() - Init the 'rows' and 'cols' vars to avoid a compiler error (gcc 4.8.4) - Rebase to master - Return error value of efi_allocate_pages() - Update function comment for write_smbios_table()
Changes in v2: - Rebase to master - Update return type of efi_smbios_register() to efi_status_t - Update to use mapmem instead of a cast - Use return value of efi_install_configuration_table
Simon Glass (16): efi: Init the 'rows' and 'cols' variables efi: Update some comments related to smbios tables efi: sandbox: Adjust memory usage 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: Split out test init/uninit into functions efi: sandbox: Add a simple 'bootefi test' command efi: Create a function to set up for running EFI code efi: Rename bootefi_test_finish() to bootefi_run_finish()
arch/arm/include/asm/u-boot-arm.h | 1 - arch/sandbox/cpu/cpu.c | 13 +++ arch/sandbox/cpu/os.c | 23 +++++ arch/sandbox/cpu/u-boot.lds | 29 ++++++ arch/sandbox/include/asm/setjmp.h | 30 ++++++ 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 | 158 +++++++++++++++++++++--------- common/bootm.c | 4 + include/bootm.h | 8 ++ include/config_distro_bootcmd.h | 2 +- include/efi_loader.h | 10 ++ include/os.h | 21 ++++ include/smbios.h | 5 +- lib/efi_loader/Kconfig | 12 ++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_console.c | 5 +- lib/efi_loader/efi_memory.c | 31 +++--- lib/efi_loader/efi_runtime.c | 7 ++ lib/efi_loader/efi_smbios.c | 7 +- lib/efi_loader/efi_test.c | 16 +++ lib/smbios.c | 32 ++++-- 24 files changed, 351 insertions(+), 83 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
The current code causes a compiler error on gcc 4.8.4 as used by sandbox on Ubuntu 14.04, which is fairly recent. Init these variables to fix the problem.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Move the fix to query_console_serial()
Changes in v3: - Add new patch to init the 'rows' and 'cols' variables
Changes in v2: None
lib/efi_loader/efi_console.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d777db8a3ed..001f68df0a7 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -185,8 +185,11 @@ static int query_console_serial(int *rows, int *cols) return -1;
/* Read {depth,rows,cols} */ - if (term_read_reply(n, 3, 't')) + if (term_read_reply(n, 3, 't')) { + *rows = -1; + *cols = -1; return -1; + }
*cols = n[2]; *rows = n[1];
On 05/16/2018 05:42 PM, Simon Glass wrote:
The current code causes a compiler error on gcc 4.8.4 as used by sandbox on Ubuntu 14.04, which is fairly recent. Init these variables to fix the problem.
Signed-off-by: Simon Glass sjg@chromium.org
This patch may silence the compiler but does not solve the bug leading to the possible usage of unintialized values. See https://lists.denx.de/pipermail/u-boot/2018-May/328762.html
Best regards
Heinrich Schuchardt
Changes in v4:
- Move the fix to query_console_serial()
Changes in v3:
- Add new patch to init the 'rows' and 'cols' variables
Changes in v2: None
lib/efi_loader/efi_console.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index d777db8a3ed..001f68df0a7 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -185,8 +185,11 @@ static int query_console_serial(int *rows, int *cols) return -1;
/* Read {depth,rows,cols} */
- if (term_read_reply(n, 3, 't'))
if (term_read_reply(n, 3, 't')) {
*rows = -1;*cols = -1;return -1;
}
*cols = n[2]; *rows = n[1];
Clarify the operation of this code with some additional comments.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Remove code already applied - Update subject
Changes in v3: - Add comments on aligment - Return error value of efi_allocate_pages() - Update function comment for write_smbios_table()
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 | 7 +++++++ include/smbios.h | 5 +++-- lib/efi_loader/efi_smbios.c | 7 ++++++- 3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 2868ca25abb..2519a7c33a7 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -207,6 +207,13 @@ efi_status_t efi_net_register(void); /* Called by bootefi to make the watchdog available */ efi_status_t efi_watchdog_register(void); /* Called by bootefi to make SMBIOS tables available */ +/** + * 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 * diff --git a/include/smbios.h b/include/smbios.h index 79880ef5b5c..97b9ddce237 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -231,8 +231,9 @@ typedef int (*smbios_write_type)(ulong *addr, int handle); * * This writes SMBIOS table at a given address. * - * @addr: start address to write SMBIOS table - * @return: end address of SMBIOS table + * @addr: start address to write SMBIOS table. If this is not + * 16-byte-aligned then it will be aligned before the table is written + * @return: end address of SMBIOS table (and start address for next entry) */ ulong write_smbios_table(ulong addr);
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 482436e2adb..7c3fc8af0b2 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -29,7 +29,12 @@ efi_status_t efi_smbios_register(void) if (ret != EFI_SUCCESS) return ret;
- /* Generate SMBIOS tables */ + /* + * Generate SMBIOS tables - we know that efi_allocate_pages() returns + * a 4k-aligned address, so it is safe to assume that + * write_smbios_table() will write the table at that address. + */ + assert(!(dmi & 0xf)); write_smbios_table(dmi);
/* And expose them to our EFI payload */
On 05/16/2018 05:42 PM, Simon Glass wrote:
Clarify the operation of this code with some additional comments.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Changes in v4:
- Remove code already applied
- Update subject
Changes in v3:
- Add comments on aligment
- Return error value of efi_allocate_pages()
- Update function comment for write_smbios_table()
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 | 7 +++++++ include/smbios.h | 5 +++-- lib/efi_loader/efi_smbios.c | 7 ++++++- 3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 2868ca25abb..2519a7c33a7 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -207,6 +207,13 @@ efi_status_t efi_net_register(void); /* Called by bootefi to make the watchdog available */ efi_status_t efi_watchdog_register(void); /* Called by bootefi to make SMBIOS tables available */ +/**
- 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 * diff --git a/include/smbios.h b/include/smbios.h index 79880ef5b5c..97b9ddce237 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -231,8 +231,9 @@ typedef int (*smbios_write_type)(ulong *addr, int handle);
- This writes SMBIOS table at a given address.
- @addr: start address to write SMBIOS table
- @return: end address of SMBIOS table
- @addr: start address to write SMBIOS table. If this is not
- 16-byte-aligned then it will be aligned before the table is written
*/
- @return: end address of SMBIOS table (and start address for next entry)
ulong write_smbios_table(ulong addr);
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 482436e2adb..7c3fc8af0b2 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -29,7 +29,12 @@ efi_status_t efi_smbios_register(void) if (ret != EFI_SUCCESS) return ret;
- /* Generate SMBIOS tables */
/*
* Generate SMBIOS tables - we know that efi_allocate_pages() returns* a 4k-aligned address, so it is safe to assume that* write_smbios_table() will write the table at that address.*/assert(!(dmi & 0xf)); write_smbios_table(dmi);
/* And expose them to our EFI payload */
Clarify the operation of this code with some additional comments.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Thanks, applied to efi-next
Alex
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 v4: None Changes in v3: None 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 664c651db56..3fbed63728b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,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> @@ -388,7 +389,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) 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; } @@ -499,18 +500,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 */
On 05/16/2018 05:42 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
This patch does not apply to Alex's efi-next repository due to https://github.com/agraf/u-boot/commit/e62d2cd60480a867dd21a102238f5387b8214...
Please, rebase.
Best regards
Heinrich
Changes in v4: None Changes in v3: None 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 664c651db56..3fbed63728b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,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> @@ -388,7 +389,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) 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);@@ -499,18 +500,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 */
On 05/16/2018 05:42 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
running ./u-boot
bootefi selftest Found 0 disks WARNING: booting without device tree
Testing EFI API implementation
Number of tests to execute: 17
Setting up 'block device' Setting up 'block device' succeeded
Executing 'block device' lib/efi_selftest/efi_selftest_block_device.c(378): TODO: Wrong volume label 'xxa1', expected 'U-BOOT TEST' FAT: Misaligned buffer address (00007ff70aafe658) Segmentation fault
Please, fix the alignment fault. You have to ensure that the memory that Sandbox has retrieved via malloc is reduced to 4k aligned pages before being published to the EFI implementation in lib/efi_loader/efi_memory.c
Best regards
Heinrich
Changes in v4: None Changes in v3: None 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 664c651db56..3fbed63728b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,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> @@ -388,7 +389,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) 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);@@ -499,18 +500,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 */
On 05/16/2018 07:15 PM, Heinrich Schuchardt wrote:
On 05/16/2018 05:42 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
running ./u-boot
bootefi selftest Found 0 disks WARNING: booting without device tree
Testing EFI API implementation
Number of tests to execute: 17
Setting up 'block device' Setting up 'block device' succeeded
Executing 'block device' lib/efi_selftest/efi_selftest_block_device.c(378): TODO: Wrong volume label 'xxa1', expected 'U-BOOT TEST' FAT: Misaligned buffer address (00007ff70aafe658) Segmentation fault
Please, fix the alignment fault. You have to ensure that the memory that Sandbox has retrieved via malloc is reduced to 4k aligned pages before being published to the EFI implementation in lib/efi_loader/efi_memory.c
Hello Simon,
couldn't we use mmap() instead of malloc() to allocate the memory used by the Sandbox? This would guarantee page aligned memory. mmap() with MAP_ANON is available both on POSIX and BSD systems.
Best regards
Heinrich
Hi Heinrick,
On 24 May 2018 at 13:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 05/16/2018 07:15 PM, Heinrich Schuchardt wrote:
On 05/16/2018 05:42 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
running ./u-boot
bootefi selftest Found 0 disks WARNING: booting without device tree
Testing EFI API implementation
Number of tests to execute: 17
Setting up 'block device' Setting up 'block device' succeeded
Executing 'block device' lib/efi_selftest/efi_selftest_block_device.c(378): TODO: Wrong volume label 'xxa1', expected 'U-BOOT TEST' FAT: Misaligned buffer address (00007ff70aafe658) Segmentation fault
Please, fix the alignment fault. You have to ensure that the memory that Sandbox has retrieved via malloc is reduced to 4k aligned pages before being published to the EFI implementation in lib/efi_loader/efi_memory.c
Hello Simon,
couldn't we use mmap() instead of malloc() to allocate the memory used by the Sandbox? This would guarantee page aligned memory. mmap() with MAP_ANON is available both on POSIX and BSD systems.
We do use mmap() to allocate U-Boot's memory. I wonder why it is not page-aligned?
See os_malloc() for the implementation. Perhaps it needs another arg added?
Regards, Simon
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 v4: None Changes in v3: - Drop incorrect map_sysmem() in write_smbios_table()
Changes in v2: None
lib/smbios.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index df3d26b0710..fc3dabcbc1b 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -6,6 +6,7 @@ */
#include <common.h> +#include <mapmem.h> #include <smbios.h> #include <tables_csum.h> #include <version.h> @@ -72,9 +73,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"); @@ -101,16 +103,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); @@ -122,15 +126,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); @@ -140,15 +146,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); @@ -160,6 +168,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; } @@ -198,9 +207,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; @@ -214,32 +224,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; } @@ -268,7 +283,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); @@ -297,6 +312,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 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
Can't we just simply make sandbox behave like any other target instead?
Alex
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Regards, Simon
On 25.05.18 04:42, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Not if we write a smart enough linker script. I can try to see when I get around to give you an example. But basically all we need to do is reserve a section for guest ram at a constant virtual address.
Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Most platforms don't have RAM start at 0x0 (and making sure nobody assumes it does start at 0 is a good thing). The only bit we need to make sure is that it always starts at *the same* address on every invocation. But if that address is 256MB, things should still be fine.
Alex
Hi Alex,
On 3 June 2018 at 04:13, Alexander Graf agraf@suse.de wrote:
On 25.05.18 04:42, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Not if we write a smart enough linker script. I can try to see when I get around to give you an example. But basically all we need to do is reserve a section for guest ram at a constant virtual address.
Yes, but ideally that would be 0, or something small.
Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Most platforms don't have RAM start at 0x0 (and making sure nobody assumes it does start at 0 is a good thing). The only bit we need to make sure is that it always starts at *the same* address on every invocation. But if that address is 256MB, things should still be fine.
Yes but putting a 10000000 base address on everything is a bit of a pain.
Regards, Simon
On 07.06.18 22:25, Simon Glass wrote:
Hi Alex,
On 3 June 2018 at 04:13, Alexander Graf agraf@suse.de wrote:
On 25.05.18 04:42, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Not if we write a smart enough linker script. I can try to see when I get around to give you an example. But basically all we need to do is reserve a section for guest ram at a constant virtual address.
Yes, but ideally that would be 0, or something small.
You can't do 0 because 0 is protected on a good number of OSs. And if it can't be 0, better use something that makes pointers easy to read.
Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Most platforms don't have RAM start at 0x0 (and making sure nobody assumes it does start at 0 is a good thing). The only bit we need to make sure is that it always starts at *the same* address on every invocation. But if that address is 256MB, things should still be fine.
Yes but putting a 10000000 base address on everything is a bit of a pain.
Why? It's what we do on arm systems that have ram starting at higher offsets already.
Alex
Hi Alex,
On 7 June 2018 at 12:36, Alexander Graf agraf@suse.de wrote:
On 07.06.18 22:25, Simon Glass wrote:
Hi Alex,
On 3 June 2018 at 04:13, Alexander Graf agraf@suse.de wrote:
On 25.05.18 04:42, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Not if we write a smart enough linker script. I can try to see when I get around to give you an example. But basically all we need to do is reserve a section for guest ram at a constant virtual address.
Yes, but ideally that would be 0, or something small.
You can't do 0 because 0 is protected on a good number of OSs. And if it can't be 0, better use something that makes pointers easy to read.
Yes this is one reason for map_sysmem().
Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Most platforms don't have RAM start at 0x0 (and making sure nobody assumes it does start at 0 is a good thing). The only bit we need to make sure is that it always starts at *the same* address on every invocation. But if that address is 256MB, things should still be fine.
Yes but putting a 10000000 base address on everything is a bit of a pain.
Why? It's what we do on arm systems that have ram starting at higher offsets already.
It's a pain because you have to type 1 and 5-6 zeroes before you can get to the address you want. Otherwise sandbox just segfaults, which is annoying.
Regards, Simon
On 07.06.18 22:41, Simon Glass wrote:
Hi Alex,
On 7 June 2018 at 12:36, Alexander Graf agraf@suse.de wrote:
On 07.06.18 22:25, Simon Glass wrote:
Hi Alex,
On 3 June 2018 at 04:13, Alexander Graf agraf@suse.de wrote:
On 25.05.18 04:42, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
I really dislike the whole fact that you have to call map_sysmem() at all. I don't quite understand the whole point of it either - it just seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Not if we write a smart enough linker script. I can try to see when I get around to give you an example. But basically all we need to do is reserve a section for guest ram at a constant virtual address.
Yes, but ideally that would be 0, or something small.
You can't do 0 because 0 is protected on a good number of OSs. And if it can't be 0, better use something that makes pointers easy to read.
Yes this is one reason for map_sysmem().
Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Most platforms don't have RAM start at 0x0 (and making sure nobody assumes it does start at 0 is a good thing). The only bit we need to make sure is that it always starts at *the same* address on every invocation. But if that address is 256MB, things should still be fine.
Yes but putting a 10000000 base address on everything is a bit of a pain.
Why? It's what we do on arm systems that have ram starting at higher offsets already.
It's a pain because you have to type 1 and 5-6 zeroes before you can get to the address you want. Otherwise sandbox just segfaults, which is annoying.
It's the same as any other device that does not have RAM starting at 0. The benefit of it is that you manage to catch NULL pointer accesses quite easily, which I guess is something you'll want from a testing target.
Also, you shouldn't use hard addresses in the first place. That's why we have $kernel_addr_r and friends. As long as you stick to those, nothing should change for you at all.
Alex
Hi Alex,
On 7 June 2018 at 12:47, Alexander Graf agraf@suse.de wrote:
On 07.06.18 22:41, Simon Glass wrote:
Hi Alex,
On 7 June 2018 at 12:36, Alexander Graf agraf@suse.de wrote:
On 07.06.18 22:25, Simon Glass wrote:
Hi Alex,
On 3 June 2018 at 04:13, Alexander Graf agraf@suse.de wrote:
On 25.05.18 04:42, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:24, Alexander Graf agraf@suse.de wrote: > > > On 16.05.18 17:42, 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 > > I really dislike the whole fact that you have to call map_sysmem() at > all. I don't quite understand the whole point of it either - it just > seems to clutter the code and make it harder to follow.
The purpose is to map U-Boot addresses (e.g. 0x1234) to actual user-space addresses in sandbox (gd->arch.ram_buf + 0x1234).
Otherwise we cannot write tests which use particular addresses, and people have to worry about the host memory layout when using sandbox.
Not if we write a smart enough linker script. I can try to see when I get around to give you an example. But basically all we need to do is reserve a section for guest ram at a constant virtual address.
Yes, but ideally that would be 0, or something small.
You can't do 0 because 0 is protected on a good number of OSs. And if it can't be 0, better use something that makes pointers easy to read.
Yes this is one reason for map_sysmem().
> Can't we just simply make sandbox behave like any other target instead?
Actually that's the goal of the sandbox support. Memory is modelled as a contiguous chunk starting at 0x0, regardless of what the OS actually gives U-Boot in terms of addresses.
Most platforms don't have RAM start at 0x0 (and making sure nobody assumes it does start at 0 is a good thing). The only bit we need to make sure is that it always starts at *the same* address on every invocation. But if that address is 256MB, things should still be fine.
Yes but putting a 10000000 base address on everything is a bit of a pain.
Why? It's what we do on arm systems that have ram starting at higher offsets already.
It's a pain because you have to type 1 and 5-6 zeroes before you can get to the address you want. Otherwise sandbox just segfaults, which is annoying.
It's the same as any other device that does not have RAM starting at 0. The benefit of it is that you manage to catch NULL pointer accesses quite easily, which I guess is something you'll want from a testing target.
You're confusing the U-Boot memory address with the pointer address. If you use NULL in sandbox it will fault. But address 0 is valid.
Also, you shouldn't use hard addresses in the first place. That's why we have $kernel_addr_r and friends. As long as you stick to those, nothing should change for you at all.
See for example test_fit.py where it is very useful to know the addresses we are using.
Of course we can remove this constraint, but it does make things more painful in sandbox. Also IMO using open casts to convert between addresses and pointers is not desirable, as they are not really tagged in any way. With map_sysmem(), etc., they are explicit and obvious.
Regards, Simon
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 ---
Changes in v4: - Fix up the sizeof() operations on jmp_buf - Update SPDX tags
Changes in v3: None Changes in v2: None
arch/sandbox/cpu/cpu.c | 13 +++++++++++++ arch/sandbox/cpu/os.c | 23 +++++++++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++ include/os.h | 21 +++++++++++++++++++++ 4 files changed, 87 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 d4ad020012e..cde0b055a67 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -9,6 +9,7 @@ #include <linux/libfdt.h> #include <os.h> #include <asm/io.h> +#include <asm/setjmp.h> #include <asm/state.h> #include <dm/root.h>
@@ -164,3 +165,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 d76d0211a2d..5839932b005 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> @@ -628,3 +629,25 @@ 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) +{ + jmp_buf dummy; + + /* + * We cannot rely on the struct name that jmp_buf uses, so use a + * local variable here + */ + if (size < sizeof(dummy)) { + 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 00000000000..0fb1a11f234 --- /dev/null +++ b/arch/sandbox/include/asm/setjmp.h @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) 2018 Google, Inc + * Written by Simon Glass sjg@chromium.org + */ + +#ifndef _SETJMP_H_ +#define _SETJMP_H_ + +struct jmp_buf_data { + /* + * We're not sure how long this should be: + * + * amd64: 200 bytes + * arm64: 392 bytes + * armhf: 392 bytes + * + * So allow space for all of those, plus some extra. + * We don't need to worry about 16-byte alignment, since this does not + * run on Windows. + */ + ulong data[128]; +}; + +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 64e89a06c94..c8e0f52d306 100644 --- a/include/os.h +++ b/include/os.h @@ -330,4 +330,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
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
Thanks, applied to efi-next
Alex
On 16.05.18 17:42, 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
Changes in v4:
- Fix up the sizeof() operations on jmp_buf
- Update SPDX tags
Changes in v3: None Changes in v2: None
arch/sandbox/cpu/cpu.c | 13 +++++++++++++ arch/sandbox/cpu/os.c | 23 +++++++++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++ include/os.h | 21 +++++++++++++++++++++ 4 files changed, 87 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 d4ad020012e..cde0b055a67 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -9,6 +9,7 @@ #include <linux/libfdt.h> #include <os.h> #include <asm/io.h> +#include <asm/setjmp.h> #include <asm/state.h> #include <dm/root.h>
@@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
return (count - base_count) / 1000; }
+int setjmp(jmp_buf jmp) +{
- return os_setjmp((ulong *)jmp, sizeof(*jmp));
So, this doesn't work. Function returns increase the stack pointer which means after setjmp() you are not allowed to return until the longjmp occured. The documentation is quite clear about this:
DESCRIPTION setjmp() and longjmp(3) are useful for dealing with errors and interrupts encountered in a low-level subroutine of a program. setjmp() saves the stack context/environment in env for later use by longjmp(3). The stack context will be invalidated if the function which called setjmp() returns.
So we need to find a way to call setjmp() directly from the code point where we want to call it, rather than jump through helper functions, as these break its functionality.
Also, os_longjmp() is broken. It calls longjmp() which however is not the system longjmp, but the U-Boot internal one that again calls os_longjmp. My quick fix was to make it call _longjmp() instead - that at least makes that part work.
Alex
Hi Alex,
On 15 June 2018 at 06:01, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
Changes in v4:
- Fix up the sizeof() operations on jmp_buf
- Update SPDX tags
Changes in v3: None Changes in v2: None
arch/sandbox/cpu/cpu.c | 13 +++++++++++++ arch/sandbox/cpu/os.c | 23 +++++++++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++ include/os.h | 21 +++++++++++++++++++++ 4 files changed, 87 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 d4ad020012e..cde0b055a67 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -9,6 +9,7 @@ #include <linux/libfdt.h> #include <os.h> #include <asm/io.h> +#include <asm/setjmp.h> #include <asm/state.h> #include <dm/root.h>
@@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
return (count - base_count) / 1000;}
+int setjmp(jmp_buf jmp) +{
return os_setjmp((ulong *)jmp, sizeof(*jmp));So, this doesn't work. Function returns increase the stack pointer which means after setjmp() you are not allowed to return until the longjmp occured. The documentation is quite clear about this:
DESCRIPTION setjmp() and longjmp(3) are useful for dealing with errors and interrupts encountered in a low-level subroutine of a program. setjmp() saves the stack context/environment in env for later use by longjmp(3). The stack context will be invalidated if the function which called setjmp() returns.
So we need to find a way to call setjmp() directly from the code point where we want to call it, rather than jump through helper functions, as these break its functionality.
That sounds hard but perhaps we can do something with inline functions? It's hard because we want to call the C library in the sandbox case. Perhaps we should rename the U-Boot version of the function.
I wonder if in my test I just relied on hello world returning, rather than calling the exit() function? BTW we need to find a way for efi_selftest() to exit cleanly too. At the moment it resets.
Also, os_longjmp() is broken. It calls longjmp() which however is not the system longjmp, but the U-Boot internal one that again calls os_longjmp. My quick fix was to make it call _longjmp() instead - that at least makes that part work.
But it hangs after that?
Regards, Simon
Am 15.06.2018 um 17:16 schrieb Simon Glass sjg@chromium.org:
Hi Alex,
On 15 June 2018 at 06:01, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
Changes in v4:
- Fix up the sizeof() operations on jmp_buf
- Update SPDX tags
Changes in v3: None Changes in v2: None
arch/sandbox/cpu/cpu.c | 13 +++++++++++++ arch/sandbox/cpu/os.c | 23 +++++++++++++++++++++++ arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++ include/os.h | 21 +++++++++++++++++++++ 4 files changed, 87 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 d4ad020012e..cde0b055a67 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -9,6 +9,7 @@ #include <linux/libfdt.h> #include <os.h> #include <asm/io.h> +#include <asm/setjmp.h> #include <asm/state.h> #include <dm/root.h>
@@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
return (count - base_count) / 1000;}
+int setjmp(jmp_buf jmp) +{
return os_setjmp((ulong *)jmp, sizeof(*jmp));So, this doesn't work. Function returns increase the stack pointer which means after setjmp() you are not allowed to return until the longjmp occured. The documentation is quite clear about this:
DESCRIPTION setjmp() and longjmp(3) are useful for dealing with errors and interrupts encountered in a low-level subroutine of a program. setjmp() saves the stack context/environment in env for later use by longjmp(3). The stack context will be invalidated if the function which called setjmp() returns.
So we need to find a way to call setjmp() directly from the code point where we want to call it, rather than jump through helper functions, as these break its functionality.
That sounds hard but perhaps we can do something with inline functions? It's hard because we want to call the C library in the sandbox case. Perhaps we should rename the U-Boot version of the function.
I wonder if in my test I just relied on hello world returning, rather than calling the exit() function? BTW we need to find a way for efi_selftest() to exit cleanly too. At the moment it resets.
Also, os_longjmp() is broken. It calls longjmp() which however is not the system longjmp, but the U-Boot internal one that again calls os_longjmp. My quick fix was to make it call _longjmp() instead - that at least makes that part work.
But it hangs after that?
It hangs in the return path of setjmp afterwards because of the clobbered stack. See my patch in the latest series for reference. With that patch it works (might be glibc only though).
Alex
The EFI loader code requires certain linker sections to exist. Add these for sandbox so that the EFI loader code will link.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
arch/sandbox/cpu/u-boot.lds | 29 +++++++++++++++++++++++++++++ arch/sandbox/lib/Makefile | 2 +- arch/sandbox/lib/sections.c | 12 ++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 arch/sandbox/lib/sections.c
diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds index f97abdfa050..3a6cf55eb99 100644 --- a/arch/sandbox/cpu/u-boot.lds +++ b/arch/sandbox/cpu/u-boot.lds @@ -18,6 +18,35 @@ SECTIONS __u_boot_sandbox_option_end = .;
__bss_start = .; + + .__efi_runtime_start : { + *(.__efi_runtime_start) + } + + .efi_runtime : { + *(efi_runtime_text) + *(efi_runtime_data) + } + + .__efi_runtime_stop : { + *(.__efi_runtime_stop) + } + + .efi_runtime_rel_start : + { + *(.__efi_runtime_rel_start) + } + + .efi_runtime_rel : { + *(.relefi_runtime_text) + *(.relefi_runtime_data) + } + + .efi_runtime_rel_stop : + { + *(.__efi_runtime_rel_stop) + } + }
INSERT BEFORE .data; diff --git a/arch/sandbox/lib/Makefile b/arch/sandbox/lib/Makefile index 52eef2e662e..b4ff717e784 100644 --- a/arch/sandbox/lib/Makefile +++ b/arch/sandbox/lib/Makefile @@ -5,7 +5,7 @@ # (C) Copyright 2002-2006 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
-obj-y += interrupts.o +obj-y += interrupts.o sections.o obj-$(CONFIG_PCI) += pci_io.o obj-$(CONFIG_CMD_BOOTM) += bootm.o obj-$(CONFIG_CMD_BOOTZ) += bootm.o diff --git a/arch/sandbox/lib/sections.c b/arch/sandbox/lib/sections.c new file mode 100644 index 00000000000..697a8167ddf --- /dev/null +++ b/arch/sandbox/lib/sections.c @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2013 Albert ARIBAUD albert.u.boot@aribaud.net + * + */ + +char __efi_runtime_start[0] __attribute__((section(".__efi_runtime_start"))); +char __efi_runtime_stop[0] __attribute__((section(".__efi_runtime_stop"))); +char __efi_runtime_rel_start[0] + __attribute__((section(".__efi_runtime_rel_start"))); +char __efi_runtime_rel_stop[0] + __attribute__((section(".__efi_runtime_rel_stop")));
The EFI loader code requires certain linker sections to exist. Add these for sandbox so that the EFI loader code will link.
Signed-off-by: Simon Glass sjg@chromium.org
Thanks, applied to efi-next
Alex
With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None 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 8d5feb3ac77..97d6baab4be 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -246,7 +246,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 16.05.18 17:42, Simon Glass wrote:
With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3: None 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 8d5feb3ac77..97d6baab4be 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -246,7 +246,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)
Why not
#elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) && defined(__x86_64__))
and similar for other architectures? That way we should be quite safe in determining our target architecture, no?
Alex
/* Always assume we're running 64bit */ #define BOOTENV_EFI_PXE_ARCH "0x7" #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
Hi Alex,
On 24 May 2018 at 06:32, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, Simon Glass wrote:
With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3: None 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 8d5feb3ac77..97d6baab4be 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -246,7 +246,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)
Why not
#elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) && defined(__x86_64__))
and similar for other architectures? That way we should be quite safe in determining our target architecture, no?
I suspect that would work, although I think it would need to be done centrally, rather than ad-hoc in files that need to know the sandbox host architecture.
We are not currently aware of the sandbox host architecture, but I wonder whether we are going to have to teach the build system about it. Does U-Boot sandbox actually run on ARM platforms? I have not tried it.
Regards, Simon
On 12.06.18 07:27, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:32, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, Simon Glass wrote:
With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3: None 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 8d5feb3ac77..97d6baab4be 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -246,7 +246,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)
Why not
#elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) && defined(__x86_64__))
and similar for other architectures? That way we should be quite safe in determining our target architecture, no?
I suspect that would work, although I think it would need to be done centrally, rather than ad-hoc in files that need to know the sandbox host architecture.
We are not currently aware of the sandbox host architecture, but I wonder whether we are going to have to teach the build system about it. Does U-Boot sandbox actually run on ARM platforms? I have not tried it.
I haven't tried it either, but IMHO it's a bug if it doesn't run :).
I also don't quite understand why CONFIG_SANDBOX contradicts CONFIG_X86. They probably should both be set for an x86 host system. That way you wouldn't have to double-check these conditionals all over the code base.
Alex
Hi Alex,
On 11 June 2018 at 23:42, Alexander Graf agraf@suse.de wrote:
On 12.06.18 07:27, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:32, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, Simon Glass wrote:
With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3: None 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 8d5feb3ac77..97d6baab4be 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -246,7 +246,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)
Why not
#elif defined(CONFIG_X86) || (defined(CONFIG_SANDBOX) && defined(__x86_64__))
and similar for other architectures? That way we should be quite safe in determining our target architecture, no?
I suspect that would work, although I think it would need to be done centrally, rather than ad-hoc in files that need to know the sandbox host architecture.
We are not currently aware of the sandbox host architecture, but I wonder whether we are going to have to teach the build system about it. Does U-Boot sandbox actually run on ARM platforms? I have not tried it.
I haven't tried it either, but IMHO it's a bug if it doesn't run :).
Well until someone tests it, it doesn't work. Any changes made to attempt to make it work without testing are just conjecture, IMO.
I also don't quite understand why CONFIG_SANDBOX contradicts CONFIG_X86. They probably should both be set for an x86 host system. That way you wouldn't have to double-check these conditionals all over the code base.
The point here is that they are separate U-Boot architectures.
What I'm getting at is that in U-Boot we have the concept of a host architecture and we can use HOSTCC. But with sandbox this takes on a lot more meaning, since the host architecture is actually the one on which sandbox is running.
So you could say that we have ARM sandbox or x86_64 sandbox. At present we have not such concept. I can see that it could be useful as things get more complicated.
I suppose whoever tries U-Boot sandbox out on ARM first will hit there problems and send a patch?
Regards, Simon
This undocumented function relies on arch-specific code to declare a nop 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 v4: None Changes in v3: None 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 01c3ba87c3f..cc828c45045 100644 --- a/arch/arm/include/asm/u-boot-arm.h +++ b/arch/arm/include/asm/u-boot-arm.h @@ -37,7 +37,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 9be45846a53..2340ef83323 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -84,7 +84,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 533ba075bb1..54c22fe6de3 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -27,10 +27,6 @@ DECLARE_GLOBAL_DATA_PTR;
#define COMMAND_LINE_OFFSET 0x9000
-__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 a0ffc1cd679..e789f6818aa 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -46,6 +46,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 9e42e179878..9f7956d2e35 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -72,4 +72,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
This undocumented function relies on arch-specific code to declare a nop 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
Thanks, applied to efi-next
Alex
This exported function should have a comment describing what it does. Also it should really be removed in favour of device_remove(), which handles this sort of thing now. Add a comment with a TODO.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
include/bootm.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/bootm.h b/include/bootm.h index 9f7956d2e35..0501414e0dc 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -72,6 +72,12 @@ 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);
+/* + * boards should define this to disable devices when EFI exits from boot + * services. + * + * TODO(sjg@chromium.org>): Update this to use driver model's device_remove(). + */ void board_quiesce_devices(void);
#endif
This exported function should have a comment describing what it does. Also it should really be removed in favour of device_remove(), which handles this sort of thing now. Add a comment with a TODO.
Signed-off-by: Simon Glass sjg@chromium.org
Thanks, applied to efi-next
Alex
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 ---
Changes in v4: None Changes in v3: None Changes in v2: None
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 52f1301d75b..ac02f64d967 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -47,6 +47,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) +#define R_RELATIVE 8 +#define R_MASK 0xffffffffULL #else #error Need to add relocation awareness #endif
On 16.05.18 17:42, 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
Changes in v4: None Changes in v3: None Changes in v2: None
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 52f1301d75b..ac02f64d967 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -47,6 +47,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)
Same comment applies here, just change the ifdef above to match on defined(__x86_64__) && defined(CONFIG_SANDBOX)
Alex
+#define R_RELATIVE 8 +#define R_MASK 0xffffffffULL #else #error Need to add relocation awareness #endif
Hi Alex,
On 24 May 2018 at 06:34, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
Changes in v4: None Changes in v3: None Changes in v2: None
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 52f1301d75b..ac02f64d967 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -47,6 +47,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)
Same comment applies here, just change the ifdef above to match on defined(__x86_64__) && defined(CONFIG_SANDBOX)
Yes, understood, same comment as on the other patch. We can always add support for ARM, etc. when people can try it and test it.
Regards, Simon
On 12.06.18 07:27, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:34, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
Changes in v4: None Changes in v3: None Changes in v2: None
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 52f1301d75b..ac02f64d967 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -47,6 +47,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)
Same comment applies here, just change the ifdef above to match on defined(__x86_64__) && defined(CONFIG_SANDBOX)
Yes, understood, same comment as on the other patch. We can always add support for ARM, etc. when people can try it and test it.
What would keep people from trying it?
Alex
Hi Alex,
On 11 June 2018 at 23:44, Alexander Graf agraf@suse.de wrote:
On 12.06.18 07:27, Simon Glass wrote:
Hi Alex,
On 24 May 2018 at 06:34, Alexander Graf agraf@suse.de wrote:
On 16.05.18 17:42, 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
Changes in v4: None Changes in v3: None Changes in v2: None
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 52f1301d75b..ac02f64d967 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -47,6 +47,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)
Same comment applies here, just change the ifdef above to match on defined(__x86_64__) && defined(CONFIG_SANDBOX)
Yes, understood, same comment as on the other patch. We can always add support for ARM, etc. when people can try it and test it.
What would keep people from trying it?
Time and inclination, most likely.
Regards, Simon
These constants are defined in arch-specific code but redefined here. Add a TODO to clean this up.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
lib/efi_loader/efi_runtime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index ac02f64d967..e94b94389d8 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -36,6 +36,10 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void); #define EFI_CACHELINE_SIZE 128 #endif
+/* + * TODO(sjg@chromium.org): These defines and structs should come from the elf. + * header for each arch (or a generic header) rather than being repeated here. + */ #if defined(CONFIG_ARM64) #define R_RELATIVE 1027 #define R_MASK 0xffffffffULL
On 05/16/2018 05:42 PM, Simon Glass wrote:
These constants are defined in arch-specific code but redefined here. Add a TODO to clean this up.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3: None Changes in v2: None
lib/efi_loader/efi_runtime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index ac02f64d967..e94b94389d8 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -36,6 +36,10 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void); #define EFI_CACHELINE_SIZE 128 #endif
+/*
- TODO(sjg@chromium.org): These defines and structs should come from the elf.
%s/elf./elf/
- header for each arch (or a generic header) rather than being repeated here.
- */
#if defined(CONFIG_ARM64) #define R_RELATIVE 1027 #define R_MASK 0xffffffffULL
I would prefer if we would simply use glibc elf.h. But this will also involve some rewriting. The value above for example is part of macro ELF64_R_TYPE(i) in glibc elf.h.
Except for the typo.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
This allows this feature to build within sandbox. This is for testing purposes only since it is not possible for sandbox to load native code.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: - Init the 'rows' and 'cols' vars to avoid a compiler error (gcc 4.8.4)
Changes in v2: None
lib/efi_loader/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index d38780b604e..96107a90a90 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -1,6 +1,6 @@ config EFI_LOADER bool "Support running EFI Applications in U-Boot" - depends on (ARM || X86) && OF_LIBFDT + depends on (ARM || X86 || SANDBOX) && OF_LIBFDT # We do not support bootefi booting ARMv7 in non-secure mode depends on !ARMV7_NONSEC # We need EFI_STUB_64BIT to be set on x86_64 with EFI_STUB
We plan to run more than one EFI test. In order to avoid duplicating code, create functions which handle preparing for running the test and cleaning up afterwards.
Also shorten the awfully long variable names here.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: - Add new patch to split out test init/uninit into functions
Changes in v2: None
cmd/bootefi.c | 93 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 67 insertions(+), 26 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 11b84c55289..9bcdf8bdc48 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -335,6 +335,64 @@ exit: return ret; }
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST +/** + * bootefi_test_prepare() - prepare to run an EFI test + * + * This sets things up so we can call EFI functions. This involves preparing + * the 'gd' pointer and setting up the load ed image data structures. + * + * @image: Pointer to a struct which will hold the loaded image info + * @obj: Pointer to a struct which will hold the loaded image object + * @path: File path to the test being run (often just the test name with a + * backslash before it + * @test_func: Address of the test function that is being run + * @return 0 if OK, -ve on error + */ +static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image, + struct efi_object *obj, + const char *path, ulong test_func) +{ + efi_status_t ret; + + memset(image, '\0', sizeof(*image)); + memset(obj, '\0', sizeof(*obj)); + /* Construct a dummy device path */ + bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, + test_func, test_func); + bootefi_image_path = efi_dp_from_file(NULL, 0, path); + efi_setup_loaded_image(image, obj, bootefi_device_path, + bootefi_image_path); + /* + * gd lives in a fixed register which may get clobbered while we execute + * the payload. So save it here and restore it on every callback entry + */ + efi_save_gd(); + /* Initialize and populate our EFI object list */ + ret = efi_init_obj_list(); + if (ret) + return ret; + /* Transfer environment variable efi_selftest as load options */ + set_load_options(image, "efi_selftest"); + + return 0; +} + +/** + * bootefi_test_finish() - finish up after running an EFI test + * + * @image: Pointer to a struct which holds the loaded image info + * @obj: Pointer to a struct which holds the loaded image object + */ +static void bootefi_test_finish(struct efi_loaded_image *image, + struct efi_object *obj) +{ + efi_restore_gd(); + free(image->load_options); + list_del(&obj->link); +} +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ + static int do_bootefi_bootmgr_exec(void) { struct efi_device_path *device_path, *file_path; @@ -410,33 +468,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) #endif #ifdef CONFIG_CMD_BOOTEFI_SELFTEST if (!strcmp(argv[1], "selftest")) { - struct efi_loaded_image loaded_image_info = {}; - struct efi_object loaded_image_info_obj = {}; - - /* Construct a dummy device path. */ - bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - (uintptr_t)&efi_selftest, - (uintptr_t)&efi_selftest); - bootefi_image_path = efi_dp_from_file(NULL, 0, "\selftest"); - - efi_setup_loaded_image(&loaded_image_info, - &loaded_image_info_obj, - bootefi_device_path, bootefi_image_path); - /* - * gd lives in a fixed register which may get clobbered while we - * execute the payload. So save it here and restore it on every - * callback entry - */ - efi_save_gd(); - /* Initialize and populate EFI object list */ - efi_init_obj_list(); - /* Transfer environment variable efi_selftest as load options */ - set_load_options(&loaded_image_info, "efi_selftest"); + struct efi_loaded_image image; + struct efi_object obj; + + if (bootefi_test_prepare(&image, &obj, "\test", + (uintptr_t)&efi_selftest)) + return CMD_RET_FAILURE; + /* Execute the test */ - r = efi_selftest(loaded_image_info_obj.handle, &systab); - efi_restore_gd(); - free(loaded_image_info.load_options); - list_del(&loaded_image_info_obj.link); + r = efi_selftest(obj.handle, &systab); + bootefi_test_finish(&image, &obj); return r != EFI_SUCCESS; } else #endif
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.
This test can be executed without causing problems to the run-time environemnt (e.g. U-Boot does not need to reboot afterwards).
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 ---
Changes in v4: - Rebase to master - Update SPDX tags
Changes in v3: - Rebase to master
Changes in v2: - Rebase to master
cmd/bootefi.c | 26 ++++++++++++++++++++------ include/efi_loader.h | 3 +++ lib/efi_loader/Kconfig | 10 ++++++++++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_test.c | 16 ++++++++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 lib/efi_loader/efi_test.c
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 9bcdf8bdc48..46bd80d2077 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -335,7 +335,6 @@ exit: return ret; }
-#ifdef CONFIG_CMD_BOOTEFI_SELFTEST /** * bootefi_test_prepare() - prepare to run an EFI test * @@ -391,7 +390,6 @@ static void bootefi_test_finish(struct efi_loaded_image *image, free(image->load_options); list_del(&obj->link); } -#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
static int do_bootefi_bootmgr_exec(void) { @@ -423,8 +421,10 @@ static int do_bootefi_bootmgr_exec(void) /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - unsigned long addr; + struct efi_loaded_image image; + struct efi_object obj; char *saddr; + unsigned long addr; efi_status_t r; void *fdt_addr;
@@ -466,11 +466,25 @@ 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; + + if (bootefi_test_prepare(&image, &obj, "\test", + (ulong)&efi_test)) + return CMD_RET_FAILURE; + + ret = efi_test(&image, &systab); + bootefi_test_finish(&image, &obj); + 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 image; - struct efi_object obj; - if (bootefi_test_prepare(&image, &obj, "\test", (uintptr_t)&efi_selftest)) return CMD_RET_FAILURE; diff --git a/include/efi_loader.h b/include/efi_loader.h index 2519a7c33a7..a19f1eaef24 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -435,6 +435,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 96107a90a90..59bdd389f4f 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -24,3 +24,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 c6046e36d26..2da28f9c90c 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 00000000000..4b8d49f324b --- /dev/null +++ b/lib/efi_loader/efi_test.c @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2017, Google Inc. + */ + +#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; +}
Add a new bootefi_run_prepare() function which holds common code used to set up U-Boot to run EFI code. Make use of this from the existing bootefi_test_prepare() function, as well as do_bootefi_exec().
Also shorten a few variable names.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Rebase to master
Changes in v3: - Add patch to create a function to set up for running EFI code
Changes in v2: None
cmd/bootefi.c | 82 +++++++++++++++++++++++++++------------------------ 1 file changed, 44 insertions(+), 38 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 46bd80d2077..23f05aa867a 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -241,6 +241,33 @@ static efi_status_t efi_install_fdt(void *fdt) return ret; }
+static efi_status_t bootefi_run_prepare(struct efi_loaded_image *image, + struct efi_object *obj, + const char *path, + struct efi_device_path *device_path, + struct efi_device_path *image_path) +{ + efi_status_t ret; + + efi_setup_loaded_image(image, obj, device_path, image_path); + + /* Initialize and populate EFI object list */ + ret = efi_init_obj_list(); + if (ret) + return ret; + + /* + * gd lives in a fixed register which may get clobbered while we execute + * the payload. So save it here and restore it on every callback entry + */ + efi_save_gd(); + + /* Transfer environment variable bootargs as load options */ + set_load_options(image, path); + + return 0; +} + /* * Load an EFI payload into a newly allocated piece of memory, register all * EFI objects it would want to access and jump to it. @@ -249,8 +276,8 @@ static efi_status_t do_bootefi_exec(void *efi, struct efi_device_path *device_path, struct efi_device_path *image_path) { - struct efi_loaded_image loaded_image_info = {}; - struct efi_object loaded_image_info_obj = {}; + struct efi_loaded_image image; + struct efi_object obj; struct efi_device_path *memdp = NULL; efi_status_t ret;
@@ -271,19 +298,13 @@ static efi_status_t do_bootefi_exec(void *efi, assert(device_path && image_path); }
- efi_setup_loaded_image(&loaded_image_info, &loaded_image_info_obj, - device_path, image_path); - - /* - * gd lives in a fixed register which may get clobbered while we execute - * the payload. So save it here and restore it on every callback entry - */ - efi_save_gd(); + ret = bootefi_run_prepare(&image, &obj, "bootargs", device_path, + image_path); + if (ret) + return ret;
- /* Transfer environment variable bootargs as load options */ - set_load_options(&loaded_image_info, "bootargs"); /* Load the EFI payload */ - entry = efi_load_pe(efi, &loaded_image_info); + entry = efi_load_pe(efi, &image); if (!entry) { ret = EFI_LOAD_ERROR; goto exit; @@ -291,10 +312,10 @@ static efi_status_t do_bootefi_exec(void *efi,
if (memdp) { struct efi_device_path_memory *mdp = (void *)memdp; - mdp->memory_type = loaded_image_info.image_code_type; - mdp->start_address = (uintptr_t)loaded_image_info.image_base; + mdp->memory_type = image.image_code_type; + mdp->start_address = (uintptr_t)image.image_base; mdp->end_address = mdp->start_address + - loaded_image_info.image_size; + image.image_size; }
/* we don't support much: */ @@ -304,8 +325,8 @@ static efi_status_t do_bootefi_exec(void *efi, /* Call our payload! */ debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
- if (setjmp(&loaded_image_info.exit_jmp)) { - ret = loaded_image_info.exit_status; + if (setjmp(&image.exit_jmp)) { + ret = image.exit_status; goto exit; }
@@ -317,7 +338,7 @@ static efi_status_t do_bootefi_exec(void *efi,
/* Move into EL2 and keep running there */ armv8_switch_to_el2((ulong)entry, - (ulong)&loaded_image_info_obj.handle, + (ulong)&obj.handle, (ulong)&systab, 0, (ulong)efi_run_in_el2, ES_TO_AARCH64);
@@ -326,11 +347,11 @@ static efi_status_t do_bootefi_exec(void *efi, } #endif
- ret = efi_do_enter(loaded_image_info_obj.handle, &systab, entry); + ret = efi_do_enter(obj.handle, &systab, entry);
exit: /* image has returned, loaded-image obj goes *poof*: */ - list_del(&loaded_image_info_obj.link); + list_del(&obj.link);
return ret; } @@ -352,29 +373,14 @@ static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image, struct efi_object *obj, const char *path, ulong test_func) { - efi_status_t ret; - memset(image, '\0', sizeof(*image)); memset(obj, '\0', sizeof(*obj)); /* Construct a dummy device path */ bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, test_func, test_func); bootefi_image_path = efi_dp_from_file(NULL, 0, path); - efi_setup_loaded_image(image, obj, bootefi_device_path, - bootefi_image_path); - /* - * gd lives in a fixed register which may get clobbered while we execute - * the payload. So save it here and restore it on every callback entry - */ - efi_save_gd(); - /* Initialize and populate our EFI object list */ - ret = efi_init_obj_list(); - if (ret) - return ret; - /* Transfer environment variable efi_selftest as load options */ - set_load_options(image, "efi_selftest"); - - return 0; + return bootefi_run_prepare(image, obj, path, bootefi_device_path, + bootefi_image_path); }
/**
This function can be used from do_bootefi_exec() so that we use mostly the same code for a normal EFI application and an EFI test.
Rename the function and use it in both places.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Rebase to master
Changes in v3: - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
Changes in v2: None
cmd/bootefi.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 23f05aa867a..00a94fc8560 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -268,6 +268,20 @@ static efi_status_t bootefi_run_prepare(struct efi_loaded_image *image, return 0; }
+/** + * bootefi_run_finish() - finish up after running an EFI test + * + * @image: Pointer to a struct which holds the loaded image info + * @obj: Pointer to a struct which holds the loaded image object + */ +static void bootefi_run_finish(struct efi_loaded_image *image, + struct efi_object *obj) +{ + efi_restore_gd(); + free(image->load_options); + list_del(&obj->link); +} + /* * Load an EFI payload into a newly allocated piece of memory, register all * EFI objects it would want to access and jump to it. @@ -350,8 +364,7 @@ static efi_status_t do_bootefi_exec(void *efi, ret = efi_do_enter(obj.handle, &systab, entry);
exit: - /* image has returned, loaded-image obj goes *poof*: */ - list_del(&obj.link); + bootefi_run_finish(&image, &obj);
return ret; } @@ -383,20 +396,6 @@ static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image, bootefi_image_path); }
-/** - * bootefi_test_finish() - finish up after running an EFI test - * - * @image: Pointer to a struct which holds the loaded image info - * @obj: Pointer to a struct which holds the loaded image object - */ -static void bootefi_test_finish(struct efi_loaded_image *image, - struct efi_object *obj) -{ - efi_restore_gd(); - free(image->load_options); - list_del(&obj->link); -} - static int do_bootefi_bootmgr_exec(void) { struct efi_device_path *device_path, *file_path; @@ -480,7 +479,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE;
ret = efi_test(&image, &systab); - bootefi_test_finish(&image, &obj); + bootefi_run_finish(&image, &obj); if (ret) { printf("Test failed: err=%d\n", ret); return CMD_RET_FAILURE; @@ -497,7 +496,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
/* Execute the test */ r = efi_selftest(obj.handle, &systab); - bootefi_test_finish(&image, &obj); + bootefi_run_finish(&image, &obj); return r != EFI_SUCCESS; } else #endif
On 05/16/2018 05:42 PM, 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.
This series is at u-boot-dm/efi-working
I applied you patch series
make sandbox_defconfig CONFIG_CMD_BOOTEFI_SELFTEST=y make -j6
results in:
ld.bfd: read in flex scanner failed scripts/Makefile.lib:407: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_exit_efi.so' failed make[2]: *** [lib/efi_selftest/efi_selftest_miniapp_exit_efi.so] Error 1 rm lib/efi_selftest/efi_selftest_miniapp_exit.o lib/efi_selftest/efi_selftest_miniapp_return.o scripts/Makefile.build:423: recipe for target 'lib/efi_selftest' failed make[1]: *** [lib/efi_selftest] Error 2 make[1]: *** Waiting for unfinished jobs....
Please, change /lib/efi_selftest/Makefile -ifeq ($(CONFIG_X86_64),) +ifeq ($(CONFIG_X86_64)$(CONFIG_SANDBOX),)
Now running ./u-boot
bootefi selftest Found 0 disks WARNING: booting without device tree
Testing EFI API implementation
Number of tests to execute: 17
Setting up 'block device' Setting up 'block device' succeeded
Executing 'block device' lib/efi_selftest/efi_selftest_block_device.c(378): TODO: Wrong volume label 'xxa1', expected 'U-BOOT TEST' FAT: Misaligned buffer address (00007ff70aafe658) Segmentation fault
Please, fix the alignment fault. You have to ensure that the memory that Sandbox has retrieved via malloc is reduced to 4k aligned pages before being published to the EFI implementation in lib/efi_loader/efi_memory.c
Best regards
Heinrich
Changes in v4:
- Fix up the sizeof() operations on jmp_buf
- Move the fix to query_console_serial()
- Rebase to master
- Remove code already applied
- Update SPDX tags
- Update subject
Changes in v3:
- Add comments on aligment
- Add new patch to init the 'rows' and 'cols' variables
- Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
- Add new patch to split out test init/uninit into functions
- Add patch to create a function to set up for running EFI code
- Drop incorrect map_sysmem() in write_smbios_table()
- Init the 'rows' and 'cols' vars to avoid a compiler error (gcc 4.8.4)
- Rebase to master
- Return error value of efi_allocate_pages()
- Update function comment for write_smbios_table()
Changes in v2:
- Rebase to master
- Update return type of efi_smbios_register() to efi_status_t
- Update to use mapmem instead of a cast
- Use return value of efi_install_configuration_table
Simon Glass (16): efi: Init the 'rows' and 'cols' variables efi: Update some comments related to smbios tables efi: sandbox: Adjust memory usage 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: Split out test init/uninit into functions efi: sandbox: Add a simple 'bootefi test' command efi: Create a function to set up for running EFI code efi: Rename bootefi_test_finish() to bootefi_run_finish()
arch/arm/include/asm/u-boot-arm.h | 1 - arch/sandbox/cpu/cpu.c | 13 +++ arch/sandbox/cpu/os.c | 23 +++++ arch/sandbox/cpu/u-boot.lds | 29 ++++++ arch/sandbox/include/asm/setjmp.h | 30 ++++++ 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 | 158 +++++++++++++++++++++--------- common/bootm.c | 4 + include/bootm.h | 8 ++ include/config_distro_bootcmd.h | 2 +- include/efi_loader.h | 10 ++ include/os.h | 21 ++++ include/smbios.h | 5 +- lib/efi_loader/Kconfig | 12 ++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_console.c | 5 +- lib/efi_loader/efi_memory.c | 31 +++--- lib/efi_loader/efi_runtime.c | 7 ++ lib/efi_loader/efi_smbios.c | 7 +- lib/efi_loader/efi_test.c | 16 +++ lib/smbios.c | 32 ++++-- 24 files changed, 351 insertions(+), 83 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
On 05/16/2018 07:13 PM, Heinrich Schuchardt wrote:
On 05/16/2018 05:42 PM, 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.
This series is at u-boot-dm/efi-working
I applied you patch series
make sandbox_defconfig CONFIG_CMD_BOOTEFI_SELFTEST=y make -j6
results in:
ld.bfd: read in flex scanner failed scripts/Makefile.lib:407: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_exit_efi.so' failed make[2]: *** [lib/efi_selftest/efi_selftest_miniapp_exit_efi.so] Error 1 rm lib/efi_selftest/efi_selftest_miniapp_exit.o lib/efi_selftest/efi_selftest_miniapp_return.o scripts/Makefile.build:423: recipe for target 'lib/efi_selftest' failed make[1]: *** [lib/efi_selftest] Error 2 make[1]: *** Waiting for unfinished jobs....
Please, change /lib/efi_selftest/Makefile -ifeq ($(CONFIG_X86_64),) +ifeq ($(CONFIG_X86_64)$(CONFIG_SANDBOX),)
A better solution would be to define EFI_LDS, EFI_CRT0, and EFI_RELOC in file arch/sandbox/config.mk in accordance with the host architecture.
Best regards
Heinrich
Hi Heinrich,
On 16 May 2018 at 23:31, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 05/16/2018 07:13 PM, Heinrich Schuchardt wrote:
On 05/16/2018 05:42 PM, 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.
This series is at u-boot-dm/efi-working
I applied you patch series
make sandbox_defconfig CONFIG_CMD_BOOTEFI_SELFTEST=y make -j6
results in:
ld.bfd: read in flex scanner failed scripts/Makefile.lib:407: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_exit_efi.so' failed make[2]: *** [lib/efi_selftest/efi_selftest_miniapp_exit_efi.so] Error 1 rm lib/efi_selftest/efi_selftest_miniapp_exit.o lib/efi_selftest/efi_selftest_miniapp_return.o scripts/Makefile.build:423: recipe for target 'lib/efi_selftest' failed make[1]: *** [lib/efi_selftest] Error 2 make[1]: *** Waiting for unfinished jobs....
Please, change /lib/efi_selftest/Makefile -ifeq ($(CONFIG_X86_64),) +ifeq ($(CONFIG_X86_64)$(CONFIG_SANDBOX),)
A better solution would be to define EFI_LDS, EFI_CRT0, and EFI_RELOC in file arch/sandbox/config.mk in accordance with the host architecture.
I think we should get the patches in so that these tests can run, and worry about CONFIG_CMD_BOOTEFI_SELFTEST later.
This work has been outstanding for a long time and it has been painful to rebase on a tree that is changing quite regularly.
After all, the point of this series is to enable running the code from EFI app directly within sandbox, not to run the .efi app itself.
I suspect you are right that with more work on the linker flags this can be made to work. But it is not the subject of this series.
For now I'll add a patch to explicitly disable that option for sandbox, so people know it is not supported.
Regards, Simon
On 16.05.18 17:42, 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.
This series is at u-boot-dm/efi-working
I've picked a few patches that were obviously correct and made sense. Overall, I think we should allow for real UEFI binaries to run in sandbox and the only way to get there is to have sandboxed U-Boot live in the same address space as what it thinks it lives in.
For the selftest bits I'd like to see acks from Heinrich, so I didn't pick those up yet. They seem to make good sense to me though.
Alex
participants (3)
-
Alexander Graf -
Heinrich Schuchardt -
Simon Glass