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

[U-Boot] [PATCH v6 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 v6:
- Warn about building sandbox EFI support on something other than __x86_64__

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()
  Revert "buildman: Extract environment as part of each build"

 cmd/bootefi.c                   | 153 ++++++++++++++++++++++----------
 include/config_distro_bootcmd.h |  13 +++
 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    |  16 ++++
 lib/efi_loader/efi_test.c       |  16 ++++
 lib/efi_selftest/Kconfig        |   2 +-
 lib/smbios.c                    |  32 +++++--
 tools/buildman/builderthread.py |  10 ---
 tools/buildman/func_test.py     |   5 --
 13 files changed, 213 insertions(+), 86 deletions(-)
 create mode 100644 lib/efi_loader/efi_test.c

-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v6 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 v6: None
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 v6 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 v6: None
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 v6 02/13] efi: Init the 'rows' and 'cols' variables

[Alexander Graf]

On 06/13/2018 04:37 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>

I thought this patch was obsolete now?

Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 04:11, Alexander Graf <agraf@suse.de> wrote:
> On 06/13/2018 04:37 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>
>
>
> I thought this patch was obsolete now?
>
> Alex
>

Yes Heinrich pointed out that it isn't needed now.

Regards,
Simon

[U-Boot] [PATCH v6 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 v6: None
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 v6 03/13] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 06/13/2018 04:37 AM, Simon Glass wrote:
> With sandbox the U-Boot code is not mapped into the sandbox memory range
> so does not need to be excluded when allocating EFI memory. Update the EFI
> memory init code to take account of that.
>
> 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 v6: None
> 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);

This is still the wrong spot. We don't want the conversion to happen 
when going from an EFI internal address to an allocation, but when 
determining which addresses are usable in the first place.

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

Didn't we want to have these in a function?


Alex

> +
> +		/* Add Runtime Services */
> +		runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
> +		runtime_end = (ulong)&__efi_runtime_stop;
> +		runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
> +		runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
> +		efi_add_memory_map(runtime_start, runtime_pages,
> +				   EFI_RUNTIME_SERVICES_CODE, false);
> +	}
>   
>   #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
>   	/* Request a 32bit 64MB bounce buffer region */

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>
>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>> so does not need to be excluded when allocating EFI memory. Update the EFI
>> memory init code to take account of that.
>>
>> 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 v6: None
>> 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);
>
>
> This is still the wrong spot. We don't want the conversion to happen when
> going from an EFI internal address to an allocation, but when determining
> which addresses are usable in the first place.

There seem to be two ways to do this:

1. Record addresses (ulong) in the EFI tables and use map_sysmem()
before returning an address in the allocator
2. Store pointers in the EFI tables using map_sysmem(), then do no
mapping in the allocator

I've gone with option 1 since:

- the EFI addresses are not pointers
- it makes sandbox consistent with other architectures which use an
address rather than a pointer in EFI tables
- we don't normally do pointer arithmetic on the results of map_sysmem()
- we normally map the memory when it is used rather than when it is set up

I think you are asking for option 2. I think that would get very
confusing. The addresses where things actually end up in sandbox are
best kept to sandbox.

Overall I feel that you are either missing the point of sandbox
addressing, or don't agree with how it is done. But it does work
pretty well and we don't get a lot of confusion with sandbox pointers
since we typically use the address until the last moment.

>
>>                 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);
>
>
> Didn't we want to have these in a function?

Yes but I forgot. Will do for v7.

Regards,
Simon

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

[Alexander Graf]

On 06/14/2018 02:58 PM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>> so does not need to be excluded when allocating EFI memory. Update the EFI
>>> memory init code to take account of that.
>>>
>>> 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 v6: None
>>> 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);
>>
>> This is still the wrong spot. We don't want the conversion to happen when
>> going from an EFI internal address to an allocation, but when determining
>> which addresses are usable in the first place.
> There seem to be two ways to do this:
>
> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
> before returning an address in the allocator
> 2. Store pointers in the EFI tables using map_sysmem(), then do no
> mapping in the allocator
>
> I've gone with option 1 since:
>
> - the EFI addresses are not pointers
> - it makes sandbox consistent with other architectures which use an
> address rather than a pointer in EFI tables
> - we don't normally do pointer arithmetic on the results of map_sysmem()
> - we normally map the memory when it is used rather than when it is set up
>
> I think you are asking for option 2. I think that would get very
> confusing. The addresses where things actually end up in sandbox are
> best kept to sandbox.
>
> Overall I feel that you are either missing the point of sandbox
> addressing, or don't agree with how it is done. But it does work
> pretty well and we don't get a lot of confusion with sandbox pointers
> since we typically use the address until the last moment.

I've assembled a quick tree for version 2. With that I'm able to run a 
simple hello world efi application. Grub refuses to start because it 
wants memory in the low 32bit and also emits random PIO accessing 
functions, which obviously don't work work from user space.

But overall, I think this is the right path to tackle this:

   https://github.com/agraf/u-boot/tree/efi-sandbox


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>
>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>> EFI
>>>> memory init code to take account of that.
>>>>
>>>> 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 v6: None
>>>> 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);
>>>
>>>
>>> This is still the wrong spot. We don't want the conversion to happen when
>>> going from an EFI internal address to an allocation, but when determining
>>> which addresses are usable in the first place.
>>
>> There seem to be two ways to do this:
>>
>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>> before returning an address in the allocator
>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>> mapping in the allocator
>>
>> I've gone with option 1 since:
>>
>> - the EFI addresses are not pointers
>> - it makes sandbox consistent with other architectures which use an
>> address rather than a pointer in EFI tables
>> - we don't normally do pointer arithmetic on the results of map_sysmem()
>> - we normally map the memory when it is used rather than when it is set up
>>
>> I think you are asking for option 2. I think that would get very
>> confusing. The addresses where things actually end up in sandbox are
>> best kept to sandbox.
>>
>> Overall I feel that you are either missing the point of sandbox
>> addressing, or don't agree with how it is done. But it does work
>> pretty well and we don't get a lot of confusion with sandbox pointers
>> since we typically use the address until the last moment.
>
>
> I've assembled a quick tree for version 2. With that I'm able to run a
> simple hello world efi application. Grub refuses to start because it wants
> memory in the low 32bit and also emits random PIO accessing functions, which
> obviously don't work work from user space.
>
> But overall, I think this is the right path to tackle this:
>
>   https://github.com/agraf/u-boot/tree/efi-sandbox

What do you mean by version 2? It looks like you've added one patch,
so will you send that to the list?

Anyway, I hope I can convince you of the above, the way sandbox memory works.

Regards,
Simon

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

[Alexander Graf]

> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>> 
>>> Hi Alex,
>>> 
>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>> 
>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>>> EFI
>>>>> memory init code to take account of that.
>>>>> 
>>>>> 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 v6: None
>>>>> 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);
>>>> 
>>>> 
>>>> This is still the wrong spot. We don't want the conversion to happen when
>>>> going from an EFI internal address to an allocation, but when determining
>>>> which addresses are usable in the first place.
>>> 
>>> There seem to be two ways to do this:
>>> 
>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>> before returning an address in the allocator
>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>> mapping in the allocator
>>> 
>>> I've gone with option 1 since:
>>> 
>>> - the EFI addresses are not pointers
>>> - it makes sandbox consistent with other architectures which use an
>>> address rather than a pointer in EFI tables
>>> - we don't normally do pointer arithmetic on the results of map_sysmem()
>>> - we normally map the memory when it is used rather than when it is set up
>>> 
>>> I think you are asking for option 2. I think that would get very
>>> confusing. The addresses where things actually end up in sandbox are
>>> best kept to sandbox.
>>> 
>>> Overall I feel that you are either missing the point of sandbox
>>> addressing, or don't agree with how it is done. But it does work
>>> pretty well and we don't get a lot of confusion with sandbox pointers
>>> since we typically use the address until the last moment.
>> 
>> 
>> I've assembled a quick tree for version 2. With that I'm able to run a
>> simple hello world efi application. Grub refuses to start because it wants
>> memory in the low 32bit and also emits random PIO accessing functions, which
>> obviously don't work work from user space.
>> 
>> But overall, I think this is the right path to tackle this:
>> 
>>  https://github.com/agraf/u-boot/tree/efi-sandbox
> 
> What do you mean by version 2?

Option 2 is what you called it. It's the only option we have to make efi binaries work.

> It looks like you've added one patch,
> so will you send that to the list?

It's more than 1 patch. And yes, I'll send them.

> 
> Anyway, I hope I can convince you of the above, the way sandbox memory works.

I still dislike option 1 :)

The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> > Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
> >
> > Hi Alex,
> >
> >> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
> >>> On 06/14/2018 02:58 PM, Simon Glass wrote:
> >>>
> >>> Hi Alex,
> >>>
> >>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
> >>>>
> >>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
> >>>>>
> >>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
> >>>>> so does not need to be excluded when allocating EFI memory. Update the
> >>>>> EFI
> >>>>> memory init code to take account of that.
> >>>>>
> >>>>> 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 v6: None
> >>>>> 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);
> >>>>
> >>>>
> >>>> This is still the wrong spot. We don't want the conversion to happen when
> >>>> going from an EFI internal address to an allocation, but when determining
> >>>> which addresses are usable in the first place.
> >>>
> >>> There seem to be two ways to do this:
> >>>
> >>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
> >>> before returning an address in the allocator
> >>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
> >>> mapping in the allocator
> >>>
> >>> I've gone with option 1 since:
> >>>
> >>> - the EFI addresses are not pointers
> >>> - it makes sandbox consistent with other architectures which use an
> >>> address rather than a pointer in EFI tables
> >>> - we don't normally do pointer arithmetic on the results of map_sysmem()
> >>> - we normally map the memory when it is used rather than when it is set up
> >>>
> >>> I think you are asking for option 2. I think that would get very
> >>> confusing. The addresses where things actually end up in sandbox are
> >>> best kept to sandbox.
> >>>
> >>> Overall I feel that you are either missing the point of sandbox
> >>> addressing, or don't agree with how it is done. But it does work
> >>> pretty well and we don't get a lot of confusion with sandbox pointers
> >>> since we typically use the address until the last moment.
> >>
> >>
> >> I've assembled a quick tree for version 2. With that I'm able to run a
> >> simple hello world efi application. Grub refuses to start because it wants
> >> memory in the low 32bit and also emits random PIO accessing functions, which
> >> obviously don't work work from user space.
> >>
> >> But overall, I think this is the right path to tackle this:
> >>
> >>  https://github.com/agraf/u-boot/tree/efi-sandbox
> >
> > What do you mean by version 2?
>
> Option 2 is what you called it. It's the only option we have to make efi binaries work.
>
> > It looks like you've added one patch,
> > so will you send that to the list?
>
> It's more than 1 patch. And yes, I'll send them.
>
> >
> > Anyway, I hope I can convince you of the above, the way sandbox memory works.
>
> I still dislike option 1 :)
>
> The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.

OK so you mean they don't have to allocate memory before using it? How
then do you make sure that there is no conflict? I thought EFI was an
API?

Regards,
Simon

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

[Alexander Graf]

> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>> 
>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>> 
>>> Hi Alex,
>>> 
>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>> 
>>>>> Hi Alex,
>>>>> 
>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>> 
>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>> 
>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>>>>> EFI
>>>>>>> memory init code to take account of that.
>>>>>>> 
>>>>>>> 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 v6: None
>>>>>>> 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);
>>>>>> 
>>>>>> 
>>>>>> This is still the wrong spot. We don't want the conversion to happen when
>>>>>> going from an EFI internal address to an allocation, but when determining
>>>>>> which addresses are usable in the first place.
>>>>> 
>>>>> There seem to be two ways to do this:
>>>>> 
>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>> before returning an address in the allocator
>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>> mapping in the allocator
>>>>> 
>>>>> I've gone with option 1 since:
>>>>> 
>>>>> - the EFI addresses are not pointers
>>>>> - it makes sandbox consistent with other architectures which use an
>>>>> address rather than a pointer in EFI tables
>>>>> - we don't normally do pointer arithmetic on the results of map_sysmem()
>>>>> - we normally map the memory when it is used rather than when it is set up
>>>>> 
>>>>> I think you are asking for option 2. I think that would get very
>>>>> confusing. The addresses where things actually end up in sandbox are
>>>>> best kept to sandbox.
>>>>> 
>>>>> Overall I feel that you are either missing the point of sandbox
>>>>> addressing, or don't agree with how it is done. But it does work
>>>>> pretty well and we don't get a lot of confusion with sandbox pointers
>>>>> since we typically use the address until the last moment.
>>>> 
>>>> 
>>>> I've assembled a quick tree for version 2. With that I'm able to run a
>>>> simple hello world efi application. Grub refuses to start because it wants
>>>> memory in the low 32bit and also emits random PIO accessing functions, which
>>>> obviously don't work work from user space.
>>>> 
>>>> But overall, I think this is the right path to tackle this:
>>>> 
>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>> 
>>> What do you mean by version 2?
>> 
>> Option 2 is what you called it. It's the only option we have to make efi binaries work.
>> 
>>> It looks like you've added one patch,
>>> so will you send that to the list?
>> 
>> It's more than 1 patch. And yes, I'll send them.
>> 
>>> 
>>> Anyway, I hope I can convince you of the above, the way sandbox memory works.
>> 
>> I still dislike option 1 :)
>> 
>> The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.
> 
> OK so you mean they don't have to allocate memory before using it? How
> then do you make sure that there is no conflict? I thought EFI was an
> API?

You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.

In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.

Alex

> 
> Regards,
> Simon

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>
>
>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>
>> Hi Alex,
>>
>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>
>>>> Hi Alex,
>>>>
>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>>>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>>>>>> EFI
>>>>>>>> memory init code to take account of that.
>>>>>>>>
>>>>>>>> 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 v6: None
>>>>>>>> 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);
>>>>>>>
>>>>>>>
>>>>>>> This is still the wrong spot. We don't want the conversion to happen when
>>>>>>> going from an EFI internal address to an allocation, but when determining
>>>>>>> which addresses are usable in the first place.
>>>>>>
>>>>>> There seem to be two ways to do this:
>>>>>>
>>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>>> before returning an address in the allocator
>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>>> mapping in the allocator
>>>>>>
>>>>>> I've gone with option 1 since:
>>>>>>
>>>>>> - the EFI addresses are not pointers
>>>>>> - it makes sandbox consistent with other architectures which use an
>>>>>> address rather than a pointer in EFI tables
>>>>>> - we don't normally do pointer arithmetic on the results of map_sysmem()
>>>>>> - we normally map the memory when it is used rather than when it is set up
>>>>>>
>>>>>> I think you are asking for option 2. I think that would get very
>>>>>> confusing. The addresses where things actually end up in sandbox are
>>>>>> best kept to sandbox.
>>>>>>
>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>> pretty well and we don't get a lot of confusion with sandbox pointers
>>>>>> since we typically use the address until the last moment.
>>>>>
>>>>>
>>>>> I've assembled a quick tree for version 2. With that I'm able to run a
>>>>> simple hello world efi application. Grub refuses to start because it wants
>>>>> memory in the low 32bit and also emits random PIO accessing functions, which
>>>>> obviously don't work work from user space.
>>>>>
>>>>> But overall, I think this is the right path to tackle this:
>>>>>
>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>
>>>> What do you mean by version 2?
>>>
>>> Option 2 is what you called it. It's the only option we have to make efi binaries work.
>>>
>>>> It looks like you've added one patch,
>>>> so will you send that to the list?
>>>
>>> It's more than 1 patch. And yes, I'll send them.
>>>
>>>>
>>>> Anyway, I hope I can convince you of the above, the way sandbox memory works.
>>>
>>> I still dislike option 1 :)
>>>
>>> The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.
>>
>> OK so you mean they don't have to allocate memory before using it? How
>> then do you make sure that there is no conflict? I thought EFI was an
>> API?
>
> You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.
>

Can you please point me to that? Are you referring to this call?

static efi_status_t EFIAPI efi_get_memory_map_ext(
        efi_uintn_t *memory_map_size,
        struct efi_mem_desc *memory_map,
        efi_uintn_t *map_key,
        efi_uintn_t *descriptor_size,
        uint32_t *descriptor_version)

If so, we can still support that.

> In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.

I see this function:

efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)

So there appears to be no way for people to specify the address they
want. Is there another call somewhere?

NOTE: I am not proposing that the address is made available to the EFI
application - that would always see the sandbox pointer.

Regards,
Simon

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

[Alexander Graf]

On 06/14/2018 05:53 PM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>
>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>
>>> Hi Alex,
>>>
>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>>>>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>>>>>>> EFI
>>>>>>>>> memory init code to take account of that.
>>>>>>>>>
>>>>>>>>> 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 v6: None
>>>>>>>>> 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);
>>>>>>>>
>>>>>>>> This is still the wrong spot. We don't want the conversion to happen when
>>>>>>>> going from an EFI internal address to an allocation, but when determining
>>>>>>>> which addresses are usable in the first place.
>>>>>>> There seem to be two ways to do this:
>>>>>>>
>>>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>>>> before returning an address in the allocator
>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>>>> mapping in the allocator
>>>>>>>
>>>>>>> I've gone with option 1 since:
>>>>>>>
>>>>>>> - the EFI addresses are not pointers
>>>>>>> - it makes sandbox consistent with other architectures which use an
>>>>>>> address rather than a pointer in EFI tables
>>>>>>> - we don't normally do pointer arithmetic on the results of map_sysmem()
>>>>>>> - we normally map the memory when it is used rather than when it is set up
>>>>>>>
>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>> confusing. The addresses where things actually end up in sandbox are
>>>>>>> best kept to sandbox.
>>>>>>>
>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>> pretty well and we don't get a lot of confusion with sandbox pointers
>>>>>>> since we typically use the address until the last moment.
>>>>>>
>>>>>> I've assembled a quick tree for version 2. With that I'm able to run a
>>>>>> simple hello world efi application. Grub refuses to start because it wants
>>>>>> memory in the low 32bit and also emits random PIO accessing functions, which
>>>>>> obviously don't work work from user space.
>>>>>>
>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>
>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>> What do you mean by version 2?
>>>> Option 2 is what you called it. It's the only option we have to make efi binaries work.
>>>>
>>>>> It looks like you've added one patch,
>>>>> so will you send that to the list?
>>>> It's more than 1 patch. And yes, I'll send them.
>>>>
>>>>> Anyway, I hope I can convince you of the above, the way sandbox memory works.
>>>> I still dislike option 1 :)
>>>>
>>>> The reason is simple: The efi memory map is available to efi payloads. It's perfectly legal for them to do a static allocation at a particular address. We also share a lot of (host) pointers for callbacks and structs already with efi applications, so there is no real point to have a split brain situation between u-boot and host pointers.
>>> OK so you mean they don't have to allocate memory before using it? How
>>> then do you make sure that there is no conflict? I thought EFI was an
>>> API?
>> You allocate it, but payloads expect that the address you pass in as address you allocate at is the pointer/address that gets returned.
>>
> Can you please point me to that? Are you referring to this call?
>
> static efi_status_t EFIAPI efi_get_memory_map_ext(
>          efi_uintn_t *memory_map_size,
>          struct efi_mem_desc *memory_map,
>          efi_uintn_t *map_key,
>          efi_uintn_t *descriptor_size,
>          uint32_t *descriptor_version)
>
> If so, we can still support that.
>
>> In a nutshell, what you're proposing is that the efi quivalent of mmap(addr, MAP_FIXED) returns something != addr. That will break things.
> I see this function:
>
> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>
> So there appears to be no way for people to specify the address they
> want. Is there another call somewhere?

You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even 
EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you 
receive from efi_get_memory_map().

> NOTE: I am not proposing that the address is made available to the EFI
> application - that would always see the sandbox pointer.

In that case it's *much* easier to just always store host pointers in 
the efi memory map. Everything else will be very intrusive.


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>>>>>>> range
>>>>>>>>>> so does not need to be excluded when allocating EFI memory. Update
>>>>>>>>>> the
>>>>>>>>>> EFI
>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>
>>>>>>>>>> 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 v6: None
>>>>>>>>>> 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);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>> happen when
>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>> determining
>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>
>>>>>>>> There seem to be two ways to do this:
>>>>>>>>
>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>>>>> before returning an address in the allocator
>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>>>>> mapping in the allocator
>>>>>>>>
>>>>>>>> I've gone with option 1 since:
>>>>>>>>
>>>>>>>> - the EFI addresses are not pointers
>>>>>>>> - it makes sandbox consistent with other architectures which use an
>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>> map_sysmem()
>>>>>>>> - we normally map the memory when it is used rather than when it is
>>>>>>>> set up
>>>>>>>>
>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>> confusing. The addresses where things actually end up in sandbox are
>>>>>>>> best kept to sandbox.
>>>>>>>>
>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>> pointers
>>>>>>>> since we typically use the address until the last moment.
>>>>>>>
>>>>>>>
>>>>>>> I've assembled a quick tree for version 2. With that I'm able to run
>>>>>>> a
>>>>>>> simple hello world efi application. Grub refuses to start because it
>>>>>>> wants
>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>> functions, which
>>>>>>> obviously don't work work from user space.
>>>>>>>
>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>
>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>
>>>>>> What do you mean by version 2?
>>>>>
>>>>> Option 2 is what you called it. It's the only option we have to make
>>>>> efi binaries work.
>>>>>
>>>>>> It looks like you've added one patch,
>>>>>> so will you send that to the list?
>>>>>
>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>
>>>>>> Anyway, I hope I can convince you of the above, the way sandbox memory
>>>>>> works.
>>>>>
>>>>> I still dislike option 1 :)
>>>>>
>>>>> The reason is simple: The efi memory map is available to efi payloads.
>>>>> It's perfectly legal for them to do a static allocation at a particular
>>>>> address. We also share a lot of (host) pointers for callbacks and structs
>>>>> already with efi applications, so there is no real point to have a split
>>>>> brain situation between u-boot and host pointers.
>>>>
>>>> OK so you mean they don't have to allocate memory before using it? How
>>>> then do you make sure that there is no conflict? I thought EFI was an
>>>> API?
>>>
>>> You allocate it, but payloads expect that the address you pass in as
>>> address you allocate at is the pointer/address that gets returned.
>>>
>> Can you please point me to that? Are you referring to this call?
>>
>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>          efi_uintn_t *memory_map_size,
>>          struct efi_mem_desc *memory_map,
>>          efi_uintn_t *map_key,
>>          efi_uintn_t *descriptor_size,
>>          uint32_t *descriptor_version)
>>
>> If so, we can still support that.
>>
>>> In a nutshell, what you're proposing is that the efi quivalent of
>>> mmap(addr, MAP_FIXED) returns something != addr. That will break things.
>>
>> I see this function:
>>
>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>> **buffer)
>>
>> So there appears to be no way for people to specify the address they
>> want. Is there another call somewhere?
>
>
> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you receive
> from efi_get_memory_map().

That's fine though, I'm not worried about that. It will work correctly, right?

>
>> NOTE: I am not proposing that the address is made available to the EFI
>> application - that would always see the sandbox pointer.
>
>
> In that case it's *much* easier to just always store host pointers in the
> efi memory map. Everything else will be very intrusive.

Yes there will be map_sysmem() and map_to_sysmem() calls at each
boundary. That 'intrusion' is what we have everywhere in sandbox and
it is how things work. But the nice thing is that the tables don't end
up with 'private' sandbox addresses in them. Also see me comments
above.

Regards,
Simon

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

[Alexander Graf]

