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

[U-Boot] [PATCH v5 00/13] 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 v5:
- Add new patch to disallow CMD_BOOTEFI_SELFTEST on sandbox
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
- Drop period after "elf" in comment
- Introduce load_options_path to specifyc U-Boot env var for load_options_path
- Rebase to master

Changes in v4:
- Move the fix to query_console_serial()
- Rebase to master
- Update SPDX tags

Changes in v3:
- 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

Changes in v2:
- Rebase to master
- Update to use mapmem instead of a cast

Simon Glass (13):
  efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox
  efi: Init the 'rows' and 'cols' variables
  efi: sandbox: Adjust memory usage for sandbox
  sandbox: smbios: Update to support sandbox
  efi: sandbox: Add distroboot support
  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()
  buildman: Add a --boards option to specify particular boards to build

 cmd/bootefi.c                   | 153 ++++++++++++++++++++++----------
 include/config_distro_bootcmd.h |   2 +-
 include/efi_loader.h            |   3 +
 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_test.c       |  16 ++++
 lib/efi_selftest/Kconfig        |   2 +-
 lib/smbios.c                    |  32 +++++--
 tools/buildman/README           |  12 ++-
 tools/buildman/board.py         |  28 +++++-
 tools/buildman/cmdline.py       |   4 +-
 tools/buildman/control.py       |  20 ++++-
 tools/buildman/test.py          |  31 ++++---
 16 files changed, 264 insertions(+), 95 deletions(-)
 create mode 100644 lib/efi_loader/efi_test.c

-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 01/13] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox

[Simon Glass]

This does not work at present and gives the following error:

output: 'ld.bfd: read in flex scanner failed
scripts/Makefile.lib:390: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_return_efi.so' failed

It may be possible to figure this out with suitable linker magic but it
does not seem to be easy. Also, we will be able to run the tests on
sandbox without using the miniapp.

So for now at least, disable this option.

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

Changes in v5:
- Add new patch to disallow CMD_BOOTEFI_SELFTEST on sandbox

Changes in v4: None
Changes in v3: None
Changes in v2: None

 lib/efi_selftest/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_selftest/Kconfig b/lib/efi_selftest/Kconfig
index 59f9f36801..b52696778d 100644
--- a/lib/efi_selftest/Kconfig
+++ b/lib/efi_selftest/Kconfig
@@ -1,6 +1,6 @@
 config CMD_BOOTEFI_SELFTEST
 	bool "Allow booting an EFI efi_selftest"
-	depends on CMD_BOOTEFI
+	depends on CMD_BOOTEFI && !SANDBOX
 	imply FAT
 	imply FAT_WRITE
 	help
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 01/13] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox

[Heinrich Schuchardt]

On 06/12/2018 07:26 AM, Simon Glass wrote:
> This does not work at present and gives the following error:
> 
> output: 'ld.bfd: read in flex scanner failed
> scripts/Makefile.lib:390: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_return_efi.so' failed
> 
> It may be possible to figure this out with suitable linker magic but it
> does not seem to be easy. Also, we will be able to run the tests on
> sandbox without using the miniapp.
> 
> So for now at least, disable this option.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v5:
> - Add new patch to disallow CMD_BOOTEFI_SELFTEST on sandbox
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  lib/efi_selftest/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/efi_selftest/Kconfig b/lib/efi_selftest/Kconfig
> index 59f9f36801..b52696778d 100644
> --- a/lib/efi_selftest/Kconfig
> +++ b/lib/efi_selftest/Kconfig
> @@ -1,6 +1,6 @@
>  config CMD_BOOTEFI_SELFTEST
>  	bool "Allow booting an EFI efi_selftest"
> -	depends on CMD_BOOTEFI
> +	depends on CMD_BOOTEFI && !SANDBOX

It is sufficient to change the following line in
lib/efi_selftest/Makefile to exclude building of
efi_selftest_startimage_exit.o and
efi_selftest_startimage_return.o:

-ifeq ($(CONFIG_X86_64),)
+ifeq ($(CONFIG_X86_64)$(SANDBOX),)

This way we can run all other tests.

Best regards

Heinrich

>  	imply FAT
>  	imply FAT_WRITE
>  	help
> 

[U-Boot] [PATCH v5 01/13] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox

[Simon Glass]

Hi Heinrich,

On 11 June 2018 at 23:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/12/2018 07:26 AM, Simon Glass wrote:
>> This does not work at present and gives the following error:
>>
>> output: 'ld.bfd: read in flex scanner failed
>> scripts/Makefile.lib:390: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_return_efi.so' failed
>>
>> It may be possible to figure this out with suitable linker magic but it
>> does not seem to be easy. Also, we will be able to run the tests on
>> sandbox without using the miniapp.
>>
>> So for now at least, disable this option.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v5:
>> - Add new patch to disallow CMD_BOOTEFI_SELFTEST on sandbox
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>  lib/efi_selftest/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_selftest/Kconfig b/lib/efi_selftest/Kconfig
>> index 59f9f36801..b52696778d 100644
>> --- a/lib/efi_selftest/Kconfig
>> +++ b/lib/efi_selftest/Kconfig
>> @@ -1,6 +1,6 @@
>>  config CMD_BOOTEFI_SELFTEST
>>       bool "Allow booting an EFI efi_selftest"
>> -     depends on CMD_BOOTEFI
>> +     depends on CMD_BOOTEFI && !SANDBOX
>
> It is sufficient to change the following line in
> lib/efi_selftest/Makefile to exclude building of
> efi_selftest_startimage_exit.o and
> efi_selftest_startimage_return.o:
>
> -ifeq ($(CONFIG_X86_64),)
> +ifeq ($(CONFIG_X86_64)$(SANDBOX),)
>
> This way we can run all other tests.

It can build them but they don't work for me. I would like to leave
this as future work as there have been plenty of changes to this
long-running series already.

Regards,
Simon

[U-Boot] [PATCH v5 02/13] 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 v5: None
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 ce66c935ec..bd953a1485 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -204,8 +204,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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 02/13] efi: Init the 'rows' and 'cols' variables

[Heinrich Schuchardt]

On 06/12/2018 07:26 AM, 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>

Is this needed after Alex's
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=80483b2ab62ca7cd200db445b6920ee96d17df88
?

Best regards

Heinrich

