[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader

[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader

[Simon Glass]

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

-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 01/16] efi: Init the 'rows' and 'cols' variables

[Simon Glass]

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];
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 01/16] efi: Init the 'rows' and 'cols' variables

[Heinrich Schuchardt]

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];
> 

[U-Boot] [PATCH v4 02/16] efi: Update some comments related to smbios tables

[Simon Glass]

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 */
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 02/16] efi: Update some comments related to smbios tables

[Heinrich Schuchardt]

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

[U-Boot] [U-Boot, v4, 02/16] efi: Update some comments related to smbios tables

[Alexander Graf]

> 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

[U-Boot] [PATCH v4 03/16] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

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 */
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 03/16] efi: sandbox: Adjust memory usage for sandbox

[Heinrich Schuchardt]

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

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

[U-Boot] [PATCH v4 03/16] efi: sandbox: Adjust memory usage for sandbox

[Heinrich Schuchardt]

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

[U-Boot] [PATCH v4 03/16] efi: sandbox: Adjust memory usage for sandbox

[Heinrich Schuchardt]

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

[U-Boot] [PATCH v4 03/16] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

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;
 }
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Alexander Graf]

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

[U-Boot] [PATCH v4 04/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

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

[U-Boot] [PATCH v4 05/16] sandbox: Add a setjmp() implementation

[Simon Glass]

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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [U-Boot, v4, 05/16] sandbox: Add a setjmp() implementation

[Alexander Graf]

> 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

[U-Boot] [PATCH v4 05/16] sandbox: Add a setjmp() implementation

[Alexander Graf]

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

[U-Boot] [PATCH v4 05/16] sandbox: Add a setjmp() implementation

[Simon Glass]

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

[U-Boot] [PATCH v4 05/16] sandbox: Add a setjmp() implementation

[Alexander Graf]

> 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

[U-Boot] [PATCH v4 06/16] efi: sandbox: Add required linker sections

[Simon Glass]

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 at 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")));
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [U-Boot, v4, 06/16] efi: sandbox: Add required linker sections

[Alexander Graf]

> 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

[U-Boot] [PATCH v4 07/16] efi: sandbox: Add distroboot support

[Simon Glass]

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"
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 07/16] efi: sandbox: Add distroboot support

[Alexander Graf]

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"
> 

[U-Boot] [PATCH v4 07/16] efi: sandbox: Add distroboot support

[Simon Glass]

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

[U-Boot] [PATCH v4 07/16] efi: sandbox: Add distroboot support

[Alexander Graf]

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

[U-Boot] [PATCH v4 07/16] efi: sandbox: Add distroboot support

[Simon Glass]

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

[U-Boot] [PATCH v4 08/16] Define board_quiesce_devices() in a shared location

[Simon Glass]

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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [U-Boot, v4, 08/16] Define board_quiesce_devices() in a shared location

[Alexander Graf]

> 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

[U-Boot] [PATCH v4 09/16] Add a comment for board_quiesce_devices()

[Simon Glass]

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 at chromium.org>): Update this to use driver model's device_remove().
+ */
 void board_quiesce_devices(void);
 
 #endif
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [U-Boot, v4, 09/16] Add a comment for board_quiesce_devices()

[Alexander Graf]

> 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

[U-Boot] [PATCH v4 10/16] efi: sandbox: Add relocation constants

[Simon Glass]

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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 10/16] efi: sandbox: Add relocation constants

[Alexander Graf]

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
> 

[U-Boot] [PATCH v4 10/16] efi: sandbox: Add relocation constants

[Simon Glass]

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

[U-Boot] [PATCH v4 10/16] efi: sandbox: Add relocation constants

[Alexander Graf]

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

[U-Boot] [PATCH v4 10/16] efi: sandbox: Add relocation constants

[Simon Glass]

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

[U-Boot] [PATCH v4 11/16] efi: Add a comment about duplicated ELF constants

[Simon Glass]

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 at 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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 11/16] efi: Add a comment about duplicated ELF constants

[Heinrich Schuchardt]

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 at 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>

> 

[U-Boot] [PATCH v4 12/16] efi: sandbox: Enable EFI loader builder for sandbox

[Simon Glass]

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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 13/16] efi: Split out test init/uninit into functions

[Simon Glass]

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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 14/16] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

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;
+}
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 15/16] efi: Create a function to set up for running EFI code

[Simon Glass]

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);
 }
 
 /**
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 16/16] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Simon Glass]

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
-- 
2.17.0.441.gb46fe60e1d-goog

[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader

[Heinrich Schuchardt]

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
> 

[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader

[Heinrich Schuchardt]

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

[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader

[Simon Glass]

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

[U-Boot] [PATCH v4 00/16] efi: Enable basic sandbox support for EFI loader

[Alexander Graf]

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