On 06/14/2018 06:13 PM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>
>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>>>>>>>> range
>>>>>>>>>>> so does not need to be excluded when allocating EFI memory. Update
>>>>>>>>>>> the
>>>>>>>>>>> EFI
>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>
>>>>>>>>>>> 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 v6: None
>>>>>>>>>>> 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);
>>>>>>>>>>
>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>> happen when
>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>> determining
>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>
>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>>>>>> before returning an address in the allocator
>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>>>>>> mapping in the allocator
>>>>>>>>>
>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>
>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>> - it makes sandbox consistent with other architectures which use an
>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>> map_sysmem()
>>>>>>>>> - we normally map the memory when it is used rather than when it is
>>>>>>>>> set up
>>>>>>>>>
>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>> confusing. The addresses where things actually end up in sandbox are
>>>>>>>>> best kept to sandbox.
>>>>>>>>>
>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>> pointers
>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>
>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to run
>>>>>>>> a
>>>>>>>> simple hello world efi application. Grub refuses to start because it
>>>>>>>> wants
>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>> functions, which
>>>>>>>> obviously don't work work from user space.
>>>>>>>>
>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>
>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>> What do you mean by version 2?
>>>>>> Option 2 is what you called it. It's the only option we have to make
>>>>>> efi binaries work.
>>>>>>
>>>>>>> It looks like you've added one patch,
>>>>>>> so will you send that to the list?
>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>
>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox memory
>>>>>>> works.
>>>>>> I still dislike option 1 :)
>>>>>>
>>>>>> The reason is simple: The efi memory map is available to efi payloads.
>>>>>> It's perfectly legal for them to do a static allocation at a particular
>>>>>> address. We also share a lot of (host) pointers for callbacks and structs
>>>>>> already with efi applications, so there is no real point to have a split
>>>>>> brain situation between u-boot and host pointers.
>>>>> OK so you mean they don't have to allocate memory before using it? How
>>>>> then do you make sure that there is no conflict? I thought EFI was an
>>>>> API?
>>>> You allocate it, but payloads expect that the address you pass in as
>>>> address you allocate at is the pointer/address that gets returned.
>>>>
>>> Can you please point me to that? Are you referring to this call?
>>>
>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>           efi_uintn_t *memory_map_size,
>>>           struct efi_mem_desc *memory_map,
>>>           efi_uintn_t *map_key,
>>>           efi_uintn_t *descriptor_size,
>>>           uint32_t *descriptor_version)
>>>
>>> If so, we can still support that.
>>>
>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break things.
>>> I see this function:
>>>
>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>> **buffer)
>>>
>>> So there appears to be no way for people to specify the address they
>>> want. Is there another call somewhere?
>>
>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you receive
>> from efi_get_memory_map().
> That's fine though, I'm not worried about that. It will work correctly, right?
>
>>> NOTE: I am not proposing that the address is made available to the EFI
>>> application - that would always see the sandbox pointer.
>>
>> In that case it's *much* easier to just always store host pointers in the
>> efi memory map. Everything else will be very intrusive.
> Yes there will be map_sysmem() and map_to_sysmem() calls at each
> boundary. That 'intrusion' is what we have everywhere in sandbox and
> it is how things work. But the nice thing is that the tables don't end
> up with 'private' sandbox addresses in them. Also see me comments
> above.

I seem to be missing the point. efi_loader is just a tiny shim layer 
whose only purpose it is to interface between the binary efi interface 
and U-Boot internal code. So why shouldn't efi_loader treat addresses as 
pointers internally?

So how about this: I'll send out a patch set that works the way I think 
is the right way forward. With this I can at least run binaries 
generated by gnu-efi. Then you can send a patch on top with a proposal 
how you think the mapping should happen. My guess is that your patch 
will just clutter the code, but I'll be happy to be persuaded otherwise.


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>> memory
>>>>>>>>>>>> range
>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>> Update
>>>>>>>>>>>> the
>>>>>>>>>>>> EFI
>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>
>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>> 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);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>> happen when
>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>> determining
>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>
>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>
>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>>>>>>> mapping in the allocator
>>>>>>>>>>
>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>
>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>> an
>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>> map_sysmem()
>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>> is
>>>>>>>>>> set up
>>>>>>>>>>
>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>> are
>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>
>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>> pointers
>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>> run
>>>>>>>>> a
>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>> it
>>>>>>>>> wants
>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>> functions, which
>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>
>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>
>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>
>>>>>>>> What do you mean by version 2?
>>>>>>>
>>>>>>> Option 2 is what you called it. It's the only option we have to make
>>>>>>> efi binaries work.
>>>>>>>
>>>>>>>> It looks like you've added one patch,
>>>>>>>> so will you send that to the list?
>>>>>>>
>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>
>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>> memory
>>>>>>>> works.
>>>>>>>
>>>>>>> I still dislike option 1 :)
>>>>>>>
>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>> payloads.
>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>> particular
>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>> structs
>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>> split
>>>>>>> brain situation between u-boot and host pointers.
>>>>>>
>>>>>> OK so you mean they don't have to allocate memory before using it? How
>>>>>> then do you make sure that there is no conflict? I thought EFI was an
>>>>>> API?
>>>>>
>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>
>>>> Can you please point me to that? Are you referring to this call?
>>>>
>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>           efi_uintn_t *memory_map_size,
>>>>           struct efi_mem_desc *memory_map,
>>>>           efi_uintn_t *map_key,
>>>>           efi_uintn_t *descriptor_size,
>>>>           uint32_t *descriptor_version)
>>>>
>>>> If so, we can still support that.
>>>>
>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>> things.
>>>>
>>>> I see this function:
>>>>
>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>> **buffer)
>>>>
>>>> So there appears to be no way for people to specify the address they
>>>> want. Is there another call somewhere?
>>>
>>>
>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>> receive
>>> from efi_get_memory_map().
>>
>> That's fine though, I'm not worried about that. It will work correctly,
>> right?
>>
>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>> application - that would always see the sandbox pointer.
>>>
>>>
>>> In that case it's *much* easier to just always store host pointers in the
>>> efi memory map. Everything else will be very intrusive.
>>
>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>> it is how things work. But the nice thing is that the tables don't end
>> up with 'private' sandbox addresses in them. Also see me comments
>> above.
>
>
> I seem to be missing the point. efi_loader is just a tiny shim layer whose
> only purpose it is to interface between the binary efi interface and U-Boot
> internal code. So why shouldn't efi_loader treat addresses as pointers
> internally?
>
> So how about this: I'll send out a patch set that works the way I think is
> the right way forward. With this I can at least run binaries generated by
> gnu-efi. Then you can send a patch on top with a proposal how you think the
> mapping should happen. My guess is that your patch will just clutter the
> code, but I'll be happy to be persuaded otherwise.

I'm just not keen on putting sandbox pointers in internal data
structures, sorry. It's not the way it is supposed to work and it
makes sandbox diffferent from all the other archs. Trying to minimise
the number of calls to map_sysmem() is not really the goal here.

Regards,
Simon

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

[Alexander Graf]

On 06/14/2018 06:33 PM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>
>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>> memory
>>>>>>>>>>>>> range
>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>> Update
>>>>>>>>>>>>> the
>>>>>>>>>>>>> EFI
>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>> 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);
>>>>>>>>>>>>
>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>> happen when
>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>> determining
>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>
>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use map_sysmem()
>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do no
>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>
>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>
>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>> an
>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>> map_sysmem()
>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>> is
>>>>>>>>>>> set up
>>>>>>>>>>>
>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>> are
>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>
>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>> pointers
>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>
>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>> run
>>>>>>>>>> a
>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>> it
>>>>>>>>>> wants
>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>> functions, which
>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>
>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>
>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>> What do you mean by version 2?
>>>>>>>> Option 2 is what you called it. It's the only option we have to make
>>>>>>>> efi binaries work.
>>>>>>>>
>>>>>>>>> It looks like you've added one patch,
>>>>>>>>> so will you send that to the list?
>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>
>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>> memory
>>>>>>>>> works.
>>>>>>>> I still dislike option 1 :)
>>>>>>>>
>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>> payloads.
>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>> particular
>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>> structs
>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>> split
>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>> OK so you mean they don't have to allocate memory before using it? How
>>>>>>> then do you make sure that there is no conflict? I thought EFI was an
>>>>>>> API?
>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>
>>>>> Can you please point me to that? Are you referring to this call?
>>>>>
>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>            efi_uintn_t *memory_map_size,
>>>>>            struct efi_mem_desc *memory_map,
>>>>>            efi_uintn_t *map_key,
>>>>>            efi_uintn_t *descriptor_size,
>>>>>            uint32_t *descriptor_version)
>>>>>
>>>>> If so, we can still support that.
>>>>>
>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>> things.
>>>>> I see this function:
>>>>>
>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>> **buffer)
>>>>>
>>>>> So there appears to be no way for people to specify the address they
>>>>> want. Is there another call somewhere?
>>>>
>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>> receive
>>>> from efi_get_memory_map().
>>> That's fine though, I'm not worried about that. It will work correctly,
>>> right?
>>>
>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>> application - that would always see the sandbox pointer.
>>>>
>>>> In that case it's *much* easier to just always store host pointers in the
>>>> efi memory map. Everything else will be very intrusive.
>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>> it is how things work. But the nice thing is that the tables don't end
>>> up with 'private' sandbox addresses in them. Also see me comments
>>> above.
>>
>> I seem to be missing the point. efi_loader is just a tiny shim layer whose
>> only purpose it is to interface between the binary efi interface and U-Boot
>> internal code. So why shouldn't efi_loader treat addresses as pointers
>> internally?
>>
>> So how about this: I'll send out a patch set that works the way I think is
>> the right way forward. With this I can at least run binaries generated by
>> gnu-efi. Then you can send a patch on top with a proposal how you think the
>> mapping should happen. My guess is that your patch will just clutter the
>> code, but I'll be happy to be persuaded otherwise.
> I'm just not keen on putting sandbox pointers in internal data
> structures, sorry. It's not the way it is supposed to work and it
> makes sandbox diffferent from all the other archs. Trying to minimise
> the number of calls to map_sysmem() is not really the goal here.

Well, you're not the one who needs to maintain efi_loader code after 
every 2nd line in it ends up being some random map/unmap call ;).

But my proposal was serious. Putting virtual addresses in is trivial and 
gives us proper API interfaces. We can based on that still decide 
whether it's better to not have pointers in the map. I am not convinced 
yet, but open to the idea.