> ---
> 
> Changes in v5: None
> 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 ce66c935ec..bd953a1485 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -204,8 +204,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 v5 02/13] efi: Init the 'rows' and 'cols' variables

[Simon Glass]

Hi,

On 11 June 2018 at 23:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/12/2018 07:26 AM, 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>
>
> Is this needed after Alex's
> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=80483b2ab62ca7cd200db445b6920ee96d17df88
> ?

I missed that one.

I actually think his is the better fix, since we really shouldn't be
setting return values in case of error.

Regards,
Simon

>
> Best regards
>
> Heinrich
>
>> ---
>>
>> Changes in v5: None
>> 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 ce66c935ec..bd953a1485 100644
>> --- a/lib/efi_loader/efi_console.c
>> +++ b/lib/efi_loader/efi_console.c
>> @@ -204,8 +204,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 v5 03/13] 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 v5: None
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 ec66af98ea..210e49ee8b 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 <linux/list_sort.h>
 
@@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
 			       &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;
 	}
@@ -504,18 +505,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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 03/13] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 12.06.18 07:26, Simon Glass wrote:
> With sandbox the U-Boot code is not mapped into the sandbox memory range
> so does not need to be excluded when allocating EFI memory. Update the EFI
> memory init code to take account of that.
> 
> Also use mapmem instead of a cast to convert a memory address to a
> pointer.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v5: None
> 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 ec66af98ea..210e49ee8b 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 <linux/list_sort.h>
>  
> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>  			       &t);
>  
>  	if (r == EFI_SUCCESS) {
> -		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
> +		struct efi_pool_allocation *alloc = map_sysmem(t, size);

We want to eventually be able to run efi binaries inside sandbox, so we
need to expose a 1:1 memory map inside the efi interfaces.

That means the memory argument of efi_allocate_pages() already needs to
be set to the virtual address in real VA space. The easiest way to get
there is if you just put virtual addresses in the efi memory map.

>  		alloc->num_pages = num_pages;
>  		*buffer = alloc->data;
>  	}
> @@ -504,18 +505,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);

I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please
extract the code into a function though, so that we don't get into too
much indenting.

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

I guess we do want to indicate RTS, no? But like everything else we want
to expose it with the real VA addresses.


Alex

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

[U-Boot] [PATCH v5 03/13] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 12 June 2018 at 02:05, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 07:26, Simon Glass wrote:
>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>> so does not need to be excluded when allocating EFI memory. Update the EFI
>> memory init code to take account of that.
>>
>> Also use mapmem instead of a cast to convert a memory address to a
>> pointer.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v5: None
>> 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 ec66af98ea..210e49ee8b 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 <linux/list_sort.h>
>>
>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>>                              &t);
>>
>>       if (r == EFI_SUCCESS) {
>> -             struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
>> +             struct efi_pool_allocation *alloc = map_sysmem(t, size);
>
> We want to eventually be able to run efi binaries inside sandbox, so we
> need to expose a 1:1 memory map inside the efi interfaces.
>
> That means the memory argument of efi_allocate_pages() already needs to
> be set to the virtual address in real VA space. The easiest way to get
> there is if you just put virtual addresses in the efi memory map.

Can you please explain that a bit more, or give a code example? I'm
not quite sure what you mean.

>
>>               alloc->num_pages = num_pages;
>>               *buffer = alloc->data;
>>       }
>> @@ -504,18 +505,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);
>
> I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please
> extract the code into a function though, so that we don't get into too
> much indenting.
>

OK

>> +
>> +             /* 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);
>> +     }
>
> I guess we do want to indicate RTS, no? But like everything else we want
> to expose it with the real VA addresses.

I suspect it would never be used but also that we should indicate RTS
is present so that things that check for it don't fail. What do you
think we should do here?

Regards,
Simon

[U-Boot] [PATCH v5 03/13] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 12.06.18 15:48, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 02:05, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 12.06.18 07:26, Simon Glass wrote:
>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>> so does not need to be excluded when allocating EFI memory. Update the EFI
>>> memory init code to take account of that.
>>>
>>> Also use mapmem instead of a cast to convert a memory address to a
>>> pointer.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v5: None
>>> 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 ec66af98ea..210e49ee8b 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 <linux/list_sort.h>
>>>
>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>>>                              &t);
>>>
>>>       if (r == EFI_SUCCESS) {
>>> -             struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
>>> +             struct efi_pool_allocation *alloc = map_sysmem(t, size);
>>
>> We want to eventually be able to run efi binaries inside sandbox, so we
>> need to expose a 1:1 memory map inside the efi interfaces.
>>
>> That means the memory argument of efi_allocate_pages() already needs to
>> be set to the virtual address in real VA space. The easiest way to get
>> there is if you just put virtual addresses in the efi memory map.
> 
> Can you please explain that a bit more, or give a code example? I'm
> not quite sure what you mean.

In efi_add_known_memory() we populate the EFI memory table with
addresses that EFI allocations can return. Because we don't control the
payloads that call these functions, we can only return pointers. That
means efi_add_known_memory() should add map_sysmem()'ed values into its
own table.

Basically while we expose "virtual 0 offset" addresses to the command
line, anything internal still works on pointers. And everything EFI
internal needs to be considered a pointer, because we don't control the
code that deals with them.

So in a nutshell, I mean something like this (untested, probably
whitespace broken and line wrapped):

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index ec66af98ea..80211d8c95 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -491,6 +491,9 @@ __weak void efi_add_known_memory(void)
 		u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
 		u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;

+		/* Sandbox needs virtual addresses here */
+		start = (uintptr_t)map_sysmem(start, pages * EFI_PAGE_SIZE);
+
 		efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
 				   false);
 	}

> 
>>
>>>               alloc->num_pages = num_pages;
>>>               *buffer = alloc->data;
>>>       }
>>> @@ -504,18 +505,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);
>>
>> I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please
>> extract the code into a function though, so that we don't get into too
>> much indenting.
>>
> 
> OK
> 
>>> +
>>> +             /* 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);
>>> +     }
>>
>> I guess we do want to indicate RTS, no? But like everything else we want
>> to expose it with the real VA addresses.
> 
> I suspect it would never be used but also that we should indicate RTS
> is present so that things that check for it don't fail. What do you
> think we should do here?

I'm not 100% sure yet. It'll be very hard to support anything that
relies on exit_boot_services() from within user space. So yes, just
leave it out for sandbox for now.


Alex

[U-Boot] [PATCH v5 03/13] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 12 June 2018 at 08:02, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 15:48, Simon Glass wrote:
>> Hi Alex,
>>
>> On 12 June 2018 at 02:05, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 12.06.18 07:26, Simon Glass wrote:
>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>> so does not need to be excluded when allocating EFI memory. Update the EFI
>>>> memory init code to take account of that.
>>>>
>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>> pointer.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v5: None
>>>> 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 ec66af98ea..210e49ee8b 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 <linux/list_sort.h>
>>>>
>>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>>>>                              &t);
>>>>
>>>>       if (r == EFI_SUCCESS) {
>>>> -             struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
>>>> +             struct efi_pool_allocation *alloc = map_sysmem(t, size);
>>>
>>> We want to eventually be able to run efi binaries inside sandbox, so we
>>> need to expose a 1:1 memory map inside the efi interfaces.
>>>
>>> That means the memory argument of efi_allocate_pages() already needs to
>>> be set to the virtual address in real VA space. The easiest way to get
>>> there is if you just put virtual addresses in the efi memory map.
>>
>> Can you please explain that a bit more, or give a code example? I'm
>> not quite sure what you mean.
>
> In efi_add_known_memory() we populate the EFI memory table with
> addresses that EFI allocations can return. Because we don't control the
> payloads that call these functions, we can only return pointers. That
> means efi_add_known_memory() should add map_sysmem()'ed values into its
> own table.
>
> Basically while we expose "virtual 0 offset" addresses to the command
> line, anything internal still works on pointers. And everything EFI
> internal needs to be considered a pointer, because we don't control the
> code that deals with them.
>
> So in a nutshell, I mean something like this (untested, probably
> whitespace broken and line wrapped):
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index ec66af98ea..80211d8c95 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -491,6 +491,9 @@ __weak void efi_add_known_memory(void)
>                 u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
>                 u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>
> +               /* Sandbox needs virtual addresses here */
> +               start = (uintptr_t)map_sysmem(start, pages * EFI_PAGE_SIZE);
> +
>                 efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
>                                    false);
>         }
>

The map_sysmem() call is already when allocated addresses are returned
- see elsewhere in the file. So adding it here as well will cause a
double translation. Still, I tried this out and it just fails to init.

Does any EFI app have access to the internal tables containing the
memory addresses? If not, then perhaps it is OK to use U-Boot
addresses here?

In any case Heinrich has mentioned an alignment problem that needs to
be resolved.

Regards,
Simon

[U-Boot] [PATCH v5 04/13] 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 v5: None
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 df3d26b071..fc3dabcbc1 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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 04/13] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 12.06.18 07:26, Simon Glass wrote:
> At present this code casts addresses to pointers so cannot be used with
> sandbox. Update it to use mapmem instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v5: None
> 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 df3d26b071..fc3dabcbc1 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)

Please change the function argument to indicate that we're no longer
dealing with pointers, but instead with "u-boot physical addresses".

Same for the other functions obviously :).


Alex

>  {
> -	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;
>  }
> 

[U-Boot] [PATCH v5 04/13] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 12 June 2018 at 02:12, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 07:26, Simon Glass wrote:
>> At present this code casts addresses to pointers so cannot be used with
>> sandbox. Update it to use mapmem instead.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v5: None
>> 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 df3d26b071..fc3dabcbc1 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)
>
> Please change the function argument to indicate that we're no longer
> dealing with pointers, but instead with "u-boot physical addresses".
>
> Same for the other functions obviously :).

That actually hasn't changed. We are currently passing a U-Boot
address around and it is a ulong, as it normally is in U-Boot. What
has changed is that sandbox does not have a direct mapping between
U-Boot address and memory address, so we have to do the mapping.

While it is try that the ulong can be converted to a pointer with a
cast normally, this is not possible with sandbox, so things that need
to convert the ulong to a pointer need to do a conversion.

Regards,
Simon

[U-Boot] [PATCH v5 04/13] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 12.06.18 15:48, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 02:12, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 12.06.18 07:26, Simon Glass wrote:
>>> At present this code casts addresses to pointers so cannot be used with
>>> sandbox. Update it to use mapmem instead.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v5: None
>>> 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 df3d26b071..fc3dabcbc1 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)
>>
>> Please change the function argument to indicate that we're no longer
>> dealing with pointers, but instead with "u-boot physical addresses".
>>
>> Same for the other functions obviously :).
> 
> That actually hasn't changed. We are currently passing a U-Boot
> address around and it is a ulong, as it normally is in U-Boot. What
> has changed is that sandbox does not have a direct mapping between
> U-Boot address and memory address, so we have to do the mapping.
> 
> While it is try that the ulong can be converted to a pointer with a
> cast normally, this is not possible with sandbox, so things that need
> to convert the ulong to a pointer need to do a conversion.

Oh, I missed the * in *current. So it already does get passed as a
number which then is cast back into a pointer.

That however means that the smbios tables are now u-boot address space
relative. So anything that tries to read them from within EFI context
will explode.


Alex

[U-Boot] [PATCH v5 04/13] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 12 June 2018 at 08:05, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 15:48, Simon Glass wrote:
>> Hi Alex,
>>
>> On 12 June 2018 at 02:12, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 12.06.18 07:26, Simon Glass wrote:
>>>> At present this code casts addresses to pointers so cannot be used with
>>>> sandbox. Update it to use mapmem instead.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v5: None
>>>> 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 df3d26b071..fc3dabcbc1 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)
>>>
>>> Please change the function argument to indicate that we're no longer
>>> dealing with pointers, but instead with "u-boot physical addresses".
>>>
>>> Same for the other functions obviously :).
>>
>> That actually hasn't changed. We are currently passing a U-Boot
>> address around and it is a ulong, as it normally is in U-Boot. What
>> has changed is that sandbox does not have a direct mapping between
>> U-Boot address and memory address, so we have to do the mapping.
>>
>> While it is try that the ulong can be converted to a pointer with a
>> cast normally, this is not possible with sandbox, so things that need
>> to convert the ulong to a pointer need to do a conversion.
>
> Oh, I missed the * in *current. So it already does get passed as a
> number which then is cast back into a pointer.
>
> That however means that the smbios tables are now u-boot address space
> relative. So anything that tries to read them from within EFI context
> will explode.

Aren't these tables for the Linux kernel? If so, then this doesn't matter.