And yes, code maintainability is my uttermost goal of this. Things have 
to be simple for sandbox, otherwise they will break.


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
> On 06/14/2018 06:33 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>
>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>> range
>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>>> Update
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> EFI
>>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>>> 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);
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>>> happen when
>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>>> determining
>>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>>>
>>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
>>>>>>>>>>>> no
>>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>>
>>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>>
>>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>>> an
>>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>>> is
>>>>>>>>>>>> set up
>>>>>>>>>>>>
>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>>> are
>>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>>
>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>>> pointers
>>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>>> run
>>>>>>>>>>> a
>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>>> it
>>>>>>>>>>> wants
>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>>> functions, which
>>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>>
>>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>>
>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>>>
>>>>>>>>>> What do you mean by version 2?
>>>>>>>>>
>>>>>>>>> Option 2 is what you called it. It's the only option we have to
>>>>>>>>> make
>>>>>>>>> efi binaries work.
>>>>>>>>>
>>>>>>>>>> It looks like you've added one patch,
>>>>>>>>>> so will you send that to the list?
>>>>>>>>>
>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>>
>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>>> memory
>>>>>>>>>> works.
>>>>>>>>>
>>>>>>>>> I still dislike option 1 :)
>>>>>>>>>
>>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>>> payloads.
>>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>>> particular
>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>>> structs
>>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>>> split
>>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>>>
>>>>>>>> OK so you mean they don't have to allocate memory before using it?
>>>>>>>> How
>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
>>>>>>>> an
>>>>>>>> API?
>>>>>>>
>>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>>
>>>>>> Can you please point me to that? Are you referring to this call?
>>>>>>
>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>>            efi_uintn_t *memory_map_size,
>>>>>>            struct efi_mem_desc *memory_map,
>>>>>>            efi_uintn_t *map_key,
>>>>>>            efi_uintn_t *descriptor_size,
>>>>>>            uint32_t *descriptor_version)
>>>>>>
>>>>>> If so, we can still support that.
>>>>>>
>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>>> things.
>>>>>>
>>>>>> I see this function:
>>>>>>
>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>>> **buffer)
>>>>>>
>>>>>> So there appears to be no way for people to specify the address they
>>>>>> want. Is there another call somewhere?
>>>>>
>>>>>
>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>>> receive
>>>>> from efi_get_memory_map().
>>>>
>>>> That's fine though, I'm not worried about that. It will work correctly,
>>>> right?
>>>>
>>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>>> application - that would always see the sandbox pointer.
>>>>>
>>>>>
>>>>> In that case it's *much* easier to just always store host pointers in
>>>>> the
>>>>> efi memory map. Everything else will be very intrusive.
>>>>
>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>>> it is how things work. But the nice thing is that the tables don't end
>>>> up with 'private' sandbox addresses in them. Also see me comments
>>>> above.
>>>
>>>
>>> I seem to be missing the point. efi_loader is just a tiny shim layer
>>> whose
>>> only purpose it is to interface between the binary efi interface and
>>> U-Boot
>>> internal code. So why shouldn't efi_loader treat addresses as pointers
>>> internally?
>>>
>>> So how about this: I'll send out a patch set that works the way I think
>>> is
>>> the right way forward. With this I can at least run binaries generated by
>>> gnu-efi. Then you can send a patch on top with a proposal how you think
>>> the
>>> mapping should happen. My guess is that your patch will just clutter the
>>> code, but I'll be happy to be persuaded otherwise.
>>
>> I'm just not keen on putting sandbox pointers in internal data
>> structures, sorry. It's not the way it is supposed to work and it
>> makes sandbox diffferent from all the other archs. Trying to minimise
>> the number of calls to map_sysmem() is not really the goal here.
>
>
> Well, you're not the one who needs to maintain efi_loader code after every
> 2nd line in it ends up being some random map/unmap call ;).
>
> But my proposal was serious. Putting virtual addresses in is trivial and
> gives us proper API interfaces. We can based on that still decide whether
> it's better to not have pointers in the map. I am not convinced yet, but
> open to the idea.
>
> And yes, code maintainability is my uttermost goal of this. Things have to
> be simple for sandbox, otherwise they will break.

A virtual address has a particular meaning. This is not what is going
on with sandbox...

Making sandbox store its internal addresses in an EFI table is not a
good idea. I've tried to explain why. It is not more maintainable - it
just adds confusion. At present all ulong addresses are normal U-Boot
addresses and pointers point into the emulated RAM. It's pretty clear
for people. Please don't change that for EFI.

Can we get my patches applied and then add yours on top? I will send
v7 which I think addresses all comments.

Regards,
Simon

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

[Alexander Graf]

On 06/14/2018 06:55 PM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
>> On 06/14/2018 06:33 PM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>
>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>
>>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>> range
>>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>>>> Update
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> EFI
>>>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>>>> 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);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>>>> happen when
>>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>>>> determining
>>>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
>>>>>>>>>>>>> no
>>>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>>>> an
>>>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>>>> is
>>>>>>>>>>>>> set up
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>>>> are
>>>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>>>> pointers
>>>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>>>
>>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>>>> run
>>>>>>>>>>>> a
>>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>>>> it
>>>>>>>>>>>> wants
>>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>>>> functions, which
>>>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>>>
>>>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>>>
>>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>>>> What do you mean by version 2?
>>>>>>>>>> Option 2 is what you called it. It's the only option we have to
>>>>>>>>>> make
>>>>>>>>>> efi binaries work.
>>>>>>>>>>
>>>>>>>>>>> It looks like you've added one patch,
>>>>>>>>>>> so will you send that to the list?
>>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>>>
>>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>>>> memory
>>>>>>>>>>> works.
>>>>>>>>>> I still dislike option 1 :)
>>>>>>>>>>
>>>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>>>> payloads.
>>>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>>>> particular
>>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>>>> structs
>>>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>>>> split
>>>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>>>> OK so you mean they don't have to allocate memory before using it?
>>>>>>>>> How
>>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
>>>>>>>>> an
>>>>>>>>> API?
>>>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>>>
>>>>>>> Can you please point me to that? Are you referring to this call?
>>>>>>>
>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>>>             efi_uintn_t *memory_map_size,
>>>>>>>             struct efi_mem_desc *memory_map,
>>>>>>>             efi_uintn_t *map_key,
>>>>>>>             efi_uintn_t *descriptor_size,
>>>>>>>             uint32_t *descriptor_version)
>>>>>>>
>>>>>>> If so, we can still support that.
>>>>>>>
>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>>>> things.
>>>>>>> I see this function:
>>>>>>>
>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>>>> **buffer)
>>>>>>>
>>>>>>> So there appears to be no way for people to specify the address they
>>>>>>> want. Is there another call somewhere?
>>>>>>
>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>>>> receive
>>>>>> from efi_get_memory_map().
>>>>> That's fine though, I'm not worried about that. It will work correctly,
>>>>> right?
>>>>>
>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>>>> application - that would always see the sandbox pointer.
>>>>>>
>>>>>> In that case it's *much* easier to just always store host pointers in
>>>>>> the
>>>>>> efi memory map. Everything else will be very intrusive.
>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>>>> it is how things work. But the nice thing is that the tables don't end
>>>>> up with 'private' sandbox addresses in them. Also see me comments
>>>>> above.
>>>>
>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
>>>> whose
>>>> only purpose it is to interface between the binary efi interface and
>>>> U-Boot
>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
>>>> internally?
>>>>
>>>> So how about this: I'll send out a patch set that works the way I think
>>>> is
>>>> the right way forward. With this I can at least run binaries generated by
>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
>>>> the
>>>> mapping should happen. My guess is that your patch will just clutter the
>>>> code, but I'll be happy to be persuaded otherwise.
>>> I'm just not keen on putting sandbox pointers in internal data
>>> structures, sorry. It's not the way it is supposed to work and it
>>> makes sandbox diffferent from all the other archs. Trying to minimise
>>> the number of calls to map_sysmem() is not really the goal here.
>>
>> Well, you're not the one who needs to maintain efi_loader code after every
>> 2nd line in it ends up being some random map/unmap call ;).
>>
>> But my proposal was serious. Putting virtual addresses in is trivial and
>> gives us proper API interfaces. We can based on that still decide whether
>> it's better to not have pointers in the map. I am not convinced yet, but
>> open to the idea.
>>
>> And yes, code maintainability is my uttermost goal of this. Things have to
>> be simple for sandbox, otherwise they will break.
> A virtual address has a particular meaning. This is not what is going
> on with sandbox...
>
> Making sandbox store its internal addresses in an EFI table is not a
> good idea. I've tried to explain why. It is not more maintainable - it
> just adds confusion. At present all ulong addresses are normal U-Boot
> addresses and pointers point into the emulated RAM. It's pretty clear
> for people. Please don't change that for EFI.

If that's your concern, let's have the efi memory map contain pointers; 
because that's what it really does.

> Can we get my patches applied and then add yours on top? I will send
> v7 which I think addresses all comments.

The patch set I just sent contains all patches from your set that I 
think we need, no?


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 11:08, Alexander Graf <agraf@suse.de> wrote:
>
> On 06/14/2018 06:55 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/14/2018 06:33 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>
>>>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>> range
>>>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>>>>> Update
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> EFI
>>>>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>>>>> 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);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>>>>> happen when
>>>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>>>>> determining
>>>>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
>>>>>>>>>>>>>> no
>>>>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>>>>> an
>>>>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>>>>> is
>>>>>>>>>>>>>> set up
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>>>>> are
>>>>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>>>>> pointers
>>>>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>>>>> run
>>>>>>>>>>>>> a
>>>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>>>>> it
>>>>>>>>>>>>> wants
>>>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>>>>> functions, which
>>>>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>>>>>
>>>>>>>>>>>> What do you mean by version 2?
>>>>>>>>>>>
>>>>>>>>>>> Option 2 is what you called it. It's the only option we have to
>>>>>>>>>>> make
>>>>>>>>>>> efi binaries work.
>>>>>>>>>>>
>>>>>>>>>>>> It looks like you've added one patch,
>>>>>>>>>>>> so will you send that to the list?
>>>>>>>>>>>
>>>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>>>>
>>>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>>>>> memory
>>>>>>>>>>>> works.
>>>>>>>>>>>
>>>>>>>>>>> I still dislike option 1 :)
>>>>>>>>>>>
>>>>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>>>>> payloads.
>>>>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>>>>> particular
>>>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>>>>> structs
>>>>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>>>>> split
>>>>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>>>>>
>>>>>>>>>> OK so you mean they don't have to allocate memory before using it?
>>>>>>>>>> How
>>>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
>>>>>>>>>> an
>>>>>>>>>> API?
>>>>>>>>>
>>>>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>>>>
>>>>>>>> Can you please point me to that? Are you referring to this call?
>>>>>>>>
>>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>>>>             efi_uintn_t *memory_map_size,
>>>>>>>>             struct efi_mem_desc *memory_map,
>>>>>>>>             efi_uintn_t *map_key,
>>>>>>>>             efi_uintn_t *descriptor_size,
>>>>>>>>             uint32_t *descriptor_version)
>>>>>>>>
>>>>>>>> If so, we can still support that.
>>>>>>>>
>>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>>>>> things.
>>>>>>>>
>>>>>>>> I see this function:
>>>>>>>>
>>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>>>>> **buffer)
>>>>>>>>
>>>>>>>> So there appears to be no way for people to specify the address they
>>>>>>>> want. Is there another call somewhere?
>>>>>>>
>>>>>>>
>>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>>>>> receive
>>>>>>> from efi_get_memory_map().
>>>>>>
>>>>>> That's fine though, I'm not worried about that. It will work correctly,
>>>>>> right?
>>>>>>
>>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>>>>> application - that would always see the sandbox pointer.
>>>>>>>
>>>>>>>
>>>>>>> In that case it's *much* easier to just always store host pointers in
>>>>>>> the
>>>>>>> efi memory map. Everything else will be very intrusive.
>>>>>>
>>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>>>>> it is how things work. But the nice thing is that the tables don't end
>>>>>> up with 'private' sandbox addresses in them. Also see me comments
>>>>>> above.
>>>>>
>>>>>
>>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
>>>>> whose
>>>>> only purpose it is to interface between the binary efi interface and
>>>>> U-Boot
>>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
>>>>> internally?
>>>>>
>>>>> So how about this: I'll send out a patch set that works the way I think
>>>>> is
>>>>> the right way forward. With this I can at least run binaries generated by
>>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
>>>>> the
>>>>> mapping should happen. My guess is that your patch will just clutter the
>>>>> code, but I'll be happy to be persuaded otherwise.
>>>>
>>>> I'm just not keen on putting sandbox pointers in internal data
>>>> structures, sorry. It's not the way it is supposed to work and it
>>>> makes sandbox diffferent from all the other archs. Trying to minimise
>>>> the number of calls to map_sysmem() is not really the goal here.
>>>
>>>
>>> Well, you're not the one who needs to maintain efi_loader code after every
>>> 2nd line in it ends up being some random map/unmap call ;).
>>>
>>> But my proposal was serious. Putting virtual addresses in is trivial and
>>> gives us proper API interfaces. We can based on that still decide whether
>>> it's better to not have pointers in the map. I am not convinced yet, but
>>> open to the idea.
>>>
>>> And yes, code maintainability is my uttermost goal of this. Things have to
>>> be simple for sandbox, otherwise they will break.
>>
>> A virtual address has a particular meaning. This is not what is going
>> on with sandbox...
>>
>> Making sandbox store its internal addresses in an EFI table is not a
>> good idea. I've tried to explain why. It is not more maintainable - it
>> just adds confusion. At present all ulong addresses are normal U-Boot
>> addresses and pointers point into the emulated RAM. It's pretty clear
>> for people. Please don't change that for EFI.
>
>
> If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.
>
>> Can we get my patches applied and then add yours on top? I will send
>> v7 which I think addresses all comments.
>
>
> The patch set I just sent contains all patches from your set that I think we need, no?