But if EFI reads them, we are in trouble. We cannot always put a
64-bit address into a 32-bit word.

I suppose we could support it on 32-bit sandbox, but not a lot of
people use that.

Regards,
Simon

[U-Boot] [PATCH v5 04/13] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 12.06.18 23:56, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 08:05, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 12.06.18 15:48, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 12 June 2018 at 02:12, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>> On 12.06.18 07:26, Simon Glass wrote:
>>>>> At present this code casts addresses to pointers so cannot be used with
>>>>> sandbox. Update it to use mapmem instead.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v5: None
>>>>> 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 df3d26b071..fc3dabcbc1 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)
>>>>
>>>> Please change the function argument to indicate that we're no longer
>>>> dealing with pointers, but instead with "u-boot physical addresses".
>>>>
>>>> Same for the other functions obviously :).
>>>
>>> That actually hasn't changed. We are currently passing a U-Boot
>>> address around and it is a ulong, as it normally is in U-Boot. What
>>> has changed is that sandbox does not have a direct mapping between
>>> U-Boot address and memory address, so we have to do the mapping.
>>>
>>> While it is try that the ulong can be converted to a pointer with a
>>> cast normally, this is not possible with sandbox, so things that need
>>> to convert the ulong to a pointer need to do a conversion.
>>
>> Oh, I missed the * in *current. So it already does get passed as a
>> number which then is cast back into a pointer.
>>
>> That however means that the smbios tables are now u-boot address space
>> relative. So anything that tries to read them from within EFI context
>> will explode.
> 
> Aren't these tables for the Linux kernel? If so, then this doesn't matter.
> 
> But if EFI reads them, we are in trouble. We cannot always put a
> 64-bit address into a 32-bit word.
> 
> I suppose we could support it on 32-bit sandbox, but not a lot of
> people use that.

Sorry, I must've misremembered. I can not see a single reference of a
physical address in the SMBIOS code. So your patch looks correct to me.


Alex

[U-Boot] [PATCH v5 05/13] 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 v5: None
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 d672e8ebe6..8d11f52da0 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -251,7 +251,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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 05/13] efi: sandbox: Add distroboot support

[Alexander Graf]

On 12.06.18 07:26, 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 v5: None
> 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 d672e8ebe6..8d11f52da0 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -251,7 +251,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)

I was serious when I said I wanted to have a defined(__x86_64__) guard.
Otherwise we'll expose incorrect information. And I doubt that anyone
will catch it when porting sandbox to non-x86, because it doesn't error out.


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 v5 05/13] efi: sandbox: Add distroboot support

[Simon Glass]

Hi Alex,

On 12 June 2018 at 02:13, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 07:26, 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 v5: None
>> 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 d672e8ebe6..8d11f52da0 100644
>> --- a/include/config_distro_bootcmd.h
>> +++ b/include/config_distro_bootcmd.h
>> @@ -251,7 +251,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)
>
> I was serious when I said I wanted to have a defined(__x86_64__) guard.
> Otherwise we'll expose incorrect information. And I doubt that anyone
> will catch it when porting sandbox to non-x86, because it doesn't error out.

OK I can do a warning but I cannot use the current guard, otherwise it
prevents sandbox even building on ARM hosts!

Regards,
Simon

[U-Boot] [PATCH v5 05/13] efi: sandbox: Add distroboot support

[Alexander Graf]

On 12.06.18 15:48, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 02:13, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 12.06.18 07:26, 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 v5: None
>>> 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 d672e8ebe6..8d11f52da0 100644
>>> --- a/include/config_distro_bootcmd.h
>>> +++ b/include/config_distro_bootcmd.h
>>> @@ -251,7 +251,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)
>>
>> I was serious when I said I wanted to have a defined(__x86_64__) guard.
>> Otherwise we'll expose incorrect information. And I doubt that anyone
>> will catch it when porting sandbox to non-x86, because it doesn't error out.
> 
> OK I can do a warning but I cannot use the current guard, otherwise it
> prevents sandbox even building on ARM hosts!

Just change defined(CONFIG_X86) into defined(__x86_64__) ||
defined(__i386__) then? Maybe the same for the other archs?


Alex

[U-Boot] [PATCH v5 05/13] efi: sandbox: Add distroboot support

[Simon Glass]

Hi Alex,

On 12 June 2018 at 08:06, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 15:48, Simon Glass wrote:
>> Hi Alex,
>>
>> On 12 June 2018 at 02:13, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 12.06.18 07:26, 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 v5: None
>>>> 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 d672e8ebe6..8d11f52da0 100644
>>>> --- a/include/config_distro_bootcmd.h
>>>> +++ b/include/config_distro_bootcmd.h
>>>> @@ -251,7 +251,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)
>>>
>>> I was serious when I said I wanted to have a defined(__x86_64__) guard.
>>> Otherwise we'll expose incorrect information. And I doubt that anyone
>>> will catch it when porting sandbox to non-x86, because it doesn't error out.
>>
>> OK I can do a warning but I cannot use the current guard, otherwise it
>> prevents sandbox even building on ARM hosts!
>
> Just change defined(CONFIG_X86) into defined(__x86_64__) ||
> defined(__i386__) then? Maybe the same for the other archs?

I mean print a warning if sandbox is not being build on x86.

What you are suggesting is some sort of ad-hoc architecture detection
in the EFI header file.  If we have a problem here, it should be
solved centrally. I'll add a comment.

Regards,
Simon

[U-Boot] [PATCH v5 06/13] 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 v5: None
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 65f2bcf140..b669a2aa32 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -58,6 +58,9 @@ struct dyn_sym {
 #define R_ABSOLUTE	R_RISCV_64
 #define SYM_INDEX	32
 #endif
+#elif defined(CONFIG_SANDBOX)
+#define R_RELATIVE	8
+#define R_MASK		0xffffffffULL
 #else
 #error Need to add relocation awareness
 #endif
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 06/13] efi: sandbox: Add relocation constants

[Alexander Graf]