I don't think so, you are missing several. I would like to resolve
comments and get those applied and then move forward with expanding
the feature set. At least that's how it normally works in U-Boot - we
don't normally have the maintainer modify the patch series to his
liking :-)

Regards,
Simon

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

[Alexander Graf]

On 14.06.18 19:15, Simon Glass wrote:
> Hi Alex,
> 
> On 14 June 2018 at 11:08, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 06/14/2018 06:55 PM, Simon Glass wrote:
>>>
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 06/14/2018 06:33 PM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>
>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>
>>>>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>>> range
>>>>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>>>>>> Update
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> EFI
>>>>>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>>>>>> 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);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>>>>>> happen when
>>>>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>>>>>> determining
>>>>>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
>>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
>>>>>>>>>>>>>>> no
>>>>>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>>>>>> an
>>>>>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>> set up
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>>>>>> pointers
>>>>>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>>>>>> run
>>>>>>>>>>>>>> a
>>>>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>>>>>> it
>>>>>>>>>>>>>> wants
>>>>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>>>>>> functions, which
>>>>>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>>>>>>
>>>>>>>>>>>>> What do you mean by version 2?
>>>>>>>>>>>>
>>>>>>>>>>>> Option 2 is what you called it. It's the only option we have to
>>>>>>>>>>>> make
>>>>>>>>>>>> efi binaries work.
>>>>>>>>>>>>
>>>>>>>>>>>>> It looks like you've added one patch,
>>>>>>>>>>>>> so will you send that to the list?
>>>>>>>>>>>>
>>>>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>>>>>
>>>>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>>>>>> memory
>>>>>>>>>>>>> works.
>>>>>>>>>>>>
>>>>>>>>>>>> I still dislike option 1 :)
>>>>>>>>>>>>
>>>>>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>>>>>> payloads.
>>>>>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>>>>>> particular
>>>>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>>>>>> structs
>>>>>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>>>>>> split
>>>>>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>>>>>>
>>>>>>>>>>> OK so you mean they don't have to allocate memory before using it?
>>>>>>>>>>> How
>>>>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
>>>>>>>>>>> an
>>>>>>>>>>> API?
>>>>>>>>>>
>>>>>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>>>>>
>>>>>>>>> Can you please point me to that? Are you referring to this call?
>>>>>>>>>
>>>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>>>>>             efi_uintn_t *memory_map_size,
>>>>>>>>>             struct efi_mem_desc *memory_map,
>>>>>>>>>             efi_uintn_t *map_key,
>>>>>>>>>             efi_uintn_t *descriptor_size,
>>>>>>>>>             uint32_t *descriptor_version)
>>>>>>>>>
>>>>>>>>> If so, we can still support that.
>>>>>>>>>
>>>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>>>>>> things.
>>>>>>>>>
>>>>>>>>> I see this function:
>>>>>>>>>
>>>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>>>>>> **buffer)
>>>>>>>>>
>>>>>>>>> So there appears to be no way for people to specify the address they
>>>>>>>>> want. Is there another call somewhere?
>>>>>>>>
>>>>>>>>
>>>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>>>>>> receive
>>>>>>>> from efi_get_memory_map().
>>>>>>>
>>>>>>> That's fine though, I'm not worried about that. It will work correctly,
>>>>>>> right?
>>>>>>>
>>>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>>>>>> application - that would always see the sandbox pointer.
>>>>>>>>
>>>>>>>>
>>>>>>>> In that case it's *much* easier to just always store host pointers in
>>>>>>>> the
>>>>>>>> efi memory map. Everything else will be very intrusive.
>>>>>>>
>>>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>>>>>> it is how things work. But the nice thing is that the tables don't end
>>>>>>> up with 'private' sandbox addresses in them. Also see me comments
>>>>>>> above.
>>>>>>
>>>>>>
>>>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
>>>>>> whose
>>>>>> only purpose it is to interface between the binary efi interface and
>>>>>> U-Boot
>>>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
>>>>>> internally?
>>>>>>
>>>>>> So how about this: I'll send out a patch set that works the way I think
>>>>>> is
>>>>>> the right way forward. With this I can at least run binaries generated by
>>>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
>>>>>> the
>>>>>> mapping should happen. My guess is that your patch will just clutter the
>>>>>> code, but I'll be happy to be persuaded otherwise.
>>>>>
>>>>> I'm just not keen on putting sandbox pointers in internal data
>>>>> structures, sorry. It's not the way it is supposed to work and it
>>>>> makes sandbox diffferent from all the other archs. Trying to minimise
>>>>> the number of calls to map_sysmem() is not really the goal here.
>>>>
>>>>
>>>> Well, you're not the one who needs to maintain efi_loader code after every
>>>> 2nd line in it ends up being some random map/unmap call ;).
>>>>
>>>> But my proposal was serious. Putting virtual addresses in is trivial and
>>>> gives us proper API interfaces. We can based on that still decide whether
>>>> it's better to not have pointers in the map. I am not convinced yet, but
>>>> open to the idea.
>>>>
>>>> And yes, code maintainability is my uttermost goal of this. Things have to
>>>> be simple for sandbox, otherwise they will break.
>>>
>>> A virtual address has a particular meaning. This is not what is going
>>> on with sandbox...
>>>
>>> Making sandbox store its internal addresses in an EFI table is not a
>>> good idea. I've tried to explain why. It is not more maintainable - it
>>> just adds confusion. At present all ulong addresses are normal U-Boot
>>> addresses and pointers point into the emulated RAM. It's pretty clear
>>> for people. Please don't change that for EFI.
>>
>>
>> If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.
>>
>>> Can we get my patches applied and then add yours on top? I will send
>>> v7 which I think addresses all comments.
>>
>>
>> The patch set I just sent contains all patches from your set that I think we need, no?
> 
> I don't think so, you are missing several. I would like to resolve
> comments and get those applied and then move forward with expanding
> the feature set. At least that's how it normally works in U-Boot - we
> don't normally have the maintainer modify the patch series to his
> liking :-)

True, usually I would just ignore patches after my comments weren't
address 5 times in a row ;).

In all seriousness, I want us to move this forward properly. So what
patches from your set are missing?


[U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish()
[U-Boot,v7,09/10] efi: Create a function to set up for running EFI code	
[U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command	
[U-Boot,v7,07/10] efi: Split out test init/uninit into functions

I don't want to introduce any new commands or do refactoring at this
point. Let's make the selftest work first.

[U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox	

Included

[U-Boot,v7,05/10] efi: sandbox: Add relocation constants

Included

[U-Boot,v7,04/10] efi: sandbox: Add distroboot support

Included

[U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox

Dropped because it's no longer needed.

[U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox

Dropped because it's no longer needed.

[U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox	

Replaced with Heinrich's other proposal


So I think all patches that should be in are in?

Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 12:05, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 14.06.18 19:15, Simon Glass wrote:
> > Hi Alex,
> >
> > On 14 June 2018 at 11:08, Alexander Graf <agraf@suse.de> wrote:
> >>
> >> On 06/14/2018 06:55 PM, Simon Glass wrote:
> >>>
> >>> Hi Alex,
> >>>
> >>> On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
> >>>>
> >>>> On 06/14/2018 06:33 PM, Simon Glass wrote:
> >>>>>
> >>>>> Hi Alex,
> >>>>>
> >>>>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>
> >>>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
> >>>>>>>
> >>>>>>> Hi Alex,
> >>>>>>>
> >>>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>>
> >>>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Alex,
> >>>>>>>>>
> >>>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Alex,
> >>>>>>>>>>>
> >>>>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hi Alex,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Hi Alex,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
> >>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
> >>>>>>>>>>>>>>>>> memory
> >>>>>>>>>>>>>>>>> range
> >>>>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
> >>>>>>>>>>>>>>>>> Update
> >>>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>> EFI
> >>>>>>>>>>>>>>>>> memory init code to take account of that.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> 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 v6: None
> >>>>>>>>>>>>>>>>> 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);
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
> >>>>>>>>>>>>>>>> happen when
> >>>>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
> >>>>>>>>>>>>>>>> determining
> >>>>>>>>>>>>>>>> which addresses are usable in the first place.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> There seem to be two ways to do this:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
> >>>>>>>>>>>>>>> map_sysmem()
> >>>>>>>>>>>>>>> before returning an address in the allocator
> >>>>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
> >>>>>>>>>>>>>>> no
> >>>>>>>>>>>>>>> mapping in the allocator
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I've gone with option 1 since:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> - the EFI addresses are not pointers
> >>>>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
> >>>>>>>>>>>>>>> an
> >>>>>>>>>>>>>>> address rather than a pointer in EFI tables
> >>>>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
> >>>>>>>>>>>>>>> map_sysmem()
> >>>>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
> >>>>>>>>>>>>>>> is
> >>>>>>>>>>>>>>> set up
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
> >>>>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
> >>>>>>>>>>>>>>> are
> >>>>>>>>>>>>>>> best kept to sandbox.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
> >>>>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
> >>>>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
> >>>>>>>>>>>>>>> pointers
> >>>>>>>>>>>>>>> since we typically use the address until the last moment.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
> >>>>>>>>>>>>>> run
> >>>>>>>>>>>>>> a
> >>>>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
> >>>>>>>>>>>>>> it
> >>>>>>>>>>>>>> wants
> >>>>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
> >>>>>>>>>>>>>> functions, which
> >>>>>>>>>>>>>> obviously don't work work from user space.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> But overall, I think this is the right path to tackle this:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What do you mean by version 2?
> >>>>>>>>>>>>
> >>>>>>>>>>>> Option 2 is what you called it. It's the only option we have to
> >>>>>>>>>>>> make
> >>>>>>>>>>>> efi binaries work.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> It looks like you've added one patch,
> >>>>>>>>>>>>> so will you send that to the list?
> >>>>>>>>>>>>
> >>>>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
> >>>>>>>>>>>>> memory
> >>>>>>>>>>>>> works.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I still dislike option 1 :)
> >>>>>>>>>>>>
> >>>>>>>>>>>> The reason is simple: The efi memory map is available to efi
> >>>>>>>>>>>> payloads.
> >>>>>>>>>>>> It's perfectly legal for them to do a static allocation at a
> >>>>>>>>>>>> particular
> >>>>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
> >>>>>>>>>>>> structs
> >>>>>>>>>>>> already with efi applications, so there is no real point to have a
> >>>>>>>>>>>> split
> >>>>>>>>>>>> brain situation between u-boot and host pointers.
> >>>>>>>>>>>
> >>>>>>>>>>> OK so you mean they don't have to allocate memory before using it?
> >>>>>>>>>>> How
> >>>>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
> >>>>>>>>>>> an
> >>>>>>>>>>> API?
> >>>>>>>>>>
> >>>>>>>>>> You allocate it, but payloads expect that the address you pass in as
> >>>>>>>>>> address you allocate at is the pointer/address that gets returned.
> >>>>>>>>>>
> >>>>>>>>> Can you please point me to that? Are you referring to this call?
> >>>>>>>>>
> >>>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
> >>>>>>>>>             efi_uintn_t *memory_map_size,
> >>>>>>>>>             struct efi_mem_desc *memory_map,
> >>>>>>>>>             efi_uintn_t *map_key,
> >>>>>>>>>             efi_uintn_t *descriptor_size,
> >>>>>>>>>             uint32_t *descriptor_version)
> >>>>>>>>>
> >>>>>>>>> If so, we can still support that.
> >>>>>>>>>
> >>>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
> >>>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
> >>>>>>>>>> things.
> >>>>>>>>>
> >>>>>>>>> I see this function:
> >>>>>>>>>
> >>>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
> >>>>>>>>> **buffer)
> >>>>>>>>>
> >>>>>>>>> So there appears to be no way for people to specify the address they
> >>>>>>>>> want. Is there another call somewhere?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
> >>>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
> >>>>>>>> receive
> >>>>>>>> from efi_get_memory_map().
> >>>>>>>
> >>>>>>> That's fine though, I'm not worried about that. It will work correctly,
> >>>>>>> right?
> >>>>>>>
> >>>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
> >>>>>>>>> application - that would always see the sandbox pointer.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> In that case it's *much* easier to just always store host pointers in
> >>>>>>>> the
> >>>>>>>> efi memory map. Everything else will be very intrusive.
> >>>>>>>
> >>>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
> >>>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
> >>>>>>> it is how things work. But the nice thing is that the tables don't end
> >>>>>>> up with 'private' sandbox addresses in them. Also see me comments
> >>>>>>> above.
> >>>>>>
> >>>>>>
> >>>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
> >>>>>> whose
> >>>>>> only purpose it is to interface between the binary efi interface and
> >>>>>> U-Boot
> >>>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
> >>>>>> internally?
> >>>>>>
> >>>>>> So how about this: I'll send out a patch set that works the way I think
> >>>>>> is
> >>>>>> the right way forward. With this I can at least run binaries generated by
> >>>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
> >>>>>> the
> >>>>>> mapping should happen. My guess is that your patch will just clutter the
> >>>>>> code, but I'll be happy to be persuaded otherwise.
> >>>>>
> >>>>> I'm just not keen on putting sandbox pointers in internal data
> >>>>> structures, sorry. It's not the way it is supposed to work and it
> >>>>> makes sandbox diffferent from all the other archs. Trying to minimise
> >>>>> the number of calls to map_sysmem() is not really the goal here.
> >>>>
> >>>>
> >>>> Well, you're not the one who needs to maintain efi_loader code after every
> >>>> 2nd line in it ends up being some random map/unmap call ;).
> >>>>
> >>>> But my proposal was serious. Putting virtual addresses in is trivial and
> >>>> gives us proper API interfaces. We can based on that still decide whether
> >>>> it's better to not have pointers in the map. I am not convinced yet, but
> >>>> open to the idea.
> >>>>
> >>>> And yes, code maintainability is my uttermost goal of this. Things have to
> >>>> be simple for sandbox, otherwise they will break.
> >>>
> >>> A virtual address has a particular meaning. This is not what is going
> >>> on with sandbox...
> >>>
> >>> Making sandbox store its internal addresses in an EFI table is not a
> >>> good idea. I've tried to explain why. It is not more maintainable - it
> >>> just adds confusion. At present all ulong addresses are normal U-Boot
> >>> addresses and pointers point into the emulated RAM. It's pretty clear
> >>> for people. Please don't change that for EFI.
> >>
> >>
> >> If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.
> >>
> >>> Can we get my patches applied and then add yours on top? I will send
> >>> v7 which I think addresses all comments.
> >>
> >>
> >> The patch set I just sent contains all patches from your set that I think we need, no?
> >
> > I don't think so, you are missing several. I would like to resolve
> > comments and get those applied and then move forward with expanding
> > the feature set. At least that's how it normally works in U-Boot - we
> > don't normally have the maintainer modify the patch series to his
> > liking :-)
>
> True, usually I would just ignore patches after my comments weren't
> address 5 times in a row ;).

What are you talking about? I only missed one comment on v5, for one
patch, which was creating the function, now done in v7. Please
explain.

>
> In all seriousness, I want us to move this forward properly. So what
> patches from your set are missing?
>
>
> [U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish()
> [U-Boot,v7,09/10] efi: Create a function to set up for running EFI code
> [U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command
> [U-Boot,v7,07/10] efi: Split out test init/uninit into functions
>
> I don't want to introduce any new commands or do refactoring at this
> point. Let's make the selftest work first.
>
> [U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox
>
> Included
>
> [U-Boot,v7,05/10] efi: sandbox: Add relocation constants
>
> Included
>
> [U-Boot,v7,04/10] efi: sandbox: Add distroboot support
>
> Included
>
> [U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox
>
> Dropped because it's no longer needed.

Actually this one should be applied. These are addresses so we should
not be passing sandbox pointers in here.

>
> [U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox
>
> Dropped because it's no longer needed.

Same here

>
> [U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox
>
> Replaced with Heinrich's other proposal
>
>
> So I think all patches that should be in are in?

I'd like to get 7-10 in as well. The 'bootefi test' thing is useful as
a sanity check. Or are you saying you don't want that simple test?
Otherwise you should not be changing the ordering as a maintainer.

The main issue is the sandbox memory mapping stuff, which we still disagree on.

Regards,
Simon

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

[Alexander Graf]

On 14.06.18 21:02, Simon Glass wrote:
> Hi Alex,
> 
> On 14 June 2018 at 12:05, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>
>> On 14.06.18 19:15, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 11:08, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 06/14/2018 06:55 PM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> On 06/14/2018 06:33 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>
>>>>>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>>>>> range
>>>>>>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>>>>>>>> Update
>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>> EFI
>>>>>>>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>>>>>>>> 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);
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>>>>>>>> happen when
>>>>>>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>>>>>>>> determining
>>>>>>>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
>>>>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
>>>>>>>>>>>>>>>>> no
>>>>>>>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>>>>>>>> an
>>>>>>>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>> set up
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>>>>>>>> pointers
>>>>>>>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>>>>>>>> run
>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>> wants
>>>>>>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>>>>>>>> functions, which
>>>>>>>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> What do you mean by version 2?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Option 2 is what you called it. It's the only option we have to
>>>>>>>>>>>>>> make
>>>>>>>>>>>>>> efi binaries work.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It looks like you've added one patch,
>>>>>>>>>>>>>>> so will you send that to the list?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>> works.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I still dislike option 1 :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>>>>>>>> payloads.
>>>>>>>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>>>>>>>> particular
>>>>>>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>>>>>>>> structs
>>>>>>>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>>>>>>>> split
>>>>>>>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>>>>>>>>
>>>>>>>>>>>>> OK so you mean they don't have to allocate memory before using it?
>>>>>>>>>>>>> How
>>>>>>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
>>>>>>>>>>>>> an
>>>>>>>>>>>>> API?
>>>>>>>>>>>>
>>>>>>>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>>>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>>>>>>>
>>>>>>>>>>> Can you please point me to that? Are you referring to this call?
>>>>>>>>>>>
>>>>>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>>>>>>>             efi_uintn_t *memory_map_size,
>>>>>>>>>>>             struct efi_mem_desc *memory_map,
>>>>>>>>>>>             efi_uintn_t *map_key,
>>>>>>>>>>>             efi_uintn_t *descriptor_size,
>>>>>>>>>>>             uint32_t *descriptor_version)
>>>>>>>>>>>
>>>>>>>>>>> If so, we can still support that.
>>>>>>>>>>>
>>>>>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>>>>>>>> things.
>>>>>>>>>>>
>>>>>>>>>>> I see this function:
>>>>>>>>>>>
>>>>>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>>>>>>>> **buffer)
>>>>>>>>>>>
>>>>>>>>>>> So there appears to be no way for people to specify the address they
>>>>>>>>>>> want. Is there another call somewhere?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>>>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>>>>>>>> receive
>>>>>>>>>> from efi_get_memory_map().
>>>>>>>>>
>>>>>>>>> That's fine though, I'm not worried about that. It will work correctly,
>>>>>>>>> right?
>>>>>>>>>
>>>>>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>>>>>>>> application - that would always see the sandbox pointer.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> In that case it's *much* easier to just always store host pointers in
>>>>>>>>>> the
>>>>>>>>>> efi memory map. Everything else will be very intrusive.
>>>>>>>>>
>>>>>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>>>>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>>>>>>>> it is how things work. But the nice thing is that the tables don't end
>>>>>>>>> up with 'private' sandbox addresses in them. Also see me comments
>>>>>>>>> above.
>>>>>>>>
>>>>>>>>
>>>>>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
>>>>>>>> whose
>>>>>>>> only purpose it is to interface between the binary efi interface and
>>>>>>>> U-Boot
>>>>>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
>>>>>>>> internally?
>>>>>>>>
>>>>>>>> So how about this: I'll send out a patch set that works the way I think
>>>>>>>> is
>>>>>>>> the right way forward. With this I can at least run binaries generated by
>>>>>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
>>>>>>>> the
>>>>>>>> mapping should happen. My guess is that your patch will just clutter the
>>>>>>>> code, but I'll be happy to be persuaded otherwise.
>>>>>>>
>>>>>>> I'm just not keen on putting sandbox pointers in internal data
>>>>>>> structures, sorry. It's not the way it is supposed to work and it
>>>>>>> makes sandbox diffferent from all the other archs. Trying to minimise
>>>>>>> the number of calls to map_sysmem() is not really the goal here.
>>>>>>
>>>>>>
>>>>>> Well, you're not the one who needs to maintain efi_loader code after every
>>>>>> 2nd line in it ends up being some random map/unmap call ;).
>>>>>>
>>>>>> But my proposal was serious. Putting virtual addresses in is trivial and
>>>>>> gives us proper API interfaces. We can based on that still decide whether
>>>>>> it's better to not have pointers in the map. I am not convinced yet, but
>>>>>> open to the idea.
>>>>>>
>>>>>> And yes, code maintainability is my uttermost goal of this. Things have to
>>>>>> be simple for sandbox, otherwise they will break.
>>>>>
>>>>> A virtual address has a particular meaning. This is not what is going
>>>>> on with sandbox...
>>>>>
>>>>> Making sandbox store its internal addresses in an EFI table is not a
>>>>> good idea. I've tried to explain why. It is not more maintainable - it
>>>>> just adds confusion. At present all ulong addresses are normal U-Boot
>>>>> addresses and pointers point into the emulated RAM. It's pretty clear
>>>>> for people. Please don't change that for EFI.
>>>>
>>>>
>>>> If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.
>>>>
>>>>> Can we get my patches applied and then add yours on top? I will send
>>>>> v7 which I think addresses all comments.
>>>>
>>>>
>>>> The patch set I just sent contains all patches from your set that I think we need, no?
>>>
>>> I don't think so, you are missing several. I would like to resolve
>>> comments and get those applied and then move forward with expanding
>>> the feature set. At least that's how it normally works in U-Boot - we
>>> don't normally have the maintainer modify the patch series to his
>>> liking :-)
>>
>> True, usually I would just ignore patches after my comments weren't
>> address 5 times in a row ;).
> 
> What are you talking about? I only missed one comment on v5, for one
> patch, which was creating the function, now done in v7. Please
> explain.

I thought I had expressed my dislike for the current memory mapping
model a few times by now. Maybe I wasn't explicit enough?