On 12.06.18 07:26, 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 v5: None
> 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 65f2bcf140..b669a2aa32 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -58,6 +58,9 @@ struct dyn_sym {
>  #define R_ABSOLUTE	R_RISCV_64
>  #define SYM_INDEX	32
>  #endif
> +#elif defined(CONFIG_SANDBOX)

Same here. Please guard it with an x86_64 check to make sure anyone who
ports sandbox to non-x86 trips over it.


Alex

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

[U-Boot] [PATCH v5 07/13] 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>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

Changes in v5:
- Drop period after "elf" in comment

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 b669a2aa32..786abb46d8 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -28,6 +28,10 @@ static efi_status_t __efi_runtime EFIAPI efi_unimplemented(void);
 static efi_status_t __efi_runtime EFIAPI efi_device_error(void);
 static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);
 
+/*
+ * 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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [U-Boot, v5, 07/13] efi: Add a comment about duplicated ELF constants

[Alexander Graf]

> 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>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v5 08/13] 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 v5: None
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 df58e633d1..d471e6f4a4 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 || RISCV) && OF_LIBFDT
+	depends on (ARM || X86 || RISCV || 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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 09/13] 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 v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()

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 | 87 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 63 insertions(+), 24 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 707d159bac..a9ebde0c75 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -347,6 +347,60 @@ 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)
+{
+	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,
+					      (uintptr_t)test_func,
+					      (uintptr_t)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();
+
+	/* 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;
@@ -425,31 +479,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();
-		/* 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, "\\selftest",
+					 (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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 10/13] 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
WARNING: booting without device tree
Hello, world!
Test passed

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

Changes in v5: None
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 a9ebde0c75..ac80074bc4 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -347,7 +347,6 @@ exit:
 	return ret;
 }
 
-#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 /**
  * bootefi_test_prepare() - prepare to run an EFI test
  *
@@ -399,7 +398,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)
 {
@@ -431,8 +429,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;
 
@@ -477,11 +477,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, "\\selftest",
 					 (uintptr_t)&efi_selftest))
 			return CMD_RET_FAILURE;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index c66252a7dd..0615cfac85 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -442,6 +442,9 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor,
 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 d471e6f4a4..110dcb23c9 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -25,3 +25,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 c6046e36d2..2da28f9c90 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_DM_VIDEO) += efi_gop.o
 obj-$(CONFIG_PARTITIONS) += efi_disk.o
 obj-$(CONFIG_NET) += efi_net.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
+obj-$(CONFIG_BOOTEFI_TEST) += efi_test.o
diff --git a/lib/efi_loader/efi_test.c b/lib/efi_loader/efi_test.c
new file mode 100644
index 0000000000..4b8d49f324
--- /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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Alexander Graf]

On 12.06.18 07:26, Simon Glass wrote:
> This jumps to test code which can call directly into the EFI support. It
> does not need a separate image so it is easy to write tests with it.
> 
> 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
> WARNING: booting without device tree
> Hello, world!
> Test passed
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

From Heinrich's comments it sounded like it wouldn't be hard to make the
selftest work. That sounds more appealing to me to be honest :).


Alex

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

Hi Alex,

On 12 June 2018 at 02:28, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 07:26, Simon Glass wrote:
>> This jumps to test code which can call directly into the EFI support. It
>> does not need a separate image so it is easy to write tests with it.
>>
>> 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
>> WARNING: booting without device tree
>> Hello, world!
>> Test passed
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> From Heinrich's comments it sounded like it wouldn't be hard to make the
> selftest work. That sounds more appealing to me to be honest :).

Yes and in fact my hope was to run the tests automatically as part of
'make tests'

But rather than expanding the scope of this series, can we get this in
first? Having EFI support in sandbox is a substantial step forward.

Regards,
Simon

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Alexander Graf]

On 12.06.18 15:48, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 02:28, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 12.06.18 07:26, Simon Glass wrote:
>>> This jumps to test code which can call directly into the EFI support. It
>>> does not need a separate image so it is easy to write tests with it.
>>>
>>> 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
>>> WARNING: booting without device tree
>>> Hello, world!
>>> Test passed
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> From Heinrich's comments it sounded like it wouldn't be hard to make the
>> selftest work. That sounds more appealing to me to be honest :).
> 
> Yes and in fact my hope was to run the tests automatically as part of
> 'make tests'
> 
> But rather than expanding the scope of this series, can we get this in
> first? Having EFI support in sandbox is a substantial step forward.

I agree that it would be amazing to have it in, I just want to make sure
we're walking into the right direction. And what I want to have is an
easy way to execute EFI binaries from user space :).

Also I don't think that sandbox support is all that far off. Heinrich's
patch should have resolved compilation, no?


Alex

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

Hi Alex,

On 12 June 2018 at 08:11, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 15:48, Simon Glass wrote:
>> Hi Alex,
>>
>> On 12 June 2018 at 02:28, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 12.06.18 07:26, Simon Glass wrote:
>>>> This jumps to test code which can call directly into the EFI support. It
>>>> does not need a separate image so it is easy to write tests with it.
>>>>
>>>> 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
>>>> WARNING: booting without device tree
>>>> Hello, world!
>>>> Test passed
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>> From Heinrich's comments it sounded like it wouldn't be hard to make the
>>> selftest work. That sounds more appealing to me to be honest :).
>>
>> Yes and in fact my hope was to run the tests automatically as part of
>> 'make tests'
>>
>> But rather than expanding the scope of this series, can we get this in
>> first? Having EFI support in sandbox is a substantial step forward.
>
> I agree that it would be amazing to have it in, I just want to make sure
> we're walking into the right direction. And what I want to have is an
> easy way to execute EFI binaries from user space :).

That's a different thing entirely from the purpose of my series. My
series is designed to allow EFI applications to be *linked* with
sandbox and run just like normal C code, with a full unified stack
trace, etc.

I think this is a very useful feature separate from running EFI
binaries in user space.

>
> Also I don't think that sandbox support is all that far off. Heinrich's
> patch should have resolved compilation, no?

I don't know what it entails but Heinrich says there is a memory
alignment problem to resolve. I was able to repeat his FAT failure but
adding his patch and a few other tweaks.

I'm happy to look at this once we have basic sandbox support
available, but if Heinrich wants to take a look, he is welcome to.

Regards,
Simon

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Alexander Graf]

On 12.06.18 23:57, Simon Glass wrote:
> Hi Alex,
> 
> On 12 June 2018 at 08:11, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 12.06.18 15:48, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 12 June 2018 at 02:28, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>> On 12.06.18 07:26, Simon Glass wrote:
>>>>> This jumps to test code which can call directly into the EFI support. It
>>>>> does not need a separate image so it is easy to write tests with it.
>>>>>
>>>>> 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
>>>>> WARNING: booting without device tree
>>>>> Hello, world!
>>>>> Test passed
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> From Heinrich's comments it sounded like it wouldn't be hard to make the
>>>> selftest work. That sounds more appealing to me to be honest :).
>>>
>>> Yes and in fact my hope was to run the tests automatically as part of
>>> 'make tests'
>>>
>>> But rather than expanding the scope of this series, can we get this in
>>> first? Having EFI support in sandbox is a substantial step forward.
>>
>> I agree that it would be amazing to have it in, I just want to make sure
>> we're walking into the right direction. And what I want to have is an
>> easy way to execute EFI binaries from user space :).
> 
> That's a different thing entirely from the purpose of my series. My
> series is designed to allow EFI applications to be *linked* with
> sandbox and run just like normal C code, with a full unified stack
> trace, etc.
> 
> I think this is a very useful feature separate from running EFI
> binaries in user space.

I understand that and I agree that it's useful. I just don't want to
drive us into a corner where it blocks the other use case.


Alex

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

HI Alex,

On 13 June 2018 at 04:08, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.06.18 23:57, Simon Glass wrote:
>> Hi Alex,
>>
>> On 12 June 2018 at 08:11, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 12.06.18 15:48, Simon Glass wrote:
>>>> Hi Alex,
>>>>
>>>> On 12 June 2018 at 02:28, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>> On 12.06.18 07:26, Simon Glass wrote:
>>>>>> This jumps to test code which can call directly into the EFI support. It
>>>>>> does not need a separate image so it is easy to write tests with it.
>>>>>>
>>>>>> 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
>>>>>> WARNING: booting without device tree
>>>>>> Hello, world!
>>>>>> Test passed
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>>> From Heinrich's comments it sounded like it wouldn't be hard to make the
>>>>> selftest work. That sounds more appealing to me to be honest :).
>>>>
>>>> Yes and in fact my hope was to run the tests automatically as part of
>>>> 'make tests'
>>>>
>>>> But rather than expanding the scope of this series, can we get this in
>>>> first? Having EFI support in sandbox is a substantial step forward.
>>>
>>> I agree that it would be amazing to have it in, I just want to make sure
>>> we're walking into the right direction. And what I want to have is an
>>> easy way to execute EFI binaries from user space :).
>>
>> That's a different thing entirely from the purpose of my series. My
>> series is designed to allow EFI applications to be *linked* with
>> sandbox and run just like normal C code, with a full unified stack
>> trace, etc.
>>
>> I think this is a very useful feature separate from running EFI
>> binaries in user space.
>
> I understand that and I agree that it's useful. I just don't want to
> drive us into a corner where it blocks the other use case.

I don't thing it does. Am I missing something?

I take it you'd like to boot grub on sandbox. I imagine that will take
more work, but should be possible.

The primary purpose from my side is to enable easier testing.

Regards,
Simon

[U-Boot] [PATCH v5 10/13] efi: sandbox: Add a simple 'bootefi test' command

[Alexander Graf]

> Am 14.06.2018 um 17:12 schrieb Simon Glass <sjg@chromium.org>:
> 
> HI Alex,
> 
>> On 13 June 2018 at 04:08, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>>> On 12.06.18 23:57, Simon Glass wrote:
>>> Hi Alex,
>>> 
>>>> On 12 June 2018 at 08:11, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> 
>>>>> On 12.06.18 15:48, Simon Glass wrote:
>>>>> Hi Alex,
>>>>> 
>>>>>> On 12 June 2018 at 02:28, Alexander Graf <agraf@suse.de> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 12.06.18 07:26, Simon Glass wrote:
>>>>>>> This jumps to test code which can call directly into the EFI support. It
>>>>>>> does not need a separate image so it is easy to write tests with it.
>>>>>>> 
>>>>>>> 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
>>>>>>> WARNING: booting without device tree
>>>>>>> Hello, world!
>>>>>>> Test passed
>>>>>>> 
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>> 
>>>>>> From Heinrich's comments it sounded like it wouldn't be hard to make the
>>>>>> selftest work. That sounds more appealing to me to be honest :).
>>>>> 
>>>>> Yes and in fact my hope was to run the tests automatically as part of
>>>>> 'make tests'
>>>>> 
>>>>> But rather than expanding the scope of this series, can we get this in
>>>>> first? Having EFI support in sandbox is a substantial step forward.
>>>> 
>>>> I agree that it would be amazing to have it in, I just want to make sure
>>>> we're walking into the right direction. And what I want to have is an
>>>> easy way to execute EFI binaries from user space :).
>>> 
>>> That's a different thing entirely from the purpose of my series. My
>>> series is designed to allow EFI applications to be *linked* with
>>> sandbox and run just like normal C code, with a full unified stack
>>> trace, etc.
>>> 
>>> I think this is a very useful feature separate from running EFI
>>> binaries in user space.
>> 
>> I understand that and I agree that it's useful. I just don't want to
>> drive us into a corner where it blocks the other use case.
> 
> I don't thing it does. Am I missing something?

Anything exposed via efi interfaces has to contain host virtual adresses, as binary payloads are not aware of the virt/phys memory offset.

> I take it you'd like to boot grub on sandbox. I imagine that will take
> more work, but should be possible.

I tried, it almost works. I guess the aarch64 version could even succeed.

> 
> The primary purpose from my side is to enable easier testing.

I agree on that part. So let's make sure both use cases get enabled! :)

Alex

[U-Boot] [PATCH v5 11/13] 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 v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
- Introduce load_options_path to specifyc U-Boot env var for load_options_path

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 | 79 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index ac80074bc4..eb3ca68a86 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -253,6 +253,26 @@ 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 *load_options_path,
+					struct efi_device_path *device_path,
+					struct efi_device_path *image_path)
+{
+	efi_setup_loaded_image(image, 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();
+
+	/* Transfer environment variable as load options */
+	set_load_options(image, load_options_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.
@@ -261,8 +281,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;
 
@@ -283,19 +303,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);
+	ret = bootefi_run_prepare(&image, &obj, "bootargs", device_path,
+				  image_path);
+	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(&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;
@@ -303,10 +317,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: */
@@ -316,8 +330,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;
 	}
 
@@ -329,7 +343,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);
 
@@ -338,11 +352,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;
 }
@@ -358,11 +372,13 @@ exit:
  * @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
+ * @load_options_path: U-Boot environment variable to use as load options
  * @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)
+					 const char *path, ulong test_func,
+					 const char *load_options_path)
 {
 	memset(image, '\0', sizeof(*image));
 	memset(obj, '\0', sizeof(*obj));
@@ -371,18 +387,8 @@ static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image,
 					      (uintptr_t)test_func,
 					      (uintptr_t)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();
-
-	/* Transfer environment variable efi_selftest as load options */
-	set_load_options(image, "efi_selftest");
-
-	return 0;
+	return bootefi_run_prepare(image, obj, load_options_path,
+				   bootefi_device_path, bootefi_image_path);
 }
 
 /**
@@ -481,7 +487,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		int ret;
 
 		if (bootefi_test_prepare(&image, &obj, "\\test",
-					 (ulong)&efi_test))
+					 (ulong)&efi_test, "efi_test"))
 			return CMD_RET_FAILURE;
 
 		ret = efi_test(&image, &systab);
@@ -497,7 +503,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	if (!strcmp(argv[1], "selftest")) {
 		if (bootefi_test_prepare(&image, &obj, "\\selftest",
-					 (uintptr_t)&efi_selftest))
+					 (uintptr_t)&efi_selftest),
+					 "efi_selftest")
 			return CMD_RET_FAILURE;
 
 		/* Execute the test */
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 12/13] 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 v5:
- Rebase to master

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 eb3ca68a86..7f767aed42 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -273,6 +273,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.
@@ -355,8 +369,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;
 }