>> In all seriousness, I want us to move this forward properly. So what
>> patches from your set are missing?
>>
>>
>> [U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish()
>> [U-Boot,v7,09/10] efi: Create a function to set up for running EFI code
>> [U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command
>> [U-Boot,v7,07/10] efi: Split out test init/uninit into functions
>>
>> I don't want to introduce any new commands or do refactoring at this
>> point. Let's make the selftest work first.
>>
>> [U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox
>>
>> Included
>>
>> [U-Boot,v7,05/10] efi: sandbox: Add relocation constants
>>
>> Included
>>
>> [U-Boot,v7,04/10] efi: sandbox: Add distroboot support
>>
>> Included
>>
>> [U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox
>>
>> Dropped because it's no longer needed.
> 
> Actually this one should be applied. These are addresses so we should
> not be passing sandbox pointers in here.

Well, maybe. But then we'd have to adjust for the API the other way
around, as now pointers are what allocations return (because they're a
payload visible API, so they have to).

> 
>>
>> [U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox
>>
>> Dropped because it's no longer needed.
> 
> Same here

Why does it hurt to expose the U-Boot and RTS memory addresses?

> 
>>
>> [U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox
>>
>> Replaced with Heinrich's other proposal
>>
>>
>> So I think all patches that should be in are in?
> 
> I'd like to get 7-10 in as well. The 'bootefi test' thing is useful as
> a sanity check. Or are you saying you don't want that simple test?

I want a simple test, but I think the simple test is bootefi selftest.
You can limit its scope to only emit "hello world" by setting an
environment variable if you want to :).

> Otherwise you should not be changing the ordering as a maintainer.
> 
> The main issue is the sandbox memory mapping stuff, which we still disagree on.

I guess we agree that we disagree :).

Looking at the current code base, we pretty much only call the map/unmap
dance every time we go from command line arguments (where you pass in
u-boot addresses) to pointers. Anything internal is usually dealing with
pointers.

I think that's a reasonable thing to do. And I think we should follow
the same logic in the efi_loader parts. Command line parameters can
easily be handled via map/unmap, but anything internal - and the efi
memory map is what I consider internal - should just contain pointers.

The only reason they're not marked as pointers in the struct is that we
want to copy the structs right back into the payload and there they are
ABI defined as 64bit fields.


Alex

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

[Simon Glass]

Hi Alex,

On 14 June 2018 at 13:32, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 14.06.18 21:02, Simon Glass wrote:
>> Hi Alex,
>>
>> On 14 June 2018 at 12:05, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 14.06.18 19:15, Simon Glass wrote:
>>>> Hi Alex,
>>>>
>>>> On 14 June 2018 at 11:08, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/14/2018 06:55 PM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 14 June 2018 at 10:42, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/14/2018 06:33 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>> On 14 June 2018 at 10:26, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>> On 06/14/2018 06:13 PM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>> On 14 June 2018 at 10:07, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 06/14/2018 05:53 PM, Simon Glass wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>
>>>>>>>>>>>> On 14 June 2018 at 09:47, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Am 14.06.2018 um 17:43 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 14 June 2018 at 08:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Am 14.06.2018 um 16:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 14 June 2018 at 07:41, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>>>>>>>> On 06/14/2018 02:58 PM, Simon Glass wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Hi Alex,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 14 June 2018 at 04:12, Alexander Graf <agraf@suse.de>
>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 06/13/2018 04:37 AM, Simon Glass wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox
>>>>>>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>>>>>> range
>>>>>>>>>>>>>>>>>>>> so does not need to be excluded when allocating EFI memory.
>>>>>>>>>>>>>>>>>>>> Update
>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>> EFI
>>>>>>>>>>>>>>>>>>>> memory init code to take account of that.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> 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 v6: None
>>>>>>>>>>>>>>>>>>>> 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);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> This is still the wrong spot. We don't want the conversion to
>>>>>>>>>>>>>>>>>>> happen when
>>>>>>>>>>>>>>>>>>> going from an EFI internal address to an allocation, but when
>>>>>>>>>>>>>>>>>>> determining
>>>>>>>>>>>>>>>>>>> which addresses are usable in the first place.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> There seem to be two ways to do this:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 1. Record addresses (ulong) in the EFI tables and use
>>>>>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>>>>>> before returning an address in the allocator
>>>>>>>>>>>>>>>>>> 2. Store pointers in the EFI tables using map_sysmem(), then do
>>>>>>>>>>>>>>>>>> no
>>>>>>>>>>>>>>>>>> mapping in the allocator
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I've gone with option 1 since:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> - the EFI addresses are not pointers
>>>>>>>>>>>>>>>>>> - it makes sandbox consistent with other architectures which use
>>>>>>>>>>>>>>>>>> an
>>>>>>>>>>>>>>>>>> address rather than a pointer in EFI tables
>>>>>>>>>>>>>>>>>> - we don't normally do pointer arithmetic on the results of
>>>>>>>>>>>>>>>>>> map_sysmem()
>>>>>>>>>>>>>>>>>> - we normally map the memory when it is used rather than when it
>>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>>> set up
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I think you are asking for option 2. I think that would get very
>>>>>>>>>>>>>>>>>> confusing. The addresses where things actually end up in sandbox
>>>>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>>>>> best kept to sandbox.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Overall I feel that you are either missing the point of sandbox
>>>>>>>>>>>>>>>>>> addressing, or don't agree with how it is done. But it does work
>>>>>>>>>>>>>>>>>> pretty well and we don't get a lot of confusion with sandbox
>>>>>>>>>>>>>>>>>> pointers
>>>>>>>>>>>>>>>>>> since we typically use the address until the last moment.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I've assembled a quick tree for version 2. With that I'm able to
>>>>>>>>>>>>>>>>> run
>>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>> simple hello world efi application. Grub refuses to start because
>>>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>>> wants
>>>>>>>>>>>>>>>>> memory in the low 32bit and also emits random PIO accessing
>>>>>>>>>>>>>>>>> functions, which
>>>>>>>>>>>>>>>>> obviously don't work work from user space.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> But overall, I think this is the right path to tackle this:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> https://github.com/agraf/u-boot/tree/efi-sandbox
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> What do you mean by version 2?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Option 2 is what you called it. It's the only option we have to
>>>>>>>>>>>>>>> make
>>>>>>>>>>>>>>> efi binaries work.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> It looks like you've added one patch,
>>>>>>>>>>>>>>>> so will you send that to the list?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It's more than 1 patch. And yes, I'll send them.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Anyway, I hope I can convince you of the above, the way sandbox
>>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>> works.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I still dislike option 1 :)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The reason is simple: The efi memory map is available to efi
>>>>>>>>>>>>>>> payloads.
>>>>>>>>>>>>>>> It's perfectly legal for them to do a static allocation at a
>>>>>>>>>>>>>>> particular
>>>>>>>>>>>>>>> address. We also share a lot of (host) pointers for callbacks and
>>>>>>>>>>>>>>> structs
>>>>>>>>>>>>>>> already with efi applications, so there is no real point to have a
>>>>>>>>>>>>>>> split
>>>>>>>>>>>>>>> brain situation between u-boot and host pointers.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> OK so you mean they don't have to allocate memory before using it?
>>>>>>>>>>>>>> How
>>>>>>>>>>>>>> then do you make sure that there is no conflict? I thought EFI was
>>>>>>>>>>>>>> an
>>>>>>>>>>>>>> API?
>>>>>>>>>>>>>
>>>>>>>>>>>>> You allocate it, but payloads expect that the address you pass in as
>>>>>>>>>>>>> address you allocate at is the pointer/address that gets returned.
>>>>>>>>>>>>>
>>>>>>>>>>>> Can you please point me to that? Are you referring to this call?
>>>>>>>>>>>>
>>>>>>>>>>>> static efi_status_t EFIAPI efi_get_memory_map_ext(
>>>>>>>>>>>>             efi_uintn_t *memory_map_size,
>>>>>>>>>>>>             struct efi_mem_desc *memory_map,
>>>>>>>>>>>>             efi_uintn_t *map_key,
>>>>>>>>>>>>             efi_uintn_t *descriptor_size,
>>>>>>>>>>>>             uint32_t *descriptor_version)
>>>>>>>>>>>>
>>>>>>>>>>>> If so, we can still support that.
>>>>>>>>>>>>
>>>>>>>>>>>>> In a nutshell, what you're proposing is that the efi quivalent of
>>>>>>>>>>>>> mmap(addr, MAP_FIXED) returns something != addr. That will break
>>>>>>>>>>>>> things.
>>>>>>>>>>>>
>>>>>>>>>>>> I see this function:
>>>>>>>>>>>>
>>>>>>>>>>>> efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void
>>>>>>>>>>>> **buffer)
>>>>>>>>>>>>
>>>>>>>>>>>> So there appears to be no way for people to specify the address they
>>>>>>>>>>>> want. Is there another call somewhere?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> You can call efi_allocate_pages() with EFI_ALLOCATE_ADDRESS or even
>>>>>>>>>>> EFI_ALLOCATE_MAX_ADDRESS which you usually base on information you
>>>>>>>>>>> receive
>>>>>>>>>>> from efi_get_memory_map().
>>>>>>>>>>
>>>>>>>>>> That's fine though, I'm not worried about that. It will work correctly,
>>>>>>>>>> right?
>>>>>>>>>>
>>>>>>>>>>>> NOTE: I am not proposing that the address is made available to the EFI
>>>>>>>>>>>> application - that would always see the sandbox pointer.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> In that case it's *much* easier to just always store host pointers in
>>>>>>>>>>> the
>>>>>>>>>>> efi memory map. Everything else will be very intrusive.
>>>>>>>>>>
>>>>>>>>>> Yes there will be map_sysmem() and map_to_sysmem() calls at each
>>>>>>>>>> boundary. That 'intrusion' is what we have everywhere in sandbox and
>>>>>>>>>> it is how things work. But the nice thing is that the tables don't end
>>>>>>>>>> up with 'private' sandbox addresses in them. Also see me comments
>>>>>>>>>> above.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I seem to be missing the point. efi_loader is just a tiny shim layer
>>>>>>>>> whose
>>>>>>>>> only purpose it is to interface between the binary efi interface and
>>>>>>>>> U-Boot
>>>>>>>>> internal code. So why shouldn't efi_loader treat addresses as pointers
>>>>>>>>> internally?
>>>>>>>>>
>>>>>>>>> So how about this: I'll send out a patch set that works the way I think
>>>>>>>>> is
>>>>>>>>> the right way forward. With this I can at least run binaries generated by
>>>>>>>>> gnu-efi. Then you can send a patch on top with a proposal how you think
>>>>>>>>> the
>>>>>>>>> mapping should happen. My guess is that your patch will just clutter the
>>>>>>>>> code, but I'll be happy to be persuaded otherwise.
>>>>>>>>
>>>>>>>> I'm just not keen on putting sandbox pointers in internal data
>>>>>>>> structures, sorry. It's not the way it is supposed to work and it
>>>>>>>> makes sandbox diffferent from all the other archs. Trying to minimise
>>>>>>>> the number of calls to map_sysmem() is not really the goal here.
>>>>>>>
>>>>>>>
>>>>>>> Well, you're not the one who needs to maintain efi_loader code after every
>>>>>>> 2nd line in it ends up being some random map/unmap call ;).
>>>>>>>
>>>>>>> But my proposal was serious. Putting virtual addresses in is trivial and
>>>>>>> gives us proper API interfaces. We can based on that still decide whether
>>>>>>> it's better to not have pointers in the map. I am not convinced yet, but
>>>>>>> open to the idea.
>>>>>>>
>>>>>>> And yes, code maintainability is my uttermost goal of this. Things have to
>>>>>>> be simple for sandbox, otherwise they will break.
>>>>>>
>>>>>> A virtual address has a particular meaning. This is not what is going
>>>>>> on with sandbox...
>>>>>>
>>>>>> Making sandbox store its internal addresses in an EFI table is not a
>>>>>> good idea. I've tried to explain why. It is not more maintainable - it
>>>>>> just adds confusion. At present all ulong addresses are normal U-Boot
>>>>>> addresses and pointers point into the emulated RAM. It's pretty clear
>>>>>> for people. Please don't change that for EFI.
>>>>>
>>>>>
>>>>> If that's your concern, let's have the efi memory map contain pointers; because that's what it really does.
>>>>>
>>>>>> Can we get my patches applied and then add yours on top? I will send
>>>>>> v7 which I think addresses all comments.
>>>>>
>>>>>
>>>>> The patch set I just sent contains all patches from your set that I think we need, no?
>>>>
>>>> I don't think so, you are missing several. I would like to resolve
>>>> comments and get those applied and then move forward with expanding
>>>> the feature set. At least that's how it normally works in U-Boot - we
>>>> don't normally have the maintainer modify the patch series to his
>>>> liking :-)
>>>
>>> True, usually I would just ignore patches after my comments weren't
>>> address 5 times in a row ;).
>>
>> What are you talking about? I only missed one comment on v5, for one
>> patch, which was creating the function, now done in v7. Please
>> explain.
>
> I thought I had expressed my dislike for the current memory mapping
> model a few times by now. Maybe I wasn't explicit enough?