@@ -391,20 +404,6 @@ static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image,
 				   bootefi_device_path, 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;
@@ -491,7 +490,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;
@@ -509,7 +508,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.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 13/13] buildman: Add a --boards option to specify particular boards to build

[Simon Glass]

At present 'buildman sandbox' will build all 5 boards for the sandbox
architecture rather than the single board 'sandbox'. The only current way
to exclude sandbox_spl, sandbox_noblk, etc. is to use -x which is a bit
clumbsy.

Add a --boards option to allow individual build targets to be specified.

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 tools/buildman/README     | 12 +++++++++++-
 tools/buildman/board.py   | 28 ++++++++++++++++++++++++----
 tools/buildman/cmdline.py |  4 +++-
 tools/buildman/control.py | 20 +++++++++++++++++---
 tools/buildman/test.py    | 31 +++++++++++++++++--------------
 5 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/tools/buildman/README b/tools/buildman/README
index 76601902cb..5a709c6ff9 100644
--- a/tools/buildman/README
+++ b/tools/buildman/README
@@ -114,6 +114,10 @@ a few commits or boards, it will be pretty slow. As a tip, if you don't
 plan to use your machine for anything else, you can use -T to increase the
 number of threads beyond the default.
 
+
+Selecting which boards to build
+===============================
+
 Buildman lets you build all boards, or a subset. Specify the subset by passing
 command-line arguments that list the desired board name, architecture name,
 SOC name, or anything else in the boards.cfg file. Multiple arguments are
@@ -138,11 +142,17 @@ You can also use -x to specifically exclude some boards. For example:
 means to build all arm boards except nvidia, freescale and anything ending
 with 'ball'.
 
+For building specific boards you can use the --boards option, which takes a
+comma-separated list of board target names and be used multiple times on
+the command line:
+
+   buidman --boards sandbox,snow --boards
+
 It is convenient to use the -n option to see what will be built based on
 the subset given. Use -v as well to get an actual list of boards.
 
 Buildman does not store intermediate object files. It optionally copies
-the binary output into a directory when a build is successful. Size
+the binary output into a directory when a build is successful (-k). Size
 information is always recorded. It needs a fair bit of disk space to work,
 typically 250MB per thread.
 
diff --git a/tools/buildman/board.py b/tools/buildman/board.py
index 272bac0c21..2a1d021574 100644
--- a/tools/buildman/board.py
+++ b/tools/buildman/board.py
@@ -237,20 +237,30 @@ class Boards:
             terms.append(term)
         return terms
 
-    def SelectBoards(self, args, exclude=[]):
+    def SelectBoards(self, args, exclude=[], boards=None):
         """Mark boards selected based on args
 
+        Normally either boards (an explicit list of boards) or args (a list of
+        terms to match against) is used. It is possible to specify both, in
+        which case they are additive.
+
+        If boards and args are both empty, all boards are selected.
+
         Args:
             args: List of strings specifying boards to include, either named,
                   or by their target, architecture, cpu, vendor or soc. If
                   empty, all boards are selected.
             exclude: List of boards to exclude, regardless of 'args'
+            boards: List of boards to build
 
         Returns:
-            Dictionary which holds the list of boards which were selected
-            due to each argument, arranged by argument.
+            Tuple
+                Dictionary which holds the list of boards which were selected
+                    due to each argument, arranged by argument.
+                List of errors found
         """
         result = {}
+        warnings = []
         terms = self._BuildTerms(args)
 
         result['all'] = []
@@ -261,6 +271,7 @@ class Boards:
         for expr in exclude:
             exclude_list.append(Expr(expr))
 
+        found = []
         for board in self._boards:
             matching_term = None
             build_it = False
@@ -271,6 +282,10 @@ class Boards:
                         matching_term = str(term)
                         build_it = True
                         break
+            elif boards:
+                if board.target in boards:
+                    build_it = True
+                    found.append(board.target)
             else:
                 build_it = True
 
@@ -286,4 +301,9 @@ class Boards:
                     result[matching_term].append(board.target)
                 result['all'].append(board.target)
 
-        return result
+        if boards:
+            remaining = set(boards) - set(found)
+            if remaining:
+                warnings.append('Boards not found: %s\n' % ', '.join(remaining))
+
+        return result, warnings
diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index e493b1ac4a..49a8a13118 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -18,6 +18,8 @@ def ParseArgs():
     parser.add_option('-B', '--bloat', dest='show_bloat',
           action='store_true', default=False,
           help='Show changes in function code size for each board')
+    parser.add_option('--boards', type='string', action='append',
+          help='List of board names to build separated by comma')
     parser.add_option('-c', '--count', dest='count', type='int',
           default=-1, help='Run build on the top n commits')
     parser.add_option('-C', '--force-reconfig', dest='force_reconfig',
@@ -102,7 +104,7 @@ def ParseArgs():
           type='string', action='append',
           help='Specify a list of boards to exclude, separated by comma')
 