I was pretty clear that I don't want to change the sandbox memory
model, so that's why I didn't do anything on that point. I don't think
I could have been any clearer.

>
>>> In all seriousness, I want us to move this forward properly. So what
>>> patches from your set are missing?
>>>
>>>
>>> [U-Boot,v7,10/10] efi: Rename bootefi_test_finish() to bootefi_run_finish()
>>> [U-Boot,v7,09/10] efi: Create a function to set up for running EFI code
>>> [U-Boot,v7,08/10] efi: sandbox: Add a simple 'bootefi test' command
>>> [U-Boot,v7,07/10] efi: Split out test init/uninit into functions
>>>
>>> I don't want to introduce any new commands or do refactoring at this
>>> point. Let's make the selftest work first.
>>>
>>> [U-Boot,v7,06/10] efi: sandbox: Enable EFI loader build for sandbox
>>>
>>> Included
>>>
>>> [U-Boot,v7,05/10] efi: sandbox: Add relocation constants
>>>
>>> Included
>>>
>>> [U-Boot,v7,04/10] efi: sandbox: Add distroboot support
>>>
>>> Included
>>>
>>> [U-Boot,v7,03/10] sandbox: smbios: Update to support sandbox
>>>
>>> Dropped because it's no longer needed.
>>
>> Actually this one should be applied. These are addresses so we should
>> not be passing sandbox pointers in here.
>
> Well, maybe. But then we'd have to adjust for the API the other way
> around, as now pointers are what allocations return (because they're a
> payload visible API, so they have to).

Actually the function I am talking about is internal to U-Boot. The
_ext() function can do the mapping as needed, as per my latest series.
But I feel it can still be improved, since addresses and pointers are
still a little mixed together in the EFI code.

>
>>
>>>
>>> [U-Boot,v7,02/10] efi: sandbox: Adjust memory usage for sandbox
>>>
>>> Dropped because it's no longer needed.
>>
>> Same here
>
> Why does it hurt to expose the U-Boot and RTS memory addresses?

What is an RTS memory address?

>
>>
>>>
>>> [U-Boot,v7,01/10] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox
>>>
>>> Replaced with Heinrich's other proposal
>>>
>>>
>>> So I think all patches that should be in are in?
>>
>> I'd like to get 7-10 in as well. The 'bootefi test' thing is useful as
>> a sanity check. Or are you saying you don't want that simple test?
>
> I want a simple test, but I think the simple test is bootefi selftest.
> You can limit its scope to only emit "hello world" by setting an
> environment variable if you want to :).

I've reviewed selftest as it was built up and again recently. It is
certainly not a simple sanity chec, which is what hello word is for.

>
>> Otherwise you should not be changing the ordering as a maintainer.
>>
>> The main issue is the sandbox memory mapping stuff, which we still disagree on.
>
> I guess we agree that we disagree :).
>
> Looking at the current code base, we pretty much only call the map/unmap
> dance every time we go from command line arguments (where you pass in
> u-boot addresses) to pointers. Anything internal is usually dealing with
> pointers.

That's just not the case, as I have explained. Images in U-Boot,
device tree, frame buffer, all peripheral's register locations, bootm,
FIT and many other things are dealt with with addresses. U-Boot uses
addresses for addresses, not pointers. The pointers are created from
the addresses, but are not the primary means for communicating
addresses.

>
> I think that's a reasonable thing to do. And I think we should follow
> the same logic in the efi_loader parts. Command line parameters can
> easily be handled via map/unmap, but anything internal - and the efi
> memory map is what I consider internal - should just contain pointers.
>
> The only reason they're not marked as pointers in the struct is that we
> want to copy the structs right back into the payload and there they are
> ABI defined as 64bit fields.

I still strongly disagree with this. I would like to get the sandbox
feature in without changing how sandbox works.

Regards,
Simon

[U-Boot] [PATCH v6 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 v6: None
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 v6 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 v6:
- Warn about building sandbox EFI support on something other than __x86_64__

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

 include/config_distro_bootcmd.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index d672e8ebe6..1bd79ae3b8 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -251,6 +251,8 @@
 #elif defined(CONFIG_ARM)
 #define BOOTENV_EFI_PXE_ARCH "0xa"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
+
+/* For sandbox we only support 64-bit x86 at present */
 #elif defined(CONFIG_X86)
 /* Always assume we're running 64bit */
 #define BOOTENV_EFI_PXE_ARCH "0x7"
@@ -261,6 +263,17 @@
 #elif defined(CONFIG_CPU_RISCV_64)
 #define BOOTENV_EFI_PXE_ARCH "0x1b"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00027:UNDI:003000"
+#elif defined(CONFIG_SANDBOX)
+/*
+ * TODO(sjg at chromium.org): Consider providing a way to enable sandbox features
+ * based on the host architecture
+ */
+# ifndef __x86_64__
+#  warning "sandbox EFI support is only tested on 64-bit x86"
+# endif
+/* To support other *host* architectures this should be changed */
+#define BOOTENV_EFI_PXE_ARCH "0x7"
+#define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
 #else
 #error Please specify an EFI client identifier
 #endif
-- 
2.18.0.rc1.244.gcf134e6275-goog

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

[Alexander Graf]

On 06/13/2018 04:37 AM, 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 v6:
> - Warn about building sandbox EFI support on something other than __x86_64__
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   include/config_distro_bootcmd.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index d672e8ebe6..1bd79ae3b8 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -251,6 +251,8 @@
>   #elif defined(CONFIG_ARM)
>   #define BOOTENV_EFI_PXE_ARCH "0xa"
>   #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
> +
> +/* For sandbox we only support 64-bit x86 at present */

I think the TODO in the CONFIG_SANDBOX part is enough, no need to 
duplicate it here.

Alex

>   #elif defined(CONFIG_X86)
>   /* Always assume we're running 64bit */
>   #define BOOTENV_EFI_PXE_ARCH "0x7"
> @@ -261,6 +263,17 @@
>   #elif defined(CONFIG_CPU_RISCV_64)
>   #define BOOTENV_EFI_PXE_ARCH "0x1b"
>   #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00027:UNDI:003000"
> +#elif defined(CONFIG_SANDBOX)
> +/*
> + * TODO(sjg at chromium.org): Consider providing a way to enable sandbox features
> + * based on the host architecture
> + */
> +# ifndef __x86_64__
> +#  warning "sandbox EFI support is only tested on 64-bit x86"
> +# endif
> +/* To support other *host* architectures this should be changed */
> +#define BOOTENV_EFI_PXE_ARCH "0x7"
> +#define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
>   #else
>   #error Please specify an EFI client identifier
>   #endif

[U-Boot] [PATCH v6 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 v6:
- Warn about building sandbox EFI support on something other than __x86_64__

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

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

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 65f2bcf140..acda21c91d 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -58,6 +58,18 @@ struct dyn_sym {
 #define R_ABSOLUTE	R_RISCV_64
 #define SYM_INDEX	32
 #endif
+
+/* For sandbox we only support 64-bit x86 at present */
+#elif defined(CONFIG_SANDBOX)
+/*
+ * TODO(sjg at chromium.org): Consider providing a way to enable sandbox features
+ * based on the host architecture
+ */
+# ifndef __x86_64__
+#  warning "sandbox EFI support is only tested on 64-bit x86"
+# endif
+#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 v6 06/13] efi: sandbox: Add relocation constants

[Alexander Graf]

On 06/13/2018 04:37 AM, Simon Glass wrote:
> Add these so that we can build the EFI loader for sandbox. The values are
> for x86_64 so potentially bogus. But we don't support relocation within
> sandbox anyway.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v6:
> - Warn about building sandbox EFI support on something other than __x86_64__
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   lib/efi_loader/efi_runtime.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 65f2bcf140..acda21c91d 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -58,6 +58,18 @@ struct dyn_sym {
>   #define R_ABSOLUTE	R_RISCV_64
>   #define SYM_INDEX	32
>   #endif
> +
> +/* For sandbox we only support 64-bit x86 at present */

Same here. TODO with #ifndef is plenty to tell people what you're trying 
to say :)

Alex

> +#elif defined(CONFIG_SANDBOX)
> +/*
> + * TODO(sjg at chromium.org): Consider providing a way to enable sandbox features
> + * based on the host architecture
> + */
> +# ifndef __x86_64__
> +#  warning "sandbox EFI support is only tested on 64-bit x86"
> +# endif
> +#define R_RELATIVE	8
> +#define R_MASK		0xffffffffULL
>   #else
>   #error Need to add relocation awareness
>   #endif

[U-Boot] [PATCH v6 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 v6: None
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 acda21c91d..388dfb9840 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] [PATCH v6 07/13] efi: Add a comment about duplicated ELF constants

[Alexander Graf]

On 06/13/2018 04:37 AM, Simon Glass wrote:
> These constants are defined in arch-specific code but redefined here. Add
> a TODO to clean this up.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

This patch is part of efi-next already.


Alex

[U-Boot] [PATCH v6 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 v6: None
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 v6 08/13] efi: sandbox: Enable EFI loader builder for sandbox

[Alexander Graf]

On 06/13/2018 04:37 AM, Simon Glass wrote:
> 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>

Are you sure you want to enable a "builder", not the "build"? (see 
subject line)


Alex

[U-Boot] [PATCH v6 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 v6: None
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 v6 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 v6: None
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 v6 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 v6: None
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..b9eb04531b 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 v6 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 v6: None
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 b9eb04531b..7a98c9a6bb 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 v6 13/13] Revert "buildman: Extract environment as part of each build"

[Simon Glass]

This reverts commit 0ddc510ea37aa578b8cb197840a5125409978bec.

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

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

 tools/buildman/builderthread.py | 10 ----------
 tools/buildman/func_test.py     |  5 -----
 2 files changed, 15 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index c84ba6acf1..0efe80d945 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -351,16 +351,6 @@ class BuilderThread(threading.Thread):
                     lines.append(size_result.stdout.splitlines()[1] + ' ' +
                                  rodata_size)
 
-            # Extract the environment from U-Boot and dump it out
-            cmd = ['%sobjcopy' % self.toolchain.cross, '-O', 'binary',
-                   '-j', '.rodata.default_environment',
-                   'env/built-in.o', 'uboot.env']
-            command.RunPipe([cmd], capture=True,
-                            capture_stderr=True, cwd=result.out_dir,
-                            raise_on_error=False, env=env)
-            ubootenv = os.path.join(result.out_dir, 'uboot.env')
-            self.CopyFiles(result.out_dir, build_dir, '', ['uboot.env'])
-
             # Write out the image sizes file. This is similar to the output
             # of binutil's 'size' utility, but it omits the header line and
             # adds an additional hex value at the end of each line for the
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 363db9d8ce..9206fb299d 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -327,9 +327,6 @@ class TestFunctional(unittest.TestCase):
     def _HandleCommandObjdump(self, args):
         return command.CommandResult(return_code=0)
 
-    def _HandleCommandObjcopy(self, args):
-        return command.CommandResult(return_code=0)
-
     def _HandleCommandSize(self, args):
         return command.CommandResult(return_code=0)
 
@@ -362,8 +359,6 @@ class TestFunctional(unittest.TestCase):
             return self._HandleCommandNm(args)
         elif cmd.endswith('objdump'):
             return self._HandleCommandObjdump(args)
-        elif cmd.endswith('objcopy'):
-            return self._HandleCommandObjcopy(args)
         elif cmd.endswith( 'size'):
             return self._HandleCommandSize(args)
 
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v6 13/13] Revert "buildman: Extract environment as part of each build"

[Simon Glass]

Hi,

On 12 June 2018 at 20:37, Simon Glass <sjg@chromium.org> wrote:
> This reverts commit 0ddc510ea37aa578b8cb197840a5125409978bec.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>

Sorry, please ignore this patch. I am trying to track down a buildman
issue and put this commit on top just before sending :-(

Regards,
Simon