-    parser.usage += """
+    parser.usage += """ [list of target/arch/cpu/board/vendor/soc to build]
 
     Build U-Boot for all commits in a branch. Use -n to do a dry run"""
 
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index bc0819784f..96f8ccfe07 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -41,7 +41,8 @@ def GetActionSummary(is_summary, commits, selected, options):
             GetPlural(options.threads), options.jobs, GetPlural(options.jobs))
     return str
 
-def ShowActions(series, why_selected, boards_selected, builder, options):
+def ShowActions(series, why_selected, boards_selected, builder, options,
+                board_warnings):
     """Display a list of actions that we would take, if not a dry run.
 
     Args:
@@ -55,6 +56,7 @@ def ShowActions(series, why_selected, boards_selected, builder, options):
                 value is Board object
         builder: The builder that will be used to build the commits
         options: Command line options object
+        board_warnings: List of warnings obtained from board selected
     """
     col = terminal.Color()
     print 'Dry run, so not doing much. But I would do this:'
@@ -79,6 +81,9 @@ def ShowActions(series, why_selected, boards_selected, builder, options):
                 print '   %s' % ' '.join(why_selected[arg])
     print ('Total boards to build for each commit: %d\n' %
             len(why_selected['all']))
+    if board_warnings:
+        for warning in board_warnings:
+            print col.Color(col.YELLOW, warning)
 
 def CheckOutputDir(output_dir):
     """Make sure that the output directory is not within the current directory
@@ -210,7 +215,15 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
         for arg in options.exclude:
             exclude += arg.split(',')
 
-    why_selected = boards.SelectBoards(args, exclude)
+
+    if options.boards:
+        requested_boards = []
+        for b in options.boards:
+            requested_boards += b.split(',')
+    else:
+        requested_boards = None
+    why_selected, board_warnings = boards.SelectBoards(args, exclude,
+                                                       requested_boards)
     selected = boards.GetSelected()
     if not len(selected):
         sys.exit(col.Color(col.RED, 'No matching boards found'))
@@ -292,7 +305,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None,
 
     # For a dry run, just show our actions as a sanity check
     if options.dry_run:
-        ShowActions(series, why_selected, selected, builder, options)
+        ShowActions(series, why_selected, selected, builder, options,
+                    board_warnings)
     else:
         builder.force_build = options.force_build
         builder.force_build_failures = options.force_build_failures
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index c36bcdf6fb..e9a4ffa024 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -312,60 +312,63 @@ class TestBuild(unittest.TestCase):
     def testBoardSingle(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['sandbox']),
-                         {'all': ['board4'], 'sandbox': ['board4']})
+                         ({'all': ['board4'], 'sandbox': ['board4']}, []))
 
     def testBoardArch(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['arm']),
-                         {'all': ['board0', 'board1'],
-                          'arm': ['board0', 'board1']})
+                         ({'all': ['board0', 'board1'],
+                          'arm': ['board0', 'board1']}, []))
 
     def testBoardArchSingle(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['arm sandbox']),
-                         {'sandbox': ['board4'],
+                         ({'sandbox': ['board4'],
                           'all': ['board0', 'board1', 'board4'],
-                          'arm': ['board0', 'board1']})
+                          'arm': ['board0', 'board1']}, []))
 
 
     def testBoardArchSingleMultiWord(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['arm', 'sandbox']),
-                         {'sandbox': ['board4'], 'all': ['board0', 'board1', 'board4'], 'arm': ['board0', 'board1']})
+                         ({'sandbox': ['board4'],
+                          'all': ['board0', 'board1', 'board4'],
+                          'arm': ['board0', 'board1']}, []))
 
     def testBoardSingleAnd(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['Tester & arm']),
-                         {'Tester&arm': ['board0', 'board1'], 'all': ['board0', 'board1']})
+                         ({'Tester&arm': ['board0', 'board1'],
+                           'all': ['board0', 'board1']}, []))
 
     def testBoardTwoAnd(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['Tester', '&', 'arm',
                                                    'Tester' '&', 'powerpc',
                                                    'sandbox']),
-                         {'sandbox': ['board4'],
+                         ({'sandbox': ['board4'],
                           'all': ['board0', 'board1', 'board2', 'board3',
                                   'board4'],
                           'Tester&powerpc': ['board2', 'board3'],
-                          'Tester&arm': ['board0', 'board1']})
+                          'Tester&arm': ['board0', 'board1']}, []))
 
     def testBoardAll(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards([]),
-                         {'all': ['board0', 'board1', 'board2', 'board3',
-                                  'board4']})
+                         ({'all': ['board0', 'board1', 'board2', 'board3',
+                                  'board4']}, []))
 
     def testBoardRegularExpression(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['T.*r&^Po']),
-                         {'all': ['board2', 'board3'],
-                          'T.*r&^Po': ['board2', 'board3']})
+                         ({'all': ['board2', 'board3'],
+                          'T.*r&^Po': ['board2', 'board3']}, []))
 
     def testBoardDuplicate(self):
         """Test single board selection"""
         self.assertEqual(self.boards.SelectBoards(['sandbox sandbox',
                                                    'sandbox']),
-                         {'all': ['board4'], 'sandbox': ['board4']})
+                         ({'all': ['board4'], 'sandbox': ['board4']}, []))
     def CheckDirs(self, build, dirname):
         self.assertEqual('base%s' % dirname, build._GetOutputDir(1))
         self.assertEqual('base%s/fred' % dirname,
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v5 13/13] buildman: Add a --boards option to specify particular boards to build

[Simon Glass]

On 11 June 2018 at 23:26, Simon Glass <sjg@chromium.org> wrote:
>
> At present 'buildman sandbox' will build all 5 boards for the sandbox
> architecture rather than the single board 'sandbox'. The only current way
> to exclude sandbox_spl, sandbox_noblk, etc. is to use -x which is a bit
> clumbsy.
>
> Add a --boards option to allow individual build targets to be specified.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  tools/buildman/README     | 12 +++++++++++-
>  tools/buildman/board.py   | 28 ++++++++++++++++++++++++----
>  tools/buildman/cmdline.py |  4 +++-
>  tools/buildman/control.py | 20 +++++++++++++++++---
>  tools/buildman/test.py    | 31 +++++++++++++++++--------------
>  5 files changed, 72 insertions(+), 23 deletions(-)

Applied to u-boot-dm.