[U-Boot] [PATCH v8 00/30] efi: Enable sandbox support for EFI loader

[U-Boot] [PATCH v8 00/30] efi: Enable 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.

Also included in v8 is support for running the full EFI self tests. These
run OK with some tweaks to a few parts of the code.

This series is at u-boot-dm/efi-working

Changes in v8:
- Expand series substantially to support bootefi selftest
- Rebase to master

Changes in v7:
- Drop an unnecessary comment
- Drop patch "efi: Init the 'rows' and 'cols' variables"
- Drop patches previous applied
- Move some of the code from efi_memory_init() into a separate function
- Update patch subject s/builder/build/

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()
- Introduce load_options_path to specifyc U-Boot env var for load_options_path
- Rebase to master

Changes in v4:
- Rebase to master
- Update SPDX tags

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

Alexander Graf (4):
  efi_loader: Use map_sysmem() in bootefi command
  efi_loader: Use compiler constants for image loader
  efi_loader: Disable miniapps on sandbox
  efi_loader: Pass address to fs_read()

Heinrich Schuchardt (1):
  efi_loader: efi_allocate_pages is too restrictive

Simon Glass (25):
  efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox
  efi: sandbox: Adjust memory usage for sandbox
  sandbox: smbios: Update to support sandbox
  efi: sandbox: Add distroboot support
  efi: sandbox: Add relocation constants
  efi: sandbox: Enable EFI loader build 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()
  sandbox: Align RAM buffer to the machine page size
  sandbox: Try to start the RAM buffer at a particular address
  sandbox: Add support for calling abort()
  efi: Don't build sandbox with __attribute__((ms_abi))
  vsprintf: Handle NULL with %pU
  efi_selftest: Clean up a few comments and messages
  sandbox: Enhance map_to_sysmem() to handle foreign pointers
  efi: Add a call to exit() along with why we can't use it
  efi: Relocate FDT to 127MB instead of 128MB
  efi: Tidy up device-tree-size calculation in copy_fdt()
  efi: sandbox: Tidy up copy_fdt() to work with sandbox
  efi: Drop error return in efi_carve_out_dt_rsv()
  efi: Adjust memory handling to support sandbox
  efi: Add more debugging for memory allocations
  efi: sandbox: Enable selftest command

 arch/sandbox/cpu/cpu.c            | 141 +++++++++++++++-
 arch/sandbox/cpu/os.c             |  17 +-
 arch/sandbox/cpu/state.c          |   8 +
 arch/sandbox/include/asm/state.h  |  21 +++
 cmd/bootefi.c                     | 270 ++++++++++++++++++++----------
 configs/sandbox_defconfig         |   1 +
 include/config_distro_bootcmd.h   |  11 ++
 include/efi.h                     |  19 ++-
 include/efi_loader.h              |   3 +
 include/os.h                      |   4 +
 lib/efi_loader/Kconfig            |  12 +-
 lib/efi_loader/Makefile           |   1 +
 lib/efi_loader/efi_boottime.c     |  31 +++-
 lib/efi_loader/efi_file.c         |   5 +-
 lib/efi_loader/efi_image_loader.c |  15 +-
 lib/efi_loader/efi_memory.c       |  57 +++++--
 lib/efi_loader/efi_runtime.c      |  11 ++
 lib/efi_loader/efi_test.c         |  26 +++
 lib/efi_selftest/Makefile         |   2 +-
 lib/efi_selftest/efi_selftest.c   |  14 +-
 lib/smbios.c                      |  32 +++-
 lib/vsprintf.c                    |   5 +-
 22 files changed, 562 insertions(+), 144 deletions(-)
 create mode 100644 lib/efi_loader/efi_test.c

-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 01/30] 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 v8: None
Changes in v7: None
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 v8 01/30] efi: Don't allow CMD_BOOTEFI_SELFTEST on sandbox

[Heinrich Schuchardt]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> This does not work at present and gives the following error:
> 
> output: 'ld.bfd: read in flex scanner failed
> scripts/Makefile.lib:390: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_return_efi.so' failed
> 
> It may be possible to figure this out with suitable linker magic but it
> does not seem to be easy. Also, we will be able to run the tests on
> sandbox without using the miniapp.
> 
> So for now at least, disable this option.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v8: None
> Changes in v7: None
> 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
> 

With Alex's patch series the sandbox can execute the selftests except
those requiring CRT0 and this is what I expect to be delivered by
whatever series is merged. Concerning CRT0 the build system should be
adjusted to build CRT0 according to the host architecture.

Best regards

Heinrich

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

[Simon Glass]

Hi Heinrich,

On 20 June 2018 at 00:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>> This does not work at present and gives the following error:
>>
>> output: 'ld.bfd: read in flex scanner failed
>> scripts/Makefile.lib:390: recipe for target 'lib/efi_selftest/efi_selftest_miniapp_return_efi.so' failed
>>
>> It may be possible to figure this out with suitable linker magic but it
>> does not seem to be easy. Also, we will be able to run the tests on
>> sandbox without using the miniapp.
>>
>> So for now at least, disable this option.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> 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
>>
>
> With Alex's patch series the sandbox can execute the selftests except
> those requiring CRT0 and this is what I expect to be delivered by
> whatever series is merged. Concerning CRT0 the build system should be
> adjusted to build CRT0 according to the host architecture.

If you look at the end of my series I have a patch to enable it (and
reverse this). Assuming that we get that far, this patch can be
dropped. But given the very slow review progress and disagreement on
how sandbox should work, I feel that this patch is a good stopgap to
reflect current reality.

Regards,
Simon

[U-Boot] [PATCH v8 02/30] 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 v8: None
Changes in v7:
- Move some of the code from efi_memory_init() into a separate function

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 | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index ec66af98ea..c6410613c7 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;
 	}
@@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void)
 	}
 }
 
-int efi_memory_init(void)
+/* Add memory regions for U-Boot's memory and for the runtime services code */
+static void add_u_boot_and_runtime(void)
 {
 	unsigned long runtime_start, runtime_end, runtime_pages;
 	unsigned long uboot_start, uboot_pages;
 	unsigned long uboot_stack_size = 16 * 1024 * 1024;
 
-	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;
@@ -516,6 +516,14 @@ int efi_memory_init(void)
 	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
 	efi_add_memory_map(runtime_start, runtime_pages,
 			   EFI_RUNTIME_SERVICES_CODE, false);
+}
+
+int efi_memory_init(void)
+{
+	efi_add_known_memory();
+
+	if (!IS_ENABLED(CONFIG_SANDBOX))
+		add_u_boot_and_runtime();
 
 #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
 	/* Request a 32bit 64MB bounce buffer region */
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Heinrich Schuchardt]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> With sandbox the U-Boot code is not mapped into the sandbox memory range
> so does not need to be excluded when allocating EFI memory. Update the EFI
> memory init code to take account of that.
> 
> Also use mapmem instead of a cast to convert a memory address to a
> pointer.

This is not reflected in the patch.

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v8: None
> Changes in v7:
> - Move some of the code from efi_memory_init() into a separate function
> 
> 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 | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index ec66af98ea..c6410613c7 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>

I cannot see any use of this include in the patch.

>  #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;
>  	}
> @@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void)
>  	}
>  }
>  
> -int efi_memory_init(void)
> +/* Add memory regions for U-Boot's memory and for the runtime services code */
> +static void add_u_boot_and_runtime(void)
>  {
>  	unsigned long runtime_start, runtime_end, runtime_pages;
>  	unsigned long uboot_start, uboot_pages;
>  	unsigned long uboot_stack_size = 16 * 1024 * 1024;
>  
> -	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;
> @@ -516,6 +516,14 @@ int efi_memory_init(void)
>  	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>  	efi_add_memory_map(runtime_start, runtime_pages,
>  			   EFI_RUNTIME_SERVICES_CODE, false);
> +}
> +
> +int efi_memory_init(void)
> +{
> +	efi_add_known_memory();
> +
> +	if (!IS_ENABLED(CONFIG_SANDBOX))
> +		add_u_boot_and_runtime();

Is the sandbox not using relocation? A comment is missing in the code
here to explain why add_u_boot_and_runtime() should not be called for
the sandbox.

Best regards

Heinrich

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

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>> so does not need to be excluded when allocating EFI memory. Update the EFI
>> memory init code to take account of that.
>>
>> Also use mapmem instead of a cast to convert a memory address to a
>> pointer.
> This is not reflected in the patch.
>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7:
>> - Move some of the code from efi_memory_init() into a separate function
>>
>> 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 | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index ec66af98ea..c6410613c7 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>
> I cannot see any use of this include in the patch.
>
>>   #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK 
on the patch as-is.


Alex

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>
>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>> so does not need to be excluded when allocating EFI memory. Update the
>>> EFI
>>> memory init code to take account of that.
>>>
>>> Also use mapmem instead of a cast to convert a memory address to a
>>> pointer.
>>
>> This is not reflected in the patch.
>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v8: None
>>> Changes in v7:
>>> - Move some of the code from efi_memory_init() into a separate function
>>>
>>> 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 | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>> index ec66af98ea..c6410613c7 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>
>>
>> I cannot see any use of this include in the patch.
>>
>>>   #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK on
> the patch as-is.

What is wrong with it?

Regards,
Simon

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>> EFI
>>>> memory init code to take account of that.
>>>>
>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>> pointer.
>>> This is not reflected in the patch.
>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v8: None
>>>> Changes in v7:
>>>> - Move some of the code from efi_memory_init() into a separate function
>>>>
>>>> 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 | 16 ++++++++++++----
>>>>    1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>>> index ec66af98ea..c6410613c7 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>
>>> I cannot see any use of this include in the patch.
>>>
>>>>    #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK on
>> the patch as-is.
> What is wrong with it?

Efi_allocate_pool calls efi_allocate_pages() which according to spec 
returns a pointer. So efi_allocate_pool() should not have to map 
anything, because it does not receive an addres.


Alex

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:52, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>>>
>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>
>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>> range
>>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>>> EFI
>>>>> memory init code to take account of that.
>>>>>
>>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>>> pointer.
>>>>
>>>> This is not reflected in the patch.
>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v8: None
>>>>> Changes in v7:
>>>>> - Move some of the code from efi_memory_init() into a separate function
>>>>>
>>>>> 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 | 16 ++++++++++++----
>>>>>    1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>>>> index ec66af98ea..c6410613c7 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>
>>>>
>>>> I cannot see any use of this include in the patch.
>>>>
>>>>>    #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK
>>> on
>>> the patch as-is.
>>
>> What is wrong with it?
>
>
> Efi_allocate_pool calls efi_allocate_pages() which according to spec returns
> a pointer. So efi_allocate_pool() should not have to map anything, because
> it does not receive an addres.

You are referring to efi_allocate_pages_ext() I suspect. In my series
this does the mapping, so that efi_allocate_pages() uses addresses
only. We could rename it if you like.

Regards,
Simon

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:52, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>>> range
>>>>>> so does not need to be excluded when allocating EFI memory. Update the
>>>>>> EFI
>>>>>> memory init code to take account of that.
>>>>>>
>>>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>>>> pointer.
>>>>> This is not reflected in the patch.
>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v8: None
>>>>>> Changes in v7:
>>>>>> - Move some of the code from efi_memory_init() into a separate function
>>>>>>
>>>>>> 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 | 16 ++++++++++++----
>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>>>>> index ec66af98ea..c6410613c7 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>
>>>>> I cannot see any use of this include in the patch.
>>>>>
>>>>>>     #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK
>>>> on
>>>> the patch as-is.
>>> What is wrong with it?
>>
>> Efi_allocate_pool calls efi_allocate_pages() which according to spec returns
>> a pointer. So efi_allocate_pool() should not have to map anything, because
>> it does not receive an addres.
> You are referring to efi_allocate_pages_ext() I suspect. In my series
> this does the mapping, so that efi_allocate_pages() uses addresses
> only. We could rename it if you like.

I don't think we should rename things there. I really think there is a 
fundamental misunderstanding here on what the APIs are supposed to do. 
Basically:

   efi_allocate_pages() is mmap() in Liunx
   efi_allocate_pool() is malloc() in Linux

plus a few extra features of course, but you can think of them at the 
same level.

When a payload calls efi_allocate_pool, that really calls into a more 
sophisticated memory allocator usually which can allocate small chunks 
of memory efficiently. Given the amount of RAM modern systems have, I 
opted for simplicity though so I just mapped it to basically allocate 
pages using efi_allocate_pages().

As for _ext functions, the *only* thing we want in an _ext function is 
to isolate the logic that EFI_CALL() would do otherwise - namely swap 
gd. No more.


Alex

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:08, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 03:52, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>>>>>
>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>>>> range
>>>>>>> so does not need to be excluded when allocating EFI memory. Update
>>>>>>> the
>>>>>>> EFI
>>>>>>> memory init code to take account of that.
>>>>>>>
>>>>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>>>>> pointer.
>>>>>>
>>>>>> This is not reflected in the patch.
>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v8: None
>>>>>>> Changes in v7:
>>>>>>> - Move some of the code from efi_memory_init() into a separate
>>>>>>> function
>>>>>>>
>>>>>>> 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 | 16 ++++++++++++----
>>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/efi_loader/efi_memory.c
>>>>>>> b/lib/efi_loader/efi_memory.c
>>>>>>> index ec66af98ea..c6410613c7 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>
>>>>>>
>>>>>> I cannot see any use of this include in the patch.
>>>>>>
>>>>>>>     #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK
>>>>> on
>>>>> the patch as-is.
>>>>
>>>> What is wrong with it?
>>>
>>>
>>> Efi_allocate_pool calls efi_allocate_pages() which according to spec
>>> returns
>>> a pointer. So efi_allocate_pool() should not have to map anything,
>>> because
>>> it does not receive an addres.
>>
>> You are referring to efi_allocate_pages_ext() I suspect. In my series
>> this does the mapping, so that efi_allocate_pages() uses addresses
>> only. We could rename it if you like.
>
>
> I don't think we should rename things there. I really think there is a
> fundamental misunderstanding here on what the APIs are supposed to do.
> Basically:
>
>   efi_allocate_pages() is mmap() in Liunx
>   efi_allocate_pool() is malloc() in Linux
>
> plus a few extra features of course, but you can think of them at the same
> level.
>
> When a payload calls efi_allocate_pool, that really calls into a more
> sophisticated memory allocator usually which can allocate small chunks of
> memory efficiently. Given the amount of RAM modern systems have, I opted for
> simplicity though so I just mapped it to basically allocate pages using
> efi_allocate_pages().
>
> As for _ext functions, the *only* thing we want in an _ext function is to
> isolate the logic that EFI_CALL() would do otherwise - namely swap gd. No
> more.

Well, we can move things around, but fundamentally I think the current
efi_allocate_pages() needs something that maps its memory. In my patch
I put this in the ..._ext() function. I feel that the uint64_t *
prarameter of efi_allocate_pages() is really confusing, since it
implies an address rather than a pointer.

My vote would be to rename the function entirely and change the API to
work with pointers (doing the mapping inside itself).

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

On 22.06.18 21:28, Simon Glass wrote:
> Hi Alex,
> 
> On 22 June 2018 at 06:08, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>
>>> Hi Alex,
>>>
>>> On 21 June 2018 at 03:52, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>>>>>>
>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>>>>> range
>>>>>>>> so does not need to be excluded when allocating EFI memory. Update
>>>>>>>> the
>>>>>>>> EFI
>>>>>>>> memory init code to take account of that.
>>>>>>>>
>>>>>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>>>>>> pointer.
>>>>>>>
>>>>>>> This is not reflected in the patch.
>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v8: None
>>>>>>>> Changes in v7:
>>>>>>>> - Move some of the code from efi_memory_init() into a separate
>>>>>>>> function
>>>>>>>>
>>>>>>>> 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 | 16 ++++++++++++----
>>>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/efi_loader/efi_memory.c
>>>>>>>> b/lib/efi_loader/efi_memory.c
>>>>>>>> index ec66af98ea..c6410613c7 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>
>>>>>>>
>>>>>>> I cannot see any use of this include in the patch.
>>>>>>>
>>>>>>>>     #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK
>>>>>> on
>>>>>> the patch as-is.
>>>>>
>>>>> What is wrong with it?
>>>>
>>>>
>>>> Efi_allocate_pool calls efi_allocate_pages() which according to spec
>>>> returns
>>>> a pointer. So efi_allocate_pool() should not have to map anything,
>>>> because
>>>> it does not receive an addres.
>>>
>>> You are referring to efi_allocate_pages_ext() I suspect. In my series
>>> this does the mapping, so that efi_allocate_pages() uses addresses
>>> only. We could rename it if you like.
>>
>>
>> I don't think we should rename things there. I really think there is a
>> fundamental misunderstanding here on what the APIs are supposed to do.
>> Basically:
>>
>>   efi_allocate_pages() is mmap() in Liunx
>>   efi_allocate_pool() is malloc() in Linux
>>
>> plus a few extra features of course, but you can think of them at the same
>> level.
>>
>> When a payload calls efi_allocate_pool, that really calls into a more
>> sophisticated memory allocator usually which can allocate small chunks of
>> memory efficiently. Given the amount of RAM modern systems have, I opted for
>> simplicity though so I just mapped it to basically allocate pages using
>> efi_allocate_pages().
>>
>> As for _ext functions, the *only* thing we want in an _ext function is to
>> isolate the logic that EFI_CALL() would do otherwise - namely swap gd. No
>> more.
> 
> Well, we can move things around, but fundamentally I think the current
> efi_allocate_pages() needs something that maps its memory. In my patch
> I put this in the ..._ext() function. I feel that the uint64_t *
> prarameter of efi_allocate_pages() is really confusing, since it
> implies an address rather than a pointer.

I agree that it's confusing, but it's probably the only thing that works
when running on a 32bit platform.

> My vote would be to rename the function entirely and change the API to
> work with pointers (doing the mapping inside itself).

The problem I see there is that we're trying to do a shim that is as
tiny as possible for real efi functionality. I don't know if splitting
the function is going to give us real improvements in readability or
maintainability, since there will be people who will come from an EFI
background and grasp what the EFI functions do, but not our internal layers.

What we could do is something the other way around: An internal wrapper
on top of efi_allocate_pages. Something like efi_get_mem() which takes
an address in one parameter and returns a pointer in another. But that
function would just call efi_allocate_pages(). If that again is a static
inline function, code size shouldn't even change.


Alex

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 23 June 2018 at 01:31, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 22.06.18 21:28, Simon Glass wrote:
>> Hi Alex,
>>
>> On 22 June 2018 at 06:08, Alexander Graf <agraf@suse.de> wrote:
>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 21 June 2018 at 03:52, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 20 June 2018 at 02:54, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/20/2018 08:10 AM, Heinrich Schuchardt wrote:
>>>>>>>>
>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> With sandbox the U-Boot code is not mapped into the sandbox memory
>>>>>>>>> range
>>>>>>>>> so does not need to be excluded when allocating EFI memory. Update
>>>>>>>>> the
>>>>>>>>> EFI
>>>>>>>>> memory init code to take account of that.
>>>>>>>>>
>>>>>>>>> Also use mapmem instead of a cast to convert a memory address to a
>>>>>>>>> pointer.
>>>>>>>>
>>>>>>>> This is not reflected in the patch.
>>>>>>>>
>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes in v8: None
>>>>>>>>> Changes in v7:
>>>>>>>>> - Move some of the code from efi_memory_init() into a separate
>>>>>>>>> function
>>>>>>>>>
>>>>>>>>> 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 | 16 ++++++++++++----
>>>>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/efi_loader/efi_memory.c
>>>>>>>>> b/lib/efi_loader/efi_memory.c
>>>>>>>>> index ec66af98ea..c6410613c7 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>
>>>>>>>>
>>>>>>>> I cannot see any use of this include in the patch.
>>>>>>>>
>>>>>>>>>     #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 where mapmem.h gets used. And yes, it's the wrong place. So NAK
>>>>>>> on
>>>>>>> the patch as-is.
>>>>>>
>>>>>> What is wrong with it?
>>>>>
>>>>>
>>>>> Efi_allocate_pool calls efi_allocate_pages() which according to spec
>>>>> returns
>>>>> a pointer. So efi_allocate_pool() should not have to map anything,
>>>>> because
>>>>> it does not receive an addres.
>>>>
>>>> You are referring to efi_allocate_pages_ext() I suspect. In my series
>>>> this does the mapping, so that efi_allocate_pages() uses addresses
>>>> only. We could rename it if you like.
>>>
>>>
>>> I don't think we should rename things there. I really think there is a
>>> fundamental misunderstanding here on what the APIs are supposed to do.
>>> Basically:
>>>
>>>   efi_allocate_pages() is mmap() in Liunx
>>>   efi_allocate_pool() is malloc() in Linux
>>>
>>> plus a few extra features of course, but you can think of them at the same
>>> level.
>>>
>>> When a payload calls efi_allocate_pool, that really calls into a more
>>> sophisticated memory allocator usually which can allocate small chunks of
>>> memory efficiently. Given the amount of RAM modern systems have, I opted for
>>> simplicity though so I just mapped it to basically allocate pages using
>>> efi_allocate_pages().
>>>
>>> As for _ext functions, the *only* thing we want in an _ext function is to
>>> isolate the logic that EFI_CALL() would do otherwise - namely swap gd. No
>>> more.
>>
>> Well, we can move things around, but fundamentally I think the current
>> efi_allocate_pages() needs something that maps its memory. In my patch
>> I put this in the ..._ext() function. I feel that the uint64_t *
>> prarameter of efi_allocate_pages() is really confusing, since it
>> implies an address rather than a pointer.
>
> I agree that it's confusing, but it's probably the only thing that works
> when running on a 32bit platform.
>
>> My vote would be to rename the function entirely and change the API to
>> work with pointers (doing the mapping inside itself).
>
> The problem I see there is that we're trying to do a shim that is as
> tiny as possible for real efi functionality. I don't know if splitting
> the function is going to give us real improvements in readability or
> maintainability, since there will be people who will come from an EFI
> background and grasp what the EFI functions do, but not our internal layers.

I'm not sure either, but it might be worth taking a look once the dust settles.

>
> What we could do is something the other way around: An internal wrapper
> on top of efi_allocate_pages. Something like efi_get_mem() which takes
> an address in one parameter and returns a pointer in another. But that
> function would just call efi_allocate_pages(). If that again is a static
> inline function, code size shouldn't even change.

Well I think the problem is the API using addresses in one method and
pointers in another. We can't fix that.

Maybe that would work. One of us should take a look down the track.

Regards,
Simon

[U-Boot] [PATCH v8 02/30] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Heinrich,

On 20 June 2018 at 00:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>> With sandbox the U-Boot code is not mapped into the sandbox memory range
>> so does not need to be excluded when allocating EFI memory. Update the EFI
>> memory init code to take account of that.
>>
>> Also use mapmem instead of a cast to convert a memory address to a
>> pointer.
>
> This is not reflected in the patch.
>
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7:
>> - Move some of the code from efi_memory_init() into a separate function
>>
>> 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 | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index ec66af98ea..c6410613c7 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>
>
> I cannot see any use of this include in the patch.

Please see ^^^^^^^ below, for example.

>
>>  #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;
>>       }
>> @@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void)
>>       }
>>  }
>>
>> -int efi_memory_init(void)
>> +/* Add memory regions for U-Boot's memory and for the runtime services code */
>> +static void add_u_boot_and_runtime(void)
>>  {
>>       unsigned long runtime_start, runtime_end, runtime_pages;
>>       unsigned long uboot_start, uboot_pages;
>>       unsigned long uboot_stack_size = 16 * 1024 * 1024;
>>
>> -     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;
>> @@ -516,6 +516,14 @@ int efi_memory_init(void)
>>       runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>>       efi_add_memory_map(runtime_start, runtime_pages,
>>                          EFI_RUNTIME_SERVICES_CODE, false);
>> +}
>> +
>> +int efi_memory_init(void)
>> +{
>> +     efi_add_known_memory();
>> +
>> +     if (!IS_ENABLED(CONFIG_SANDBOX))
>> +             add_u_boot_and_runtime();
>
> Is the sandbox not using relocation? A comment is missing in the code
> here to explain why add_u_boot_and_runtime() should not be called for
> the sandbox.

That's right, sandbox does not use relocation. But the real point is
that its code is not within the sandbox memory map. Sandbox uses a
emulated RAM buffer, separate from the U-Boot code and stack.

Regards,
Simon

[U-Boot] [PATCH v8 03/30] 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 v8: None
Changes in v7: None
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 v8 04/30] 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 v8: None
Changes in v7:
- Drop an unnecessary comment

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index d672e8ebe6..75866f2abf 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -261,6 +261,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 v8 05/30] 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 v8: None
Changes in v7:
- Drop an unnecessary comment

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 4874eb602f..10a7878a1c 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -62,6 +62,17 @@ struct dyn_sym {
 #define R_ABSOLUTE	R_RISCV_64
 #define SYM_INDEX	32
 #endif
+
+#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 v8 06/30] efi: sandbox: Enable EFI loader build 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 v8: None
Changes in v7:
- Update patch subject s/builder/build/

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 v8 07/30] 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 v8: None
Changes in v7: None
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 f55a40dc84..2dfd297e78 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -356,6 +356,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;
@@ -434,31 +488,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 v8 07/30] efi: Split out test init/uninit into functions

[Heinrich Schuchardt]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> 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 v8: None
> Changes in v7: None
> 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 f55a40dc84..2dfd297e78 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -356,6 +356,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));

It is unclear why you are adding the memsets here.

> +	/* Construct a dummy device path */
> +	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					      (uintptr_t)test_func,
> +					      (uintptr_t)test_func);

efi_dp_from_mem() may return NULL in case of an error.

> +	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> +	efi_setup_loaded_image(image, obj, bootefi_device_path,
> +			       bootefi_image_path);

The return code of efi_setup_loaded_image() should be checked. This
function should not return 0 if the return code is not EFI_SUCCESS.

Best regards

Heinrich

> +	/*
> +	 * 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;
> @@ -434,31 +488,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
> 

[U-Boot] [PATCH v8 07/30] efi: Split out test init/uninit into functions

[Simon Glass]

Hi Heinrich,

On 20 June 2018 at 00:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>> 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 v8: None
>> Changes in v7: None
>> 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 f55a40dc84..2dfd297e78 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -356,6 +356,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));
>
> It is unclear why you are adding the memsets here.

It's because they are passed in as pointers by callers, and otherwise
would not inited.

I will add a comment.

>
>> +     /* Construct a dummy device path */
>> +     bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> +                                           (uintptr_t)test_func,
>> +                                           (uintptr_t)test_func);
>
> efi_dp_from_mem() may return NULL in case of an error.
>
>> +     bootefi_image_path = efi_dp_from_file(NULL, 0, path);
>> +     efi_setup_loaded_image(image, obj, bootefi_device_path,
>> +                            bootefi_image_path);
>
> The return code of efi_setup_loaded_image() should be checked. This
> function should not return 0 if the return code is not EFI_SUCCESS.

I copied the existing code, which does not check the return value
anywhere. Has this function changed, or is the current code wrong?

Regards,
Simon

[U-Boot] [PATCH v8 08/30] 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 v8:
- Expand series substantially to support bootefi selftest
- Rebase to master

Changes in v7: None
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 2dfd297e78..7faa3ea245 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -356,7 +356,6 @@ exit:
 	return ret;
 }
 
-#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 /**
  * bootefi_test_prepare() - prepare to run an EFI test
  *
@@ -408,7 +407,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)
 {
@@ -440,8 +438,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;
 
@@ -486,11 +486,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 v8 09/30] 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 v8: None
Changes in v7: None
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 7faa3ea245..c174e5e8fc 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_object mem_obj = {};
 	struct efi_device_path *memdp = NULL;
 	efi_status_t ret;
@@ -290,19 +310,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;
@@ -310,10 +324,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: */
@@ -323,8 +337,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;
 	}
 
@@ -336,7 +350,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);
 
@@ -345,11 +359,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);
 	if (mem_obj.handle)
 		list_del(&mem_obj.link);
 
@@ -367,11 +381,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));
@@ -380,18 +396,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);
 }
 
 /**
@@ -490,7 +496,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);
@@ -506,7 +512,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 v8 10/30] 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 v8: None
Changes in v7:
- Drop patch "efi: Init the 'rows' and 'cols' variables"
- Drop patches previous applied

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 c174e5e8fc..3fa4f181ba 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.
@@ -362,8 +376,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);
 	if (mem_obj.handle)
 		list_del(&mem_obj.link);
 
@@ -400,20 +413,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;
@@ -500,7 +499,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;
@@ -518,7 +517,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 v8 11/30] sandbox: Align RAM buffer to the machine page size

[Simon Glass]

At present the sandbox RAM buffer is not aligned to any particular
address boundary. This makes the internal pointers somewhat random with
respect to the associated RAM buffer addresses.

Align the buffer to the page size of the machine to help with this.

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

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

 arch/sandbox/cpu/os.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 5839932b00..a1a982af2d 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -143,14 +143,15 @@ void os_tty_raw(int fd, bool allow_sigs)
 void *os_malloc(size_t length)
 {
 	struct os_mem_hdr *hdr;
+	int page_size = getpagesize();
 
-	hdr = mmap(NULL, length + sizeof(*hdr), PROT_READ | PROT_WRITE,
-		   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	hdr = mmap(NULL, length + page_size,
+		   PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 	if (hdr == MAP_FAILED)
 		return NULL;
 	hdr->length = length;
 
-	return hdr + 1;
+	return (void *)hdr + page_size;
 }
 
 void os_free(void *ptr)
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Use a starting address of 256MB which should be available. This helps to
make sandbox RAM buffers pointers more recognisable.

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

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

 arch/sandbox/cpu/os.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index a1a982af2d..1553aa687d 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -145,7 +145,12 @@ void *os_malloc(size_t length)
 	struct os_mem_hdr *hdr;
 	int page_size = getpagesize();
 
-	hdr = mmap(NULL, length + page_size,
+	/*
+	 * Use an address that is hopefully available to us so that pointers
+	 * to this memory are fairly obvious. If we end up with a different
+	 * address, that's fine too.
+	 */
+	hdr = mmap((void *)0x10000000, length + page_size,
 		   PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 	if (hdr == MAP_FAILED)
 		return NULL;
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> Use a starting address of 256MB which should be available. This helps to
> make sandbox RAM buffers pointers more recognisable.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Nak, this has a non-0 chance of failing, in case something else is 
already mapped at that address. You don't want to have your CI blow up 
1% of the time due to this.


Alex

> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   arch/sandbox/cpu/os.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index a1a982af2d..1553aa687d 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -145,7 +145,12 @@ void *os_malloc(size_t length)
>   	struct os_mem_hdr *hdr;
>   	int page_size = getpagesize();
>   
> -	hdr = mmap(NULL, length + page_size,
> +	/*
> +	 * Use an address that is hopefully available to us so that pointers
> +	 * to this memory are fairly obvious. If we end up with a different
> +	 * address, that's fine too.
> +	 */
> +	hdr = mmap((void *)0x10000000, length + page_size,
>   		   PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>   	if (hdr == MAP_FAILED)
>   		return NULL;

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Hi Alex,

On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> Use a starting address of 256MB which should be available. This helps to
>> make sandbox RAM buffers pointers more recognisable.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>
> Nak, this has a non-0 chance of failing, in case something else is already
> mapped at that address. You don't want to have your CI blow up 1% of the
> time due to this.

It's just a hint though. Everything will still work if it doesn't get
this exact address.

Regards,
Simon

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Alexander Graf]

On 06/20/2018 12:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> Use a starting address of 256MB which should be available. This helps to
>>> make sandbox RAM buffers pointers more recognisable.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> Nak, this has a non-0 chance of failing, in case something else is already
>> mapped at that address. You don't want to have your CI blow up 1% of the
>> time due to this.
> It's just a hint though. Everything will still work if it doesn't get
> this exact address.

I don't see what it buys us then.


Alex

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Hi Alex,

On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> Use a starting address of 256MB which should be available. This helps to
>>>> make sandbox RAM buffers pointers more recognisable.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>>
>>> Nak, this has a non-0 chance of failing, in case something else is
>>> already
>>> mapped at that address. You don't want to have your CI blow up 1% of the
>>> time due to this.
>>
>> It's just a hint though. Everything will still work if it doesn't get
>> this exact address.
>
>
> I don't see what it buys us then.

These are my thoughts:

1. We get an address before 4GB which is needed for grub (so far as I can tell)
2. It will normally be a nice, easy-on-the-eyes address
3. If it isn't, it will hopefully still be an address below 4GB
4. Even if not (e.g. on some very strange system) it will still allow
sandbox to start up

But anyway, as I said, this will never fail, it is just a hint. So I
think your original comment is incorrect.

Regards,
Simon

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Alexander Graf]

On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>> Use a starting address of 256MB which should be available. This helps to
>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>> already
>>>> mapped at that address. You don't want to have your CI blow up 1% of the
>>>> time due to this.
>>> It's just a hint though. Everything will still work if it doesn't get
>>> this exact address.
>>
>> I don't see what it buys us then.
> These are my thoughts:
>
> 1. We get an address before 4GB which is needed for grub (so far as I can tell)

We only need that in the memory map which you want virtual (U-Boot 
address space) anyway. So there's no need to also have the Linux address 
be <4GB.

> 2. It will normally be a nice, easy-on-the-eyes address

I'd rather say it's going to confuse people, because now it's easy to 
mistake for a U-Boot address.

> 3. If it isn't, it will hopefully still be an address below 4GB
> 4. Even if not (e.g. on some very strange system) it will still allow
> sandbox to start up
>
> But anyway, as I said, this will never fail, it is just a hint. So I
> think your original comment is incorrect.

It will not fail, it will just potentially not map at exactly the spot 
you wanted the map at, which IMHO would cause even more confusion.

Imagine people now start to assume that %p output is stable between 
invocations - because it is 99% of the cases. They start to integrate 
that into their CI suites and suddenly things fail 1% of the time 
because the linker figured today's a great day to map glibc at that address.


Alex

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>
>>>>>> Use a starting address of 256MB which should be available. This helps
>>>>>> to
>>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>>>
>>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>>> already
>>>>> mapped at that address. You don't want to have your CI blow up 1% of
>>>>> the
>>>>> time due to this.
>>>>
>>>> It's just a hint though. Everything will still work if it doesn't get
>>>> this exact address.
>>>
>>>
>>> I don't see what it buys us then.
>>
>> These are my thoughts:
>>
>> 1. We get an address before 4GB which is needed for grub (so far as I can
>> tell)
>
>
> We only need that in the memory map which you want virtual (U-Boot address
> space) anyway. So there's no need to also have the Linux address be <4GB.

Grub cannot work without <4GB, right? I don't mind either way, but I
think it that if we are picking an address, picking a smaller one is
better.

>
>> 2. It will normally be a nice, easy-on-the-eyes address
>
>
> I'd rather say it's going to confuse people, because now it's easy to
> mistake for a U-Boot address.

That's possibly true, although at present it is outside the range of
valid U-Boot addresses. What do you suggest?

>
>> 3. If it isn't, it will hopefully still be an address below 4GB
>> 4. Even if not (e.g. on some very strange system) it will still allow
>> sandbox to start up
>>
>> But anyway, as I said, this will never fail, it is just a hint. So I
>> think your original comment is incorrect.
>
>
> It will not fail, it will just potentially not map at exactly the spot you
> wanted the map at, which IMHO would cause even more confusion.
>
> Imagine people now start to assume that %p output is stable between
> invocations - because it is 99% of the cases. They start to integrate that
> into their CI suites and suddenly things fail 1% of the time because the
> linker figured today's a great day to map glibc at that address.

Remember these are internal sandbox addresses. They should not be
visible to most of the code and it is very hard for me to imagine a
test that could fail because the address is mapped in different
places. How would you access the address from a test? Are you thinking
of something that would assume the ram buffer address is something in
particular and then call map_to_sysmem() on the pointer? I am really
not sure that we should worry about that too much.

Otherwise I just cannot imagine what sort of test you are referring to here?

Regards,
Simon

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>> Use a starting address of 256MB which should be available. This helps
>>>>>>> to
>>>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>
>>>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>>>> already
>>>>>> mapped at that address. You don't want to have your CI blow up 1% of
>>>>>> the
>>>>>> time due to this.
>>>>> It's just a hint though. Everything will still work if it doesn't get
>>>>> this exact address.
>>>>
>>>> I don't see what it buys us then.
>>> These are my thoughts:
>>>
>>> 1. We get an address before 4GB which is needed for grub (so far as I can
>>> tell)
>>
>> We only need that in the memory map which you want virtual (U-Boot address
>> space) anyway. So there's no need to also have the Linux address be <4GB.
> Grub cannot work without <4GB, right? I don't mind either way, but I
> think it that if we are picking an address, picking a smaller one is
> better.

Only if you expose host pointers as memory addresses. We don't anymore, 
and grub doesn't care whether a pointer is >4G.

>
>>> 2. It will normally be a nice, easy-on-the-eyes address
>>
>> I'd rather say it's going to confuse people, because now it's easy to
>> mistake for a U-Boot address.
> That's possibly true, although at present it is outside the range of
> valid U-Boot addresses. What do you suggest?

I see little reason for change. Also if you want to change it, it's out 
of scope of the efi_loader enablement ;).

>
>>> 3. If it isn't, it will hopefully still be an address below 4GB
>>> 4. Even if not (e.g. on some very strange system) it will still allow
>>> sandbox to start up
>>>
>>> But anyway, as I said, this will never fail, it is just a hint. So I
>>> think your original comment is incorrect.
>>
>> It will not fail, it will just potentially not map at exactly the spot you
>> wanted the map at, which IMHO would cause even more confusion.
>>
>> Imagine people now start to assume that %p output is stable between
>> invocations - because it is 99% of the cases. They start to integrate that
>> into their CI suites and suddenly things fail 1% of the time because the
>> linker figured today's a great day to map glibc at that address.
> Remember these are internal sandbox addresses. They should not be
> visible to most of the code and it is very hard for me to imagine a
> test that could fail because the address is mapped in different
> places. How would you access the address from a test? Are you thinking
> of something that would assume the ram buffer address is something in
> particular and then call map_to_sysmem() on the pointer? I am really
> not sure that we should worry about that too much.

No, I'm mostly worried about people assuming that 2 invocations of a 
U-Boot binary give them the same console output, even when there are 
printf()s that contain a %p.


Alex

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:10, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Use a starting address of 256MB which should be available. This
>>>>>>>> helps
>>>>>>>> to
>>>>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>
>>>>>>>
>>>>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>>>>> already
>>>>>>> mapped at that address. You don't want to have your CI blow up 1% of
>>>>>>> the
>>>>>>> time due to this.
>>>>>>
>>>>>> It's just a hint though. Everything will still work if it doesn't get
>>>>>> this exact address.
>>>>>
>>>>>
>>>>> I don't see what it buys us then.
>>>>
>>>> These are my thoughts:
>>>>
>>>> 1. We get an address before 4GB which is needed for grub (so far as I
>>>> can
>>>> tell)
>>>
>>>
>>> We only need that in the memory map which you want virtual (U-Boot
>>> address
>>> space) anyway. So there's no need to also have the Linux address be <4GB.
>>
>> Grub cannot work without <4GB, right? I don't mind either way, but I
>> think it that if we are picking an address, picking a smaller one is
>> better.
>
>
> Only if you expose host pointers as memory addresses. We don't anymore, and
> grub doesn't care whether a pointer is >4G.

We have to provide grub with pointers to memory with sandbox. These
will be pointers into the sandbox RAM buffer. If this buffer is >=4GB
then grub will not work, from my testing.

>
>>
>>>> 2. It will normally be a nice, easy-on-the-eyes address
>>>
>>>
>>> I'd rather say it's going to confuse people, because now it's easy to
>>> mistake for a U-Boot address.
>>
>> That's possibly true, although at present it is outside the range of
>> valid U-Boot addresses. What do you suggest?
>
>
> I see little reason for change. Also if you want to change it, it's out of
> scope of the efi_loader enablement ;).

Yes it can be dealt with separately.

>
>>
>>>> 3. If it isn't, it will hopefully still be an address below 4GB
>>>> 4. Even if not (e.g. on some very strange system) it will still allow
>>>> sandbox to start up
>>>>
>>>> But anyway, as I said, this will never fail, it is just a hint. So I
>>>> think your original comment is incorrect.
>>>
>>>
>>> It will not fail, it will just potentially not map at exactly the spot
>>> you
>>> wanted the map at, which IMHO would cause even more confusion.
>>>
>>> Imagine people now start to assume that %p output is stable between
>>> invocations - because it is 99% of the cases. They start to integrate
>>> that
>>> into their CI suites and suddenly things fail 1% of the time because the
>>> linker figured today's a great day to map glibc at that address.
>>
>> Remember these are internal sandbox addresses. They should not be
>> visible to most of the code and it is very hard for me to imagine a
>> test that could fail because the address is mapped in different
>> places. How would you access the address from a test? Are you thinking
>> of something that would assume the ram buffer address is something in
>> particular and then call map_to_sysmem() on the pointer? I am really
>> not sure that we should worry about that too much.
>
>
> No, I'm mostly worried about people assuming that 2 invocations of a U-Boot
> binary give them the same console output, even when there are printf()s that
> contain a %p.

We should NEVER printf a %p in code that might be used with sandbox.
That is another point I should have made. U-Boot (including sandbox)
uses addresses and we should print addresses, not pointers.

Regards,
Simon

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Alexander Graf]

> Am 22.06.2018 um 21:28 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 22 June 2018 at 06:10, Alexander Graf <agraf@suse.de> wrote:
>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>> 
>>> Hi Alex,
>>> 
>>>> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>> 
>>>>> Hi Alex,
>>>>> 
>>>>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
>>>>>> 
>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>> 
>>>>>>> Hi Alex,
>>>>>>> 
>>>>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>> 
>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>> 
>>>>>>>>> Use a starting address of 256MB which should be available. This
>>>>>>>>> helps
>>>>>>>>> to
>>>>>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>>>>>> already
>>>>>>>> mapped at that address. You don't want to have your CI blow up 1% of
>>>>>>>> the
>>>>>>>> time due to this.
>>>>>>> 
>>>>>>> It's just a hint though. Everything will still work if it doesn't get
>>>>>>> this exact address.
>>>>>> 
>>>>>> 
>>>>>> I don't see what it buys us then.
>>>>> 
>>>>> These are my thoughts:
>>>>> 
>>>>> 1. We get an address before 4GB which is needed for grub (so far as I
>>>>> can
>>>>> tell)
>>>> 
>>>> 
>>>> We only need that in the memory map which you want virtual (U-Boot
>>>> address
>>>> space) anyway. So there's no need to also have the Linux address be <4GB.
>>> 
>>> Grub cannot work without <4GB, right? I don't mind either way, but I
>>> think it that if we are picking an address, picking a smaller one is
>>> better.
>> 
>> 
>> Only if you expose host pointers as memory addresses. We don't anymore, and
>> grub doesn't care whether a pointer is >4G.
> 
> We have to provide grub with pointers to memory with sandbox. These
> will be pointers into the sandbox RAM buffer. If this buffer is >=4GB
> then grub will not work, from my testing.

It works just fine for me?


Alex

[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Hi Alex,

On 22 June 2018 at 15:29, Alexander Graf <agraf@suse.de> wrote:
>
>
>> Am 22.06.2018 um 21:28 schrieb Simon Glass <sjg@chromium.org>:
>>
>> Hi Alex,
>>
>>> On 22 June 2018 at 06:10, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 21 June 2018 at 03:58, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 20 June 2018 at 02:51, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> Use a starting address of 256MB which should be available. This
>>>>>>>>>> helps
>>>>>>>>>> to
>>>>>>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>>>>>>> already
>>>>>>>>> mapped at that address. You don't want to have your CI blow up 1% of
>>>>>>>>> the
>>>>>>>>> time due to this.
>>>>>>>>
>>>>>>>> It's just a hint though. Everything will still work if it doesn't get
>>>>>>>> this exact address.
>>>>>>>
>>>>>>>
>>>>>>> I don't see what it buys us then.
>>>>>>
>>>>>> These are my thoughts:
>>>>>>
>>>>>> 1. We get an address before 4GB which is needed for grub (so far as I
>>>>>> can
>>>>>> tell)
>>>>>
>>>>>
>>>>> We only need that in the memory map which you want virtual (U-Boot
>>>>> address
>>>>> space) anyway. So there's no need to also have the Linux address be <4GB.
>>>>
>>>> Grub cannot work without <4GB, right? I don't mind either way, but I
>>>> think it that if we are picking an address, picking a smaller one is
>>>> better.
>>>
>>>
>>> Only if you expose host pointers as memory addresses. We don't anymore, and
>>> grub doesn't care whether a pointer is >4G.
>>
>> We have to provide grub with pointers to memory with sandbox. These
>> will be pointers into the sandbox RAM buffer. If this buffer is >=4GB
>> then grub will not work, from my testing.
>
> It works just fine for me?

OK that's strange, but perhaps I am building grub wrong. I'l ltry it
again when things land.

Regards,
Simon

[U-Boot] [PATCH v8 13/30] sandbox: Add support for calling abort()

[Simon Glass]

This function is useful to signal that the application needs to exit
immediate. It can be caught with a debugger (e.g. gdb). Add a stub for it
so that it can be called from within sandbox when an internal error
occurs.

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

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

 arch/sandbox/cpu/os.c | 5 +++++
 include/os.h          | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 1553aa687d..f8d87df7b6 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -657,3 +657,8 @@ void os_longjmp(ulong *jmp, int ret)
 {
 	longjmp((struct __jmp_buf_tag *)jmp, ret);
 }
+
+void os_abort(void)
+{
+	abort();
+}
diff --git a/include/os.h b/include/os.h
index c8e0f52d30..e850f879ec 100644
--- a/include/os.h
+++ b/include/os.h
@@ -351,4 +351,8 @@ int os_setjmp(ulong *jmp, int size);
  */
 void os_longjmp(ulong *jmp, int ret);
 
+/**
+ * os_abort() - Raise SIGABRT to exit sandbox (e.g. to debugger)
+ */
+void os_abort(void);
 #endif
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

There appears to be a bug [1] in gcc when using varargs with this
attribute. Disable it for sandbox so that functions which use that can
work correctly, such as install_multiple_protocol_interfaces().

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955

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

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

 include/efi.h | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index e30a3c51c6..930ea74abe 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -19,11 +19,22 @@
 #include <linux/string.h>
 #include <linux/types.h>
 
-#if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) && defined(__x86_64__))
-/* EFI uses the Microsoft ABI which is not the default for GCC */
-#define EFIAPI __attribute__((ms_abi))
+#ifdef CONFIG_SANDBOX
+/*
+ * Avoid using EFIAPI due to this bug:
+ *
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
+ *
+ * This affects efi_install_multiple_protocol_interfaces().
+ */
+# define EFIAPI
 #else
-#define EFIAPI asmlinkage
+# if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) && defined(__x86_64__))
+/* EFI uses the Microsoft ABI which is not the default for GCC */
+#  define EFIAPI __attribute__((ms_abi))
+# else
+#  define EFIAPI asmlinkage
+# endif
 #endif
 
 struct efi_device_path;
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> There appears to be a bug [1] in gcc when using varargs with this
> attribute. Disable it for sandbox so that functions which use that can
> work correctly, such as install_multiple_protocol_interfaces().
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

See my patch instead please.


Alex

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

Hi Alex,

On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> There appears to be a bug [1] in gcc when using varargs with this
>> attribute. Disable it for sandbox so that functions which use that can
>> work correctly, such as install_multiple_protocol_interfaces().
>>
>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>
> See my patch instead please.

OK I see it now. Do you know what gcc fixes this problem?

Regards,
Simon

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Alexander Graf]

On 06/20/2018 12:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> There appears to be a bug [1] in gcc when using varargs with this
>>> attribute. Disable it for sandbox so that functions which use that can
>>> work correctly, such as install_multiple_protocol_interfaces().
>>>
>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> See my patch instead please.
> OK I see it now. Do you know what gcc fixes this problem?

The bug you found was really just a gcc bug that hit early gcc6 
versions. I doubt you're running into it :).


Alex

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

Hi Alex,

On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>> attribute. Disable it for sandbox so that functions which use that can
>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>
>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>>
>>> See my patch instead please.
>>
>> OK I see it now. Do you know what gcc fixes this problem?
>
>
> The bug you found was really just a gcc bug that hit early gcc6 versions. I
> doubt you're running into it :).

OK, so in fact gcc does not support varargs problems with the ms_abi?

Regards,
Simon

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Alexander Graf]

On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>> attribute. Disable it for sandbox so that functions which use that can
>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>
>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> See my patch instead please.
>>> OK I see it now. Do you know what gcc fixes this problem?
>>
>> The bug you found was really just a gcc bug that hit early gcc6 versions. I
>> doubt you're running into it :).
> OK, so in fact gcc does not support varargs problems with the ms_abi?

Gcc needs to know whether varargs are sysv varargs or ms varargs. And it 
differentiates between the two with different variable types for va_list.


Alex

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>
>>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>>> attribute. Disable it for sandbox so that functions which use that can
>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>
>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>>>
>>>>> See my patch instead please.
>>>>
>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>
>>>
>>> The bug you found was really just a gcc bug that hit early gcc6 versions.
>>> I
>>> doubt you're running into it :).
>>
>> OK, so in fact gcc does not support varargs problems with the ms_abi?
>
>
> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it
> differentiates between the two with different variable types for va_list.
>

Have you seen the builtin_va_list, etc.

Regards,
Simon

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>>>> attribute. Disable it for sandbox so that functions which use that can
>>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>>
>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>
>>>>>> See my patch instead please.
>>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>>
>>>> The bug you found was really just a gcc bug that hit early gcc6 versions.
>>>> I
>>>> doubt you're running into it :).
>>> OK, so in fact gcc does not support varargs problems with the ms_abi?
>>
>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it
>> differentiates between the two with different variable types for va_list.
>>
> Have you seen the builtin_va_list, etc.

I think this sentence is missing content?


Alex

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

Hi Alex,


On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>>>>> attribute. Disable it for sandbox so that functions which use that
>>>>>>>> can
>>>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>>>
>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>
>>>>>>>
>>>>>>> See my patch instead please.
>>>>>>
>>>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>>>
>>>>>
>>>>> The bug you found was really just a gcc bug that hit early gcc6
>>>>> versions.
>>>>> I
>>>>> doubt you're running into it :).
>>>>
>>>> OK, so in fact gcc does not support varargs problems with the ms_abi?
>>>
>>>
>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it
>>> differentiates between the two with different variable types for va_list.
>>>
>> Have you seen the builtin_va_list, etc.
>
>
> I think this sentence is missing content?

I thought that builtin_va_list and friends would work regardless of
the calling standard being used. But it looks (from your patch) like
you have to explicitly use __builtin_ms_va_list. Is that right?

Regards,
Simon

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Alexander Graf]

On 22.06.18 21:28, Simon Glass wrote:
> Hi Alex,
> 
> 
> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>
>>> Hi Alex,
>>>
>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>>>>>> attribute. Disable it for sandbox so that functions which use that
>>>>>>>>> can
>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>>>>
>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>>>>
>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>
>>>>>>>>
>>>>>>>> See my patch instead please.
>>>>>>>
>>>>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>>>>
>>>>>>
>>>>>> The bug you found was really just a gcc bug that hit early gcc6
>>>>>> versions.
>>>>>> I
>>>>>> doubt you're running into it :).
>>>>>
>>>>> OK, so in fact gcc does not support varargs problems with the ms_abi?
>>>>
>>>>
>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it
>>>> differentiates between the two with different variable types for va_list.
>>>>
>>> Have you seen the builtin_va_list, etc.
>>
>>
>> I think this sentence is missing content?
> 
> I thought that builtin_va_list and friends would work regardless of
> the calling standard being used. But it looks (from your patch) like
> you have to explicitly use __builtin_ms_va_list. Is that right?

I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv
va_list, but I'm not 100% sure. I can double check with our compiler
people next week.

Either way, I think this patch is good either way. For starters it's not
gcc specific because it uses the normal va_args in the "normal" case.
Also, it's not ambiguous. IMHO things are quite clear when reading the
code if we explicitly differentiate between sysv and ms_abi va_args.


Alex

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

Hi Alex,

On 23 June 2018 at 01:28, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 22.06.18 21:28, Simon Glass wrote:
>> Hi Alex,
>>
>>
>> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote:
>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>>>>>>> attribute. Disable it for sandbox so that functions which use that
>>>>>>>>>> can
>>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>>>>>
>>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> See my patch instead please.
>>>>>>>>
>>>>>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>>>>>
>>>>>>>
>>>>>>> The bug you found was really just a gcc bug that hit early gcc6
>>>>>>> versions.
>>>>>>> I
>>>>>>> doubt you're running into it :).
>>>>>>
>>>>>> OK, so in fact gcc does not support varargs problems with the ms_abi?
>>>>>
>>>>>
>>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it
>>>>> differentiates between the two with different variable types for va_list.
>>>>>
>>>> Have you seen the builtin_va_list, etc.
>>>
>>>
>>> I think this sentence is missing content?
>>
>> I thought that builtin_va_list and friends would work regardless of
>> the calling standard being used. But it looks (from your patch) like
>> you have to explicitly use __builtin_ms_va_list. Is that right?
>
> I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv
> va_list, but I'm not 100% sure. I can double check with our compiler
> people next week.

OK looking forward to hearing. I'm not sure when the builtin came in,
but if it has been around for a while, and it supports both calling
standards, then it would be nice to use it.

>
> Either way, I think this patch is good either way. For starters it's not
> gcc specific because it uses the normal va_args in the "normal" case.
> Also, it's not ambiguous. IMHO things are quite clear when reading the
> code if we explicitly differentiate between sysv and ms_abi va_args.

I'm OK with it since it gets us going.

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

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Alexander Graf]

On 06/23/2018 04:16 PM, Simon Glass wrote:
> Hi Alex,
>
> On 23 June 2018 at 01:28, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 22.06.18 21:28, Simon Glass wrote:
>>> Hi Alex,
>>>
>>>
>>> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
>>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this
>>>>>>>>>>> attribute. Disable it for sandbox so that functions which use that
>>>>>>>>>>> can
>>>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>>>>>>
>>>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>
>>>>>>>>>> See my patch instead please.
>>>>>>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>>>>>>
>>>>>>>> The bug you found was really just a gcc bug that hit early gcc6
>>>>>>>> versions.
>>>>>>>> I
>>>>>>>> doubt you're running into it :).
>>>>>>> OK, so in fact gcc does not support varargs problems with the ms_abi?
>>>>>>
>>>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it
>>>>>> differentiates between the two with different variable types for va_list.
>>>>>>
>>>>> Have you seen the builtin_va_list, etc.
>>>>
>>>> I think this sentence is missing content?
>>> I thought that builtin_va_list and friends would work regardless of
>>> the calling standard being used. But it looks (from your patch) like
>>> you have to explicitly use __builtin_ms_va_list. Is that right?
>> I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv
>> va_list, but I'm not 100% sure. I can double check with our compiler
>> people next week.
> OK looking forward to hearing. I'm not sure when the builtin came in,
> but if it has been around for a while, and it supports both calling
> standards, then it would be nice to use it.

So according to our toolchain people the builtin is really just the 
internal gcc name for va_list, so it still adheres to the default 
calling ABI in its semantics.

Apparently what one *can* do is to add -mabi=ms to the compiler flags 
which basically makes every function follow the ms abi. If that is set 
*maybe* va_list will also adhere to it.

However, both him and me like the explicit approach (of this patch) better.

He also quickly explained why the function abi can't directly have an 
effect on va_list. Basically at the parsing stage, gcc does not know if 
you want to use a va_list for yourself or to pass it into a function you 
call. And depending on that, you may want either a sysv abi va_list or 
an ms_abi va_list.


Alex

[U-Boot] [PATCH v8 14/30] efi: Don't build sandbox with __attribute__((ms_abi))

[Simon Glass]

Hi Alex,

On 25 June 2018 at 05:23, Alexander Graf <agraf@suse.de> wrote:
> On 06/23/2018 04:16 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 23 June 2018 at 01:28, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 22.06.18 21:28, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>>
>>>> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with
>>>>>>>>>>>> this
>>>>>>>>>>>> attribute. Disable it for sandbox so that functions which use
>>>>>>>>>>>> that
>>>>>>>>>>>> can
>>>>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces().
>>>>>>>>>>>>
>>>>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> See my patch instead please.
>>>>>>>>>>
>>>>>>>>>> OK I see it now. Do you know what gcc fixes this problem?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The bug you found was really just a gcc bug that hit early gcc6
>>>>>>>>> versions.
>>>>>>>>> I
>>>>>>>>> doubt you're running into it :).
>>>>>>>>
>>>>>>>> OK, so in fact gcc does not support varargs problems with the
>>>>>>>> ms_abi?
>>>>>>>
>>>>>>>
>>>>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And
>>>>>>> it
>>>>>>> differentiates between the two with different variable types for
>>>>>>> va_list.
>>>>>>>
>>>>>> Have you seen the builtin_va_list, etc.
>>>>>
>>>>>
>>>>> I think this sentence is missing content?
>>>>
>>>> I thought that builtin_va_list and friends would work regardless of
>>>> the calling standard being used. But it looks (from your patch) like
>>>> you have to explicitly use __builtin_ms_va_list. Is that right?
>>>
>>> I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv
>>> va_list, but I'm not 100% sure. I can double check with our compiler
>>> people next week.
>>
>> OK looking forward to hearing. I'm not sure when the builtin came in,
>> but if it has been around for a while, and it supports both calling
>> standards, then it would be nice to use it.
>
>
> So according to our toolchain people the builtin is really just the internal
> gcc name for va_list, so it still adheres to the default calling ABI in its
> semantics.
>
> Apparently what one *can* do is to add -mabi=ms to the compiler flags which
> basically makes every function follow the ms abi. If that is set *maybe*
> va_list will also adhere to it.
>
> However, both him and me like the explicit approach (of this patch) better.
>
> He also quickly explained why the function abi can't directly have an effect
> on va_list. Basically at the parsing stage, gcc does not know if you want to
> use a va_list for yourself or to pass it into a function you call. And
> depending on that, you may want either a sysv abi va_list or an ms_abi
> va_list.

Eek that sounds complicated, but good to know. So it seems like your
solution is the best option.

Regards,
Simon

[U-Boot] [PATCH v8 15/30] vsprintf: Handle NULL with %pU

[Simon Glass]

At present a NULL pointer passed to printf for a %pU argument will cause
U-Boot to access memory at 0. Fix this by adding a check, and print
"(null)" instead.

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

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

 lib/vsprintf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8b1b29fb5a..2da7f88b18 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -407,7 +407,10 @@ static char *uuid_string(char *buf, char *end, u8 *addr, int field_width,
 		break;
 	}
 
-	uuid_bin_to_str(addr, uuid, str_format);
+	if (addr)
+		uuid_bin_to_str(addr, uuid, str_format);
+	else
+		strcpy(uuid, "(null)");
 
 	return string(buf, end, uuid, field_width, precision, flags);
 }
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 15/30] vsprintf: Handle NULL with %pU

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> At present a NULL pointer passed to printf for a %pU argument will cause
> U-Boot to access memory at 0. Fix this by adding a check, and print
> "(null)" instead.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[U-Boot] [U-Boot,v8,15/30] vsprintf: Handle NULL with %pU

[Alexander Graf]

> At present a NULL pointer passed to printf for a %pU argument will cause
> U-Boot to access memory at 0. Fix this by adding a check, and print
> "(null)" instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [U-Boot,v8,15/30] vsprintf: Handle NULL with %pU

[Heinrich Schuchardt]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> At present a NULL pointer passed to printf for a %pU argument will cause
> U-Boot to access memory at 0. Fix this by adding a check, and print
> "(null)" instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Alexander Graf <agraf@suse.de>
> ---
> 
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  lib/vsprintf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8b1b29fb5a..2da7f88b18 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -407,7 +407,10 @@ static char *uuid_string(char *buf, char *end, u8 *addr, int field_width,
>  		break;
>  	}
>  
> -	uuid_bin_to_str(addr, uuid, str_format);
> +	if (addr)
> +		uuid_bin_to_str(addr, uuid, str_format);
> +	else
> +		strcpy(uuid, "(null)");

Everywhere else in vsprintf.c we use "<NULL>". I would prefer consistency.

@Alex:
You already added the patch to efi-next. Could you update the string,
please?

Best regards

Heinrich

>  
>  	return string(buf, end, uuid, field_width, precision, flags);
>  }
> 

[U-Boot] [PATCH v8 16/30] efi_selftest: Clean up a few comments and messages

[Simon Glass]

Fix the 'amp' typo, expand on what 'steps' is and fix a few other minor
things.

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

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

 lib/efi_selftest/efi_selftest.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
index 13eb2cd604..dd338db687 100644
--- a/lib/efi_selftest/efi_selftest.c
+++ b/lib/efi_selftest/efi_selftest.c
@@ -8,9 +8,7 @@
 #include <efi_selftest.h>
 #include <vsprintf.h>
 
-/*
- * Constants for test step bitmap
- */
+/* Constants for test step bitmap */
 #define EFI_ST_SETUP	1
 #define EFI_ST_EXECUTE	2
 #define EFI_ST_TEARDOWN	4
@@ -26,7 +24,7 @@ static u16 reset_message[] = L"Selftest completed";
  *
  * The size of the memory map is determined.
  * Pool memory is allocated to copy the memory map.
- * The memory amp is copied and the map key is obtained.
+ * The memory map is copied and the map key is obtained.
  * The map key is used to exit the boot services.
  */
 void efi_st_exit_boot_services(void)
@@ -146,7 +144,7 @@ static int teardown(struct efi_unit_test *test, unsigned int *failures)
  * Check that a test exists.
  *
  * @testname:	name of the test
- * @return:	test
+ * @return:	test, or NULL if not found
  */
 static struct efi_unit_test *find_test(const u16 *testname)
 {
@@ -182,7 +180,7 @@ static void list_all_tests(void)
  *
  * @testname	name of a single selected test or NULL
  * @phase	test phase
- * @steps	steps to execute
+ * @steps	steps to execute (mask with bits from EFI_ST_...)
  * failures	returns EFI_ST_SUCCESS if all test steps succeeded
  */
 void efi_st_do_tests(const u16 *testname, unsigned int phase,
@@ -296,12 +294,12 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 	efi_st_printc(EFI_WHITE, "\nSummary: %u failures\n\n", failures);
 
 	/* Reset system */
-	efi_st_printf("Preparing for reset. Press any key.\n");
+	efi_st_printf("Preparing for reset. Press any key...\n");
 	efi_st_get_key();
 	runtime->reset_system(EFI_RESET_WARM, EFI_NOT_READY,
 			      sizeof(reset_message), reset_message);
 	efi_st_printf("\n");
-	efi_st_error("Reset failed.\n");
+	efi_st_error("Reset failed\n");
 
 	return EFI_UNSUPPORTED;
 }
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [U-Boot, v8, 16/30] efi_selftest: Clean up a few comments and messages

[Alexander Graf]

> Fix the 'amp' typo, expand on what 'steps' is and fix a few other minor
> things.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

At present map_sysmem() maps an address into the sandbox RAM buffer,
return a pointer, while map_to_sysmem() goes the other way.

The mapping is currently just 1:1 since a case was not found where a more
flexible mapping was needed. PCI does have a separate and more complex
mapping, but uses its own mechanism.

However this arrange cannot handle one important case, which is where a
test declares a stack variable and passes a pointer to it into a U-Boot
function which uses map_to_sysmem() to turn it into a address. Since the
pointer is not inside emulated DRAM, this will fail.

Add a mapping feature which can handle any such pointer, mapping it to a
simple tag value which can be passed around in U-Boot as an address.

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

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

 arch/sandbox/cpu/cpu.c           | 141 +++++++++++++++++++++++++++++--
 arch/sandbox/cpu/state.c         |   8 ++
 arch/sandbox/include/asm/state.h |  21 +++++
 3 files changed, 161 insertions(+), 9 deletions(-)

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index cde0b055a6..fd77603ebf 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
 	return 0;
 }
 
+/**
+ * is_in_sandbox_mem() - Checks if a pointer is within sandbox's emulated DRAM
+ *
+ * This provides a way to check if a pointer is owned by sandbox (and is within
+ * its RAM) or not. Sometimes pointers come from a test which conceptually runs
+ * output sandbox, potentially with direct access to the C-library malloc()
+ * function, or the sandbox stack (which is not actually within the emulated
+ * DRAM.
+ *
+ * Such pointers obviously cannot be mapped into sandbox's DRAM, so we must
+ * detect them an process them separately, by recording a mapping to a tag,
+ * which we can use to map back to the pointer later.
+ *
+ * @ptr: Pointer to check
+ * @return true if this is within sandbox emulated DRAM, false if not
+ */
+static bool is_in_sandbox_mem(const void *ptr)
+{
+	return (const uint8_t *)ptr >= gd->arch.ram_buf &&
+		(const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
+}
+
+/**
+ * phys_to_virt() - Converts a sandbox RAM address to a pointer
+ *
+ * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These index into
+ * the emulated DRAM buffer used by sandbox. This function converts such an
+ * address to a pointer into thi sbuffer, which can be used to access the
+ * memory.
+ *
+ * If the address is outside this range, it is assumed to be a tag
+ */
 void *phys_to_virt(phys_addr_t paddr)
 {
-	return (void *)(gd->arch.ram_buf + paddr);
+	struct sandbox_mapmem_entry *mentry;
+	struct sandbox_state *state;
+
+	/* If the address is within emulated DRAM, calculate the value */
+	if (paddr < gd->ram_size)
+		return (void *)(gd->arch.ram_buf + paddr);
+
+	/*
+	 * Otherwise search out list of tags for the correct pointer previously
+	 * created by map_to_sysmem()
+	 */
+	state = state_get_current();
+	list_for_each_entry(mentry, &state->mapmem_head, sibling_node) {
+		if (mentry->tag == paddr) {
+			printf("%s: Used map from %lx to %p\n", __func__,
+			       (ulong)paddr, mentry->ptr);
+			return mentry->ptr;
+		}
+	}
+
+	printf("%s: Cannot map sandbox address %lx (SDRAM from 0 to %lx)\n",
+	       __func__, (ulong)paddr, (ulong)gd->ram_size);
+	os_abort();
+
+	/* Not reached */
+	return NULL;
+}
+
+struct sandbox_mapmem_entry *find_tag(const void *ptr)
+{
+	struct sandbox_mapmem_entry *mentry;
+	struct sandbox_state *state = state_get_current();
+
+	list_for_each_entry(mentry, &state->mapmem_head, sibling_node) {
+		if (mentry->ptr == ptr) {
+			debug("%s: Used map from %p to %lx\n", __func__, ptr,
+			      mentry->tag);
+			return mentry;
+		}
+	}
+	return NULL;
 }
 
-phys_addr_t virt_to_phys(void *vaddr)
+phys_addr_t virt_to_phys(void *ptr)
 {
-	return (phys_addr_t)((uint8_t *)vaddr - gd->arch.ram_buf);
+	struct sandbox_mapmem_entry *mentry;
+
+	/*
+	 * If it is in emulated RAM, don't bother looking for a tag. Just
+	 * calculate the pointer using the provides offset into the RAM buffer.
+	 */
+	if (is_in_sandbox_mem(ptr))
+		return (phys_addr_t)((uint8_t *)ptr - gd->arch.ram_buf);
+
+	mentry = find_tag(ptr);
+	if (!mentry) {
+		/* Abort so that gdb can be used here */
+		printf("%s: Cannot map sandbox address %p (SDRAM from 0 to %lx)\n",
+		       __func__, ptr, (ulong)gd->ram_size);
+		os_abort();
+	}
+	printf("%s: Used map from %p to %lx\n", __func__, ptr, mentry->tag);
+
+	return mentry->tag;
 }
 
 void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
@@ -87,24 +177,57 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
 	return phys_to_virt(paddr);
 }
 
-void unmap_physmem(const void *vaddr, unsigned long flags)
+void unmap_physmem(const void *ptr, unsigned long flags)
 {
 #ifdef CONFIG_PCI
 	if (map_dev) {
-		pci_unmap_physmem(vaddr, map_len, map_dev);
+		pci_unmap_physmem(ptr, map_len, map_dev);
 		map_dev = NULL;
 	}
 #endif
 }
 
-void sandbox_set_enable_pci_map(int enable)
+phys_addr_t map_to_sysmem(const void *ptr)
 {
-	enable_pci_map = enable;
+	struct sandbox_mapmem_entry *mentry;
+
+	/*
+	 * If it is in emulated RAM, don't bother creating a tag. Just return
+	 * the offset into the RAM buffer.
+	 */
+	if (is_in_sandbox_mem(ptr))
+		return (u8 *)ptr - gd->arch.ram_buf;
+
+	/*
+	 * See if there is an existing tag with this pointer. If not, set up a
+	 * new one.
+	 */
+	mentry = find_tag(ptr);
+	if (!mentry) {
+		struct sandbox_state *state = state_get_current();
+
+		mentry = malloc(sizeof(*mentry));
+		if (!mentry) {
+			printf("%s: Error: Out of memory\n", __func__);
+			os_exit(ENOMEM);
+		}
+		mentry->tag = state->next_tag++;
+		mentry->ptr = (void *)ptr;
+		list_add_tail(&mentry->sibling_node, &state->mapmem_head);
+		debug("%s: Added map from %p to %lx\n", __func__, ptr,
+		      (ulong)mentry->tag);
+	}
+
+	/*
+	 * Return the tag as the address to use. A later call to map_sysmem()
+	 * will return ptr
+	 */
+	return mentry->tag;
 }
 
-phys_addr_t map_to_sysmem(const void *ptr)
+void sandbox_set_enable_pci_map(int enable)
 {
-	return (u8 *)ptr - gd->arch.ram_buf;
+	enable_pci_map = enable;
 }
 
 void flush_dcache_range(unsigned long start, unsigned long stop)
diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
index cc50819ab9..04a11fed55 100644
--- a/arch/sandbox/cpu/state.c
+++ b/arch/sandbox/cpu/state.c
@@ -359,6 +359,14 @@ void state_reset_for_test(struct sandbox_state *state)
 
 	memset(&state->wdt, '\0', sizeof(state->wdt));
 	memset(state->spi, '\0', sizeof(state->spi));
+
+	/*
+	 * Set up the memory tag list. Use the top of emulated SDRAM for the
+	 * first tag number, since that address offset is outside the legal
+	 * range, and can be assumed to be a tag.
+	 */
+	INIT_LIST_HEAD(&state->mapmem_head);
+	state->next_tag = state->ram_size;
 }
 
 int state_init(void)
diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h
index 7ed4b512d2..a612ce8944 100644
--- a/arch/sandbox/include/asm/state.h
+++ b/arch/sandbox/include/asm/state.h
@@ -9,6 +9,7 @@
 #include <config.h>
 #include <sysreset.h>
 #include <stdbool.h>
+#include <linux/list.h>
 #include <linux/stringify.h>
 
 /**
@@ -45,6 +46,23 @@ struct sandbox_wdt_info {
 	bool running;
 };
 
+/**
+ * struct sandbox_mapmem_entry - maps pointers to/from U-Boot addresses
+ *
+ * When map_to_sysmem() is called with an address outside sandbox's emulated
+ * RAM, a record is created with a tag that can be used to reference that
+ * pointer. When map_sysmem() is called later with that tag, the pointer will
+ * be returned, just as it would for a normal sandbox address.
+ *
+ * @tag: Address tag (a value which U-Boot uses to refer to the address)
+ * @ptr: Associated pointer for that tag
+ */
+struct sandbox_mapmem_entry {
+	ulong tag;
+	void *ptr;
+	struct list_head sibling_node;
+};
+
 /* The complete state of the test system */
 struct sandbox_state {
 	const char *cmd;		/* Command to execute */
@@ -78,6 +96,9 @@ struct sandbox_state {
 
 	/* Information about Watchdog */
 	struct sandbox_wdt_info wdt;
+
+	ulong next_tag;			/* Next address tag to allocate */
+	struct list_head mapmem_head;	/* struct sandbox_mapmem_entry */
 };
 
 /* Minimum space we guarantee in the state FDT when calling read/write*/
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> At present map_sysmem() maps an address into the sandbox RAM buffer,
> return a pointer, while map_to_sysmem() goes the other way.
>
> The mapping is currently just 1:1 since a case was not found where a more
> flexible mapping was needed. PCI does have a separate and more complex
> mapping, but uses its own mechanism.
>
> However this arrange cannot handle one important case, which is where a
> test declares a stack variable and passes a pointer to it into a U-Boot
> function which uses map_to_sysmem() to turn it into a address. Since the
> pointer is not inside emulated DRAM, this will fail.
>
> Add a mapping feature which can handle any such pointer, mapping it to a
> simple tag value which can be passed around in U-Boot as an address.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   arch/sandbox/cpu/cpu.c           | 141 +++++++++++++++++++++++++++++--
>   arch/sandbox/cpu/state.c         |   8 ++
>   arch/sandbox/include/asm/state.h |  21 +++++
>   3 files changed, 161 insertions(+), 9 deletions(-)
>
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index cde0b055a6..fd77603ebf 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>   	return 0;
>   }
>   
> +/**
> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's emulated DRAM
> + *
> + * This provides a way to check if a pointer is owned by sandbox (and is within
> + * its RAM) or not. Sometimes pointers come from a test which conceptually runs
> + * output sandbox, potentially with direct access to the C-library malloc()
> + * function, or the sandbox stack (which is not actually within the emulated
> + * DRAM.
> + *
> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we must
> + * detect them an process them separately, by recording a mapping to a tag,
> + * which we can use to map back to the pointer later.
> + *
> + * @ptr: Pointer to check
> + * @return true if this is within sandbox emulated DRAM, false if not
> + */
> +static bool is_in_sandbox_mem(const void *ptr)
> +{
> +	return (const uint8_t *)ptr >= gd->arch.ram_buf &&
> +		(const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;

Greater/Less comparisons on pointers are invalid (albeit used) C IIRC. 
You should cast to uintptr_t instead and compare those.

> +}
> +
> +/**
> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
> + *
> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These index into
> + * the emulated DRAM buffer used by sandbox. This function converts such an
> + * address to a pointer into thi sbuffer, which can be used to access the

this

...

Thinking about this, wouldn't it be easier to just allocate the stack 
inside U-Boot RAM?


Alex

> + * memory.
> + *
> + * If the address is outside this range, it is assumed to be a tag
> + */
>   void *phys_to_virt(phys_addr_t paddr)
>   {
> -	return (void *)(gd->arch.ram_buf + paddr);
> +	struct sandbox_mapmem_entry *mentry;
> +	struct sandbox_state *state;
> +
> +	/* If the address is within emulated DRAM, calculate the value */
> +	if (paddr < gd->ram_size)
> +		return (void *)(gd->arch.ram_buf + paddr);
> +
> +	/*
> +	 * Otherwise search out list of tags for the correct pointer previously
> +	 * created by map_to_sysmem()
> +	 */
> +	state = state_get_current();
> +	list_for_each_entry(mentry, &state->mapmem_head, sibling_node) {
> +		if (mentry->tag == paddr) {
> +			printf("%s: Used map from %lx to %p\n", __func__,
> +			       (ulong)paddr, mentry->ptr);
> +			return mentry->ptr;
> +		}
> +	}
> +
> +	printf("%s: Cannot map sandbox address %lx (SDRAM from 0 to %lx)\n",
> +	       __func__, (ulong)paddr, (ulong)gd->ram_size);
> +	os_abort();
> +
> +	/* Not reached */
> +	return NULL;
> +}
> +
> +struct sandbox_mapmem_entry *find_tag(const void *ptr)
> +{
> +	struct sandbox_mapmem_entry *mentry;
> +	struct sandbox_state *state = state_get_current();
> +
> +	list_for_each_entry(mentry, &state->mapmem_head, sibling_node) {
> +		if (mentry->ptr == ptr) {
> +			debug("%s: Used map from %p to %lx\n", __func__, ptr,
> +			      mentry->tag);
> +			return mentry;
> +		}
> +	}
> +	return NULL;
>   }
>   
> -phys_addr_t virt_to_phys(void *vaddr)
> +phys_addr_t virt_to_phys(void *ptr)
>   {
> -	return (phys_addr_t)((uint8_t *)vaddr - gd->arch.ram_buf);
> +	struct sandbox_mapmem_entry *mentry;
> +
> +	/*
> +	 * If it is in emulated RAM, don't bother looking for a tag. Just
> +	 * calculate the pointer using the provides offset into the RAM buffer.
> +	 */
> +	if (is_in_sandbox_mem(ptr))
> +		return (phys_addr_t)((uint8_t *)ptr - gd->arch.ram_buf);
> +
> +	mentry = find_tag(ptr);
> +	if (!mentry) {
> +		/* Abort so that gdb can be used here */
> +		printf("%s: Cannot map sandbox address %p (SDRAM from 0 to %lx)\n",
> +		       __func__, ptr, (ulong)gd->ram_size);
> +		os_abort();
> +	}
> +	printf("%s: Used map from %p to %lx\n", __func__, ptr, mentry->tag);
> +
> +	return mentry->tag;
>   }
>   
>   void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
> @@ -87,24 +177,57 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
>   	return phys_to_virt(paddr);
>   }
>   
> -void unmap_physmem(const void *vaddr, unsigned long flags)
> +void unmap_physmem(const void *ptr, unsigned long flags)
>   {
>   #ifdef CONFIG_PCI
>   	if (map_dev) {
> -		pci_unmap_physmem(vaddr, map_len, map_dev);
> +		pci_unmap_physmem(ptr, map_len, map_dev);
>   		map_dev = NULL;
>   	}
>   #endif
>   }
>   
> -void sandbox_set_enable_pci_map(int enable)
> +phys_addr_t map_to_sysmem(const void *ptr)
>   {
> -	enable_pci_map = enable;
> +	struct sandbox_mapmem_entry *mentry;
> +
> +	/*
> +	 * If it is in emulated RAM, don't bother creating a tag. Just return
> +	 * the offset into the RAM buffer.
> +	 */
> +	if (is_in_sandbox_mem(ptr))
> +		return (u8 *)ptr - gd->arch.ram_buf;
> +
> +	/*
> +	 * See if there is an existing tag with this pointer. If not, set up a
> +	 * new one.
> +	 */
> +	mentry = find_tag(ptr);
> +	if (!mentry) {
> +		struct sandbox_state *state = state_get_current();
> +
> +		mentry = malloc(sizeof(*mentry));
> +		if (!mentry) {
> +			printf("%s: Error: Out of memory\n", __func__);
> +			os_exit(ENOMEM);
> +		}
> +		mentry->tag = state->next_tag++;
> +		mentry->ptr = (void *)ptr;
> +		list_add_tail(&mentry->sibling_node, &state->mapmem_head);
> +		debug("%s: Added map from %p to %lx\n", __func__, ptr,
> +		      (ulong)mentry->tag);
> +	}
> +
> +	/*
> +	 * Return the tag as the address to use. A later call to map_sysmem()
> +	 * will return ptr
> +	 */
> +	return mentry->tag;
>   }
>   
> -phys_addr_t map_to_sysmem(const void *ptr)
> +void sandbox_set_enable_pci_map(int enable)
>   {
> -	return (u8 *)ptr - gd->arch.ram_buf;
> +	enable_pci_map = enable;
>   }
>   
>   void flush_dcache_range(unsigned long start, unsigned long stop)
> diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
> index cc50819ab9..04a11fed55 100644
> --- a/arch/sandbox/cpu/state.c
> +++ b/arch/sandbox/cpu/state.c
> @@ -359,6 +359,14 @@ void state_reset_for_test(struct sandbox_state *state)
>   
>   	memset(&state->wdt, '\0', sizeof(state->wdt));
>   	memset(state->spi, '\0', sizeof(state->spi));
> +
> +	/*
> +	 * Set up the memory tag list. Use the top of emulated SDRAM for the
> +	 * first tag number, since that address offset is outside the legal
> +	 * range, and can be assumed to be a tag.
> +	 */
> +	INIT_LIST_HEAD(&state->mapmem_head);
> +	state->next_tag = state->ram_size;
>   }
>   
>   int state_init(void)
> diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h
> index 7ed4b512d2..a612ce8944 100644
> --- a/arch/sandbox/include/asm/state.h
> +++ b/arch/sandbox/include/asm/state.h
> @@ -9,6 +9,7 @@
>   #include <config.h>
>   #include <sysreset.h>
>   #include <stdbool.h>
> +#include <linux/list.h>
>   #include <linux/stringify.h>
>   
>   /**
> @@ -45,6 +46,23 @@ struct sandbox_wdt_info {
>   	bool running;
>   };
>   
> +/**
> + * struct sandbox_mapmem_entry - maps pointers to/from U-Boot addresses
> + *
> + * When map_to_sysmem() is called with an address outside sandbox's emulated
> + * RAM, a record is created with a tag that can be used to reference that
> + * pointer. When map_sysmem() is called later with that tag, the pointer will
> + * be returned, just as it would for a normal sandbox address.
> + *
> + * @tag: Address tag (a value which U-Boot uses to refer to the address)
> + * @ptr: Associated pointer for that tag
> + */
> +struct sandbox_mapmem_entry {
> +	ulong tag;
> +	void *ptr;
> +	struct list_head sibling_node;
> +};
> +
>   /* The complete state of the test system */
>   struct sandbox_state {
>   	const char *cmd;		/* Command to execute */
> @@ -78,6 +96,9 @@ struct sandbox_state {
>   
>   	/* Information about Watchdog */
>   	struct sandbox_wdt_info wdt;
> +
> +	ulong next_tag;			/* Next address tag to allocate */
> +	struct list_head mapmem_head;	/* struct sandbox_mapmem_entry */
>   };
>   
>   /* Minimum space we guarantee in the state FDT when calling read/write*/

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

Hi Alex,

On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>> return a pointer, while map_to_sysmem() goes the other way.
>>
>> The mapping is currently just 1:1 since a case was not found where a more
>> flexible mapping was needed. PCI does have a separate and more complex
>> mapping, but uses its own mechanism.
>>
>> However this arrange cannot handle one important case, which is where a
>> test declares a stack variable and passes a pointer to it into a U-Boot
>> function which uses map_to_sysmem() to turn it into a address. Since the
>> pointer is not inside emulated DRAM, this will fail.
>>
>> Add a mapping feature which can handle any such pointer, mapping it to a
>> simple tag value which can be passed around in U-Boot as an address.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   arch/sandbox/cpu/cpu.c           | 141 +++++++++++++++++++++++++++++--
>>   arch/sandbox/cpu/state.c         |   8 ++
>>   arch/sandbox/include/asm/state.h |  21 +++++
>>   3 files changed, 161 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>> index cde0b055a6..fd77603ebf 100644
>> --- a/arch/sandbox/cpu/cpu.c
>> +++ b/arch/sandbox/cpu/cpu.c
>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>         return 0;
>>   }
>>   +/**
>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's emulated
>> DRAM
>> + *
>> + * This provides a way to check if a pointer is owned by sandbox (and is
>> within
>> + * its RAM) or not. Sometimes pointers come from a test which
>> conceptually runs
>> + * output sandbox, potentially with direct access to the C-library
>> malloc()
>> + * function, or the sandbox stack (which is not actually within the
>> emulated
>> + * DRAM.
>> + *
>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>> must
>> + * detect them an process them separately, by recording a mapping to a
>> tag,
>> + * which we can use to map back to the pointer later.
>> + *
>> + * @ptr: Pointer to check
>> + * @return true if this is within sandbox emulated DRAM, false if not
>> + */
>> +static bool is_in_sandbox_mem(const void *ptr)
>> +{
>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>
>
> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC. You
> should cast to uintptr_t instead and compare those.

Reference? That is news to me :-)

>
>> +}
>> +
>> +/**
>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>> + *
>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These index
>> into
>> + * the emulated DRAM buffer used by sandbox. This function converts such
>> an
>> + * address to a pointer into thi sbuffer, which can be used to access the
>
>
> this

OK fixed, ta.

>
> ...
>
> Thinking about this, wouldn't it be easier to just allocate the stack inside
> U-Boot RAM?

We could change the tests so that instead of using local variables
they call a function to allocate sandbox RAM. But I'm not sure it is
easier.

This implementation of sandbox mapping is what I envisaged ages ago
when writing the original implementation. I just never got around to
it since for most cases the simple code works. For PCI I added a
hack...

So I think it is a good time to add it.

[..]

Regards,
Simon

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Alexander Graf]

On 06/21/2018 04:01 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>> return a pointer, while map_to_sysmem() goes the other way.
>>>
>>> The mapping is currently just 1:1 since a case was not found where a more
>>> flexible mapping was needed. PCI does have a separate and more complex
>>> mapping, but uses its own mechanism.
>>>
>>> However this arrange cannot handle one important case, which is where a
>>> test declares a stack variable and passes a pointer to it into a U-Boot
>>> function which uses map_to_sysmem() to turn it into a address. Since the
>>> pointer is not inside emulated DRAM, this will fail.
>>>
>>> Add a mapping feature which can handle any such pointer, mapping it to a
>>> simple tag value which can be passed around in U-Boot as an address.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v8: None
>>> Changes in v7: None
>>> Changes in v6: None
>>> Changes in v5: None
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>    arch/sandbox/cpu/cpu.c           | 141 +++++++++++++++++++++++++++++--
>>>    arch/sandbox/cpu/state.c         |   8 ++
>>>    arch/sandbox/include/asm/state.h |  21 +++++
>>>    3 files changed, 161 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>> index cde0b055a6..fd77603ebf 100644
>>> --- a/arch/sandbox/cpu/cpu.c
>>> +++ b/arch/sandbox/cpu/cpu.c
>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>          return 0;
>>>    }
>>>    +/**
>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's emulated
>>> DRAM
>>> + *
>>> + * This provides a way to check if a pointer is owned by sandbox (and is
>>> within
>>> + * its RAM) or not. Sometimes pointers come from a test which
>>> conceptually runs
>>> + * output sandbox, potentially with direct access to the C-library
>>> malloc()
>>> + * function, or the sandbox stack (which is not actually within the
>>> emulated
>>> + * DRAM.
>>> + *
>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>>> must
>>> + * detect them an process them separately, by recording a mapping to a
>>> tag,
>>> + * which we can use to map back to the pointer later.
>>> + *
>>> + * @ptr: Pointer to check
>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>> + */
>>> +static bool is_in_sandbox_mem(const void *ptr)
>>> +{
>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>>
>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC. You
>> should cast to uintptr_t instead and compare those.
> Reference? That is news to me :-)
>
>>> +}
>>> +
>>> +/**
>>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>>> + *
>>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These index
>>> into
>>> + * the emulated DRAM buffer used by sandbox. This function converts such
>>> an
>>> + * address to a pointer into thi sbuffer, which can be used to access the
>>
>> this
> OK fixed, ta.
>
>> ...
>>
>> Thinking about this, wouldn't it be easier to just allocate the stack inside
>> U-Boot RAM?
> We could change the tests so that instead of using local variables
> they call a function to allocate sandbox RAM. But I'm not sure it is
> easier.

No, what I'm saying is why don't we just in early bootup code move %sp 
to an address that resides inside the U-Boot address space? Then things 
would behave just like on real hardware.


Alex

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

Hi Alex,

On 21 June 2018 at 04:00, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>>> return a pointer, while map_to_sysmem() goes the other way.
>>>>
>>>> The mapping is currently just 1:1 since a case was not found where a
>>>> more
>>>> flexible mapping was needed. PCI does have a separate and more complex
>>>> mapping, but uses its own mechanism.
>>>>
>>>> However this arrange cannot handle one important case, which is where a
>>>> test declares a stack variable and passes a pointer to it into a U-Boot
>>>> function which uses map_to_sysmem() to turn it into a address. Since the
>>>> pointer is not inside emulated DRAM, this will fail.
>>>>
>>>> Add a mapping feature which can handle any such pointer, mapping it to a
>>>> simple tag value which can be passed around in U-Boot as an address.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v8: None
>>>> Changes in v7: None
>>>> Changes in v6: None
>>>> Changes in v5: None
>>>> Changes in v4: None
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>    arch/sandbox/cpu/cpu.c           | 141
>>>> +++++++++++++++++++++++++++++--
>>>>    arch/sandbox/cpu/state.c         |   8 ++
>>>>    arch/sandbox/include/asm/state.h |  21 +++++
>>>>    3 files changed, 161 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>>> index cde0b055a6..fd77603ebf 100644
>>>> --- a/arch/sandbox/cpu/cpu.c
>>>> +++ b/arch/sandbox/cpu/cpu.c
>>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>>          return 0;
>>>>    }
>>>>    +/**
>>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's
>>>> emulated
>>>> DRAM
>>>> + *
>>>> + * This provides a way to check if a pointer is owned by sandbox (and
>>>> is
>>>> within
>>>> + * its RAM) or not. Sometimes pointers come from a test which
>>>> conceptually runs
>>>> + * output sandbox, potentially with direct access to the C-library
>>>> malloc()
>>>> + * function, or the sandbox stack (which is not actually within the
>>>> emulated
>>>> + * DRAM.
>>>> + *
>>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>>>> must
>>>> + * detect them an process them separately, by recording a mapping to a
>>>> tag,
>>>> + * which we can use to map back to the pointer later.
>>>> + *
>>>> + * @ptr: Pointer to check
>>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>>> + */
>>>> +static bool is_in_sandbox_mem(const void *ptr)
>>>> +{
>>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>>>
>>>
>>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC.
>>> You
>>> should cast to uintptr_t instead and compare those.
>>
>> Reference? That is news to me :-)
>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>>>> + *
>>>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These
>>>> index
>>>> into
>>>> + * the emulated DRAM buffer used by sandbox. This function converts
>>>> such
>>>> an
>>>> + * address to a pointer into thi sbuffer, which can be used to access
>>>> the
>>>
>>>
>>> this
>>
>> OK fixed, ta.
>>
>>> ...
>>>
>>> Thinking about this, wouldn't it be easier to just allocate the stack
>>> inside
>>> U-Boot RAM?
>>
>> We could change the tests so that instead of using local variables
>> they call a function to allocate sandbox RAM. But I'm not sure it is
>> easier.
>
>
> No, what I'm saying is why don't we just in early bootup code move %sp to an
> address that resides inside the U-Boot address space? Then things would
> behave just like on real hardware.

I was hoping that wasn't what you were saying!

I worry about messing with the host system in that way. It seems
really gross. Is it safe?

Regards,
Simon

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 04:00, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>>>> return a pointer, while map_to_sysmem() goes the other way.
>>>>>
>>>>> The mapping is currently just 1:1 since a case was not found where a
>>>>> more
>>>>> flexible mapping was needed. PCI does have a separate and more complex
>>>>> mapping, but uses its own mechanism.
>>>>>
>>>>> However this arrange cannot handle one important case, which is where a
>>>>> test declares a stack variable and passes a pointer to it into a U-Boot
>>>>> function which uses map_to_sysmem() to turn it into a address. Since the
>>>>> pointer is not inside emulated DRAM, this will fail.
>>>>>
>>>>> Add a mapping feature which can handle any such pointer, mapping it to a
>>>>> simple tag value which can be passed around in U-Boot as an address.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v8: None
>>>>> Changes in v7: None
>>>>> Changes in v6: None
>>>>> Changes in v5: None
>>>>> Changes in v4: None
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>>
>>>>>     arch/sandbox/cpu/cpu.c           | 141
>>>>> +++++++++++++++++++++++++++++--
>>>>>     arch/sandbox/cpu/state.c         |   8 ++
>>>>>     arch/sandbox/include/asm/state.h |  21 +++++
>>>>>     3 files changed, 161 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>>>> index cde0b055a6..fd77603ebf 100644
>>>>> --- a/arch/sandbox/cpu/cpu.c
>>>>> +++ b/arch/sandbox/cpu/cpu.c
>>>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>>>           return 0;
>>>>>     }
>>>>>     +/**
>>>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's
>>>>> emulated
>>>>> DRAM
>>>>> + *
>>>>> + * This provides a way to check if a pointer is owned by sandbox (and
>>>>> is
>>>>> within
>>>>> + * its RAM) or not. Sometimes pointers come from a test which
>>>>> conceptually runs
>>>>> + * output sandbox, potentially with direct access to the C-library
>>>>> malloc()
>>>>> + * function, or the sandbox stack (which is not actually within the
>>>>> emulated
>>>>> + * DRAM.
>>>>> + *
>>>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>>>>> must
>>>>> + * detect them an process them separately, by recording a mapping to a
>>>>> tag,
>>>>> + * which we can use to map back to the pointer later.
>>>>> + *
>>>>> + * @ptr: Pointer to check
>>>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>>>> + */
>>>>> +static bool is_in_sandbox_mem(const void *ptr)
>>>>> +{
>>>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>>>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>>>>
>>>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC.
>>>> You
>>>> should cast to uintptr_t instead and compare those.
>>> Reference? That is news to me :-)
>>>
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>>>>> + *
>>>>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These
>>>>> index
>>>>> into
>>>>> + * the emulated DRAM buffer used by sandbox. This function converts
>>>>> such
>>>>> an
>>>>> + * address to a pointer into thi sbuffer, which can be used to access
>>>>> the
>>>>
>>>> this
>>> OK fixed, ta.
>>>
>>>> ...
>>>>
>>>> Thinking about this, wouldn't it be easier to just allocate the stack
>>>> inside
>>>> U-Boot RAM?
>>> We could change the tests so that instead of using local variables
>>> they call a function to allocate sandbox RAM. But I'm not sure it is
>>> easier.
>>
>> No, what I'm saying is why don't we just in early bootup code move %sp to an
>> address that resides inside the U-Boot address space? Then things would
>> behave just like on real hardware.
> I was hoping that wasn't what you were saying!
>
> I worry about messing with the host system in that way. It seems
> really gross. Is it safe?

I think it's safe - coroutines do it all the time - but it would push us 
into writing architecture specific code for sandbox.

Either way, I don't particularly care whether you want to bloat up the 
mapping logic or swap stack pointers. This is sandbox only realm and you 
can do whatever you wish there ;)


Alex

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:12, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 04:00, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>
>>>>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>>>>> return a pointer, while map_to_sysmem() goes the other way.
>>>>>>
>>>>>> The mapping is currently just 1:1 since a case was not found where a
>>>>>> more
>>>>>> flexible mapping was needed. PCI does have a separate and more complex
>>>>>> mapping, but uses its own mechanism.
>>>>>>
>>>>>> However this arrange cannot handle one important case, which is where
>>>>>> a
>>>>>> test declares a stack variable and passes a pointer to it into a
>>>>>> U-Boot
>>>>>> function which uses map_to_sysmem() to turn it into a address. Since
>>>>>> the
>>>>>> pointer is not inside emulated DRAM, this will fail.
>>>>>>
>>>>>> Add a mapping feature which can handle any such pointer, mapping it to
>>>>>> a
>>>>>> simple tag value which can be passed around in U-Boot as an address.
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v8: None
>>>>>> Changes in v7: None
>>>>>> Changes in v6: None
>>>>>> Changes in v5: None
>>>>>> Changes in v4: None
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
>>>>>>     arch/sandbox/cpu/cpu.c           | 141
>>>>>> +++++++++++++++++++++++++++++--
>>>>>>     arch/sandbox/cpu/state.c         |   8 ++
>>>>>>     arch/sandbox/include/asm/state.h |  21 +++++
>>>>>>     3 files changed, 161 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>>>>> index cde0b055a6..fd77603ebf 100644
>>>>>> --- a/arch/sandbox/cpu/cpu.c
>>>>>> +++ b/arch/sandbox/cpu/cpu.c
>>>>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>>>>           return 0;
>>>>>>     }
>>>>>>     +/**
>>>>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's
>>>>>> emulated
>>>>>> DRAM
>>>>>> + *
>>>>>> + * This provides a way to check if a pointer is owned by sandbox (and
>>>>>> is
>>>>>> within
>>>>>> + * its RAM) or not. Sometimes pointers come from a test which
>>>>>> conceptually runs
>>>>>> + * output sandbox, potentially with direct access to the C-library
>>>>>> malloc()
>>>>>> + * function, or the sandbox stack (which is not actually within the
>>>>>> emulated
>>>>>> + * DRAM.
>>>>>> + *
>>>>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so
>>>>>> we
>>>>>> must
>>>>>> + * detect them an process them separately, by recording a mapping to
>>>>>> a
>>>>>> tag,
>>>>>> + * which we can use to map back to the pointer later.
>>>>>> + *
>>>>>> + * @ptr: Pointer to check
>>>>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>>>>> + */
>>>>>> +static bool is_in_sandbox_mem(const void *ptr)
>>>>>> +{
>>>>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>>>>> +               (const uint8_t *)ptr < gd->arch.ram_buf +
>>>>>> gd->ram_size;
>>>>>
>>>>>
>>>>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC.
>>>>> You
>>>>> should cast to uintptr_t instead and compare those.
>>>>
>>>> Reference? That is news to me :-)
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>>>>>> + *
>>>>>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These
>>>>>> index
>>>>>> into
>>>>>> + * the emulated DRAM buffer used by sandbox. This function converts
>>>>>> such
>>>>>> an
>>>>>> + * address to a pointer into thi sbuffer, which can be used to access
>>>>>> the
>>>>>
>>>>>
>>>>> this
>>>>
>>>> OK fixed, ta.
>>>>
>>>>> ...
>>>>>
>>>>> Thinking about this, wouldn't it be easier to just allocate the stack
>>>>> inside
>>>>> U-Boot RAM?
>>>>
>>>> We could change the tests so that instead of using local variables
>>>> they call a function to allocate sandbox RAM. But I'm not sure it is
>>>> easier.
>>>
>>>
>>> No, what I'm saying is why don't we just in early bootup code move %sp to
>>> an
>>> address that resides inside the U-Boot address space? Then things would
>>> behave just like on real hardware.
>>
>> I was hoping that wasn't what you were saying!
>>
>> I worry about messing with the host system in that way. It seems
>> really gross. Is it safe?
>
>
> I think it's safe - coroutines do it all the time - but it would push us
> into writing architecture specific code for sandbox.

OK, well perhaps something to think about in the future.

>
> Either way, I don't particularly care whether you want to bloat up the
> mapping logic or swap stack pointers. This is sandbox only realm and you can
> do whatever you wish there ;)

To some extent, yes. But if we want to prevent 'foreign' pointers
coming into sandbox, we would need to handle bss, stack and the
rodata/data. I'm sure it could work, but it seems like it would make
sandbox into a rather strange program. I had trouble even with the
minor linker-script changes I have already done, on some
architectures.

Regards,
Simon

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Alexander Graf]

On 06/21/2018 04:01 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>> return a pointer, while map_to_sysmem() goes the other way.
>>>
>>> The mapping is currently just 1:1 since a case was not found where a more
>>> flexible mapping was needed. PCI does have a separate and more complex
>>> mapping, but uses its own mechanism.
>>>
>>> However this arrange cannot handle one important case, which is where a
>>> test declares a stack variable and passes a pointer to it into a U-Boot
>>> function which uses map_to_sysmem() to turn it into a address. Since the
>>> pointer is not inside emulated DRAM, this will fail.
>>>
>>> Add a mapping feature which can handle any such pointer, mapping it to a
>>> simple tag value which can be passed around in U-Boot as an address.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v8: None
>>> Changes in v7: None
>>> Changes in v6: None
>>> Changes in v5: None
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>    arch/sandbox/cpu/cpu.c           | 141 +++++++++++++++++++++++++++++--
>>>    arch/sandbox/cpu/state.c         |   8 ++
>>>    arch/sandbox/include/asm/state.h |  21 +++++
>>>    3 files changed, 161 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>> index cde0b055a6..fd77603ebf 100644
>>> --- a/arch/sandbox/cpu/cpu.c
>>> +++ b/arch/sandbox/cpu/cpu.c
>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>          return 0;
>>>    }
>>>    +/**
>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's emulated
>>> DRAM
>>> + *
>>> + * This provides a way to check if a pointer is owned by sandbox (and is
>>> within
>>> + * its RAM) or not. Sometimes pointers come from a test which
>>> conceptually runs
>>> + * output sandbox, potentially with direct access to the C-library
>>> malloc()
>>> + * function, or the sandbox stack (which is not actually within the
>>> emulated
>>> + * DRAM.
>>> + *
>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>>> must
>>> + * detect them an process them separately, by recording a mapping to a
>>> tag,
>>> + * which we can use to map back to the pointer later.
>>> + *
>>> + * @ptr: Pointer to check
>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>> + */
>>> +static bool is_in_sandbox_mem(const void *ptr)
>>> +{
>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>>
>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC. You
>> should cast to uintptr_t instead and compare those.
> Reference? That is news to me :-)

https://port70.net/~nsz/c/c11/n1570.html#6.5.8

Point 5. Since we're not in an array or a struct member, we fall into 
the "In all other cases, the behavior is undefined" case. Our compiler 
people basically recommend to convert to uintptr_t first and compare 
then. That way behavior is always defined.


Alex

[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

Hi Alex,

On 21 June 2018 at 04:10, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 08:50, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>>> return a pointer, while map_to_sysmem() goes the other way.
>>>>
>>>> The mapping is currently just 1:1 since a case was not found where a
>>>> more
>>>> flexible mapping was needed. PCI does have a separate and more complex
>>>> mapping, but uses its own mechanism.
>>>>
>>>> However this arrange cannot handle one important case, which is where a
>>>> test declares a stack variable and passes a pointer to it into a U-Boot
>>>> function which uses map_to_sysmem() to turn it into a address. Since the
>>>> pointer is not inside emulated DRAM, this will fail.
>>>>
>>>> Add a mapping feature which can handle any such pointer, mapping it to a
>>>> simple tag value which can be passed around in U-Boot as an address.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v8: None
>>>> Changes in v7: None
>>>> Changes in v6: None
>>>> Changes in v5: None
>>>> Changes in v4: None
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>    arch/sandbox/cpu/cpu.c           | 141
>>>> +++++++++++++++++++++++++++++--
>>>>    arch/sandbox/cpu/state.c         |   8 ++
>>>>    arch/sandbox/include/asm/state.h |  21 +++++
>>>>    3 files changed, 161 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>>> index cde0b055a6..fd77603ebf 100644
>>>> --- a/arch/sandbox/cpu/cpu.c
>>>> +++ b/arch/sandbox/cpu/cpu.c
>>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>>          return 0;
>>>>    }
>>>>    +/**
>>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's
>>>> emulated
>>>> DRAM
>>>> + *
>>>> + * This provides a way to check if a pointer is owned by sandbox (and
>>>> is
>>>> within
>>>> + * its RAM) or not. Sometimes pointers come from a test which
>>>> conceptually runs
>>>> + * output sandbox, potentially with direct access to the C-library
>>>> malloc()
>>>> + * function, or the sandbox stack (which is not actually within the
>>>> emulated
>>>> + * DRAM.
>>>> + *
>>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so we
>>>> must
>>>> + * detect them an process them separately, by recording a mapping to a
>>>> tag,
>>>> + * which we can use to map back to the pointer later.
>>>> + *
>>>> + * @ptr: Pointer to check
>>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>>> + */
>>>> +static bool is_in_sandbox_mem(const void *ptr)
>>>> +{
>>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>>> +               (const uint8_t *)ptr < gd->arch.ram_buf + gd->ram_size;
>>>
>>>
>>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC.
>>> You
>>> should cast to uintptr_t instead and compare those.
>>
>> Reference? That is news to me :-)
>
>
> https://port70.net/~nsz/c/c11/n1570.html#6.5.8
>
> Point 5. Since we're not in an array or a struct member, we fall into the
> "In all other cases, the behavior is undefined" case. Our compiler people
> basically recommend to convert to uintptr_t first and compare then. That way
> behavior is always defined.

There are two pointers being compared:

 (const uint8_t *)ptr

and

(const uint8_t *)gd->arch.ram_buf + gd->ram_size;

which is just a const uint_8 *, call it 'x' if you like.

The bahaviour is clearly defined in the spec you pointed to:

"both operands are pointers to qualified or unqualified versions of
compatible object types."

Regards,
Simon

[U-Boot] [PATCH v8 18/30] efi: Add a call to exit() along with why we can't use it

[Simon Glass]

The test should exit like any other EFI application, by calling exit()
in the boot services API. But this crashes at present on sandbox. For now,
add in the placeholder code.

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

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

 lib/efi_loader/efi_test.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_test.c b/lib/efi_loader/efi_test.c
index 4b8d49f324..5401a0f471 100644
--- a/lib/efi_loader/efi_test.c
+++ b/lib/efi_loader/efi_test.c
@@ -9,8 +9,18 @@
 int efi_test(efi_handle_t image_handle, struct efi_system_table *systable)
 {
 	struct efi_simple_text_output_protocol *con_out = systable->con_out;
+	struct efi_boot_services *boottime = systable->boottime;
+	int ret;
 
-	con_out->output_string(con_out, L"Hello, world!\n");
+	ret = con_out->output_string(con_out, L"Hello, world!\n");
+
+	/*
+	 * We should really call EFI's exit() here but this crashes at present
+	 * on sandbox due to the incorrect use of setjmp() and longjmp(). Once
+	 * we can figure out how to make that work, this can be enabled.
+	 */
+	if (0)
+		boottime->exit(image_handle, ret, 0, NULL);
 
 	return 0;
 }
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 18/30] efi: Add a call to exit() along with why we can't use it

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> The test should exit like any other EFI application, by calling exit()
> in the boot services API. But this crashes at present on sandbox. For now,
> add in the placeholder code.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

It crashes because setjmp/longjmp are broken. See my patch set.


Alex

[U-Boot] [PATCH v8 18/30] efi: Add a call to exit() along with why we can't use it

[Simon Glass]

Hi Alex,

On 18 June 2018 at 08:51, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> The test should exit like any other EFI application, by calling exit()
>> in the boot services API. But this crashes at present on sandbox. For now,
>> add in the placeholder code.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>
> It crashes because setjmp/longjmp are broken. See my patch set.

Yes that's right. But I'd like to enable this code once your patch is in.

Regards,
Simon

[U-Boot] [PATCH v8 19/30] efi: Relocate FDT to 127MB instead of 128MB

[Simon Glass]

Sandbox only has 128MB of memory so we cannot relocate the device tree up
to start at 128MB. Use 127MB instead, which should be safe.

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

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3fa4f181ba..d2458e2397 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -141,8 +141,8 @@ static void *copy_fdt(void *fdt)
 	fdt_size = ALIGN(fdt_size + 4096, EFI_PAGE_SIZE);
 	fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
 
-	/* Safe fdt location is at 128MB */
-	new_fdt_addr = fdt_ram_start + (128 * 1024 * 1024) + fdt_size;
+	/* Safe fdt location is at 127MB */
+	new_fdt_addr = fdt_ram_start + (127 * 1024 * 1024) + fdt_size;
 	if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
 			       EFI_RUNTIME_SERVICES_DATA, fdt_pages,
 			       &new_fdt_addr) != EFI_SUCCESS) {
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 19/30] efi: Relocate FDT to 127MB instead of 128MB

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> Sandbox only has 128MB of memory so we cannot relocate the device tree up
> to start at 128MB. Use 127MB instead, which should be safe.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

We should just drop into the fallback case if allocation fails, no?


Alex

> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   cmd/bootefi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3fa4f181ba..d2458e2397 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -141,8 +141,8 @@ static void *copy_fdt(void *fdt)
>   	fdt_size = ALIGN(fdt_size + 4096, EFI_PAGE_SIZE);
>   	fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
>   
> -	/* Safe fdt location is at 128MB */
> -	new_fdt_addr = fdt_ram_start + (128 * 1024 * 1024) + fdt_size;
> +	/* Safe fdt location is at 127MB */
> +	new_fdt_addr = fdt_ram_start + (127 * 1024 * 1024) + fdt_size;
>   	if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>   			       EFI_RUNTIME_SERVICES_DATA, fdt_pages,
>   			       &new_fdt_addr) != EFI_SUCCESS) {

[U-Boot] [PATCH v8 19/30] efi: Relocate FDT to 127MB instead of 128MB

[Simon Glass]

Hi Alex,

On 18 June 2018 at 08:52, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> Sandbox only has 128MB of memory so we cannot relocate the device tree up
>> to start at 128MB. Use 127MB instead, which should be safe.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>
> We should just drop into the fallback case if allocation fails, no?

I cannot see how the allocation could ever fail, now that we are
definitely in sandbox RAM. So I'm OK with doing that if you are.

Regards,
Simon

[U-Boot] [PATCH v8 19/30] efi: Relocate FDT to 127MB instead of 128MB

[Alexander Graf]

On 06/21/2018 04:01 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 08:52, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> Sandbox only has 128MB of memory so we cannot relocate the device tree up
>>> to start at 128MB. Use 127MB instead, which should be safe.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> We should just drop into the fallback case if allocation fails, no?
> I cannot see how the allocation could ever fail, now that we are
> definitely in sandbox RAM. So I'm OK with doing that if you are.

I think that's perfectly reasonable, yes :)


Alex

[U-Boot] [PATCH v8 19/30] efi: Relocate FDT to 127MB instead of 128MB

[Simon Glass]

Hi Alex,

On 21 June 2018 at 04:01, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 08:52, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> Sandbox only has 128MB of memory so we cannot relocate the device tree
>>>> up
>>>> to start at 128MB. Use 127MB instead, which should be safe.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>>
>>> We should just drop into the fallback case if allocation fails, no?
>>
>> I cannot see how the allocation could ever fail, now that we are
>> definitely in sandbox RAM. So I'm OK with doing that if you are.
>
>
> I think that's perfectly reasonable, yes :)

OK I will update the patch.

Regards,
Simon

[U-Boot] [PATCH v8 19/30] efi: Relocate FDT to 127MB instead of 128MB

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 04:01, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 08:52, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>> Sandbox only has 128MB of memory so we cannot relocate the device tree
>>>>> up
>>>>> to start at 128MB. Use 127MB instead, which should be safe.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> We should just drop into the fallback case if allocation fails, no?
>>> I cannot see how the allocation could ever fail, now that we are
>>> definitely in sandbox RAM. So I'm OK with doing that if you are.
>>
>> I think that's perfectly reasonable, yes :)
> OK I will update the patch.

No worries. I pulled in most parts that I consider ready to efi-next 
already. I'll prepare a small patch set that takes patches from your set 
and my set and gets us a working sandbox efi_loader on top.


Alex

[U-Boot] [PATCH v8 20/30] efi: Tidy up device-tree-size calculation in copy_fdt()

[Simon Glass]

This is a bit confusing at present since it adds 4KB to the pointer, then
rounds it up. It looks like a bug, but is not.

Move the 4KB addition into a separate statement and expand the comment.

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

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index d2458e2397..c89f83fb33 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -137,8 +137,12 @@ static void *copy_fdt(void *fdt)
 			fdt_ram_start = ram_start;
 	}
 
-	/* Give us at least 4kb breathing room */
-	fdt_size = ALIGN(fdt_size + 4096, EFI_PAGE_SIZE);
+	/*
+	 * Give us at least 4KB of breathing room in case the device tree needs
+	 * to be expanded later. Round up to the nearest EFI page boundary.
+	 */
+	fdt_size += 4096;
+	fdt_size = ALIGN(fdt_size + EFI_PAGE_SIZE - 1, EFI_PAGE_SIZE);
 	fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
 
 	/* Safe fdt location is at 127MB */
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [U-Boot, v8, 20/30] efi: Tidy up device-tree-size calculation in copy_fdt()

[Alexander Graf]

> This is a bit confusing at present since it adds 4KB to the pointer, then
> rounds it up. It looks like a bug, but is not.
> 
> Move the 4KB addition into a separate statement and expand the comment.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v8 21/30] efi_loader: Use map_sysmem() in bootefi command

[Simon Glass]

From: Alexander Graf <agraf@suse.de>

The bootefi command gets a few addresses as values passed in. In sandbox,
these values are in U-Boot address space, so we need to make sure we
explicitly call map_sysmem() on them to be able to access them.

Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index c89f83fb33..b62635f448 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -14,6 +14,7 @@
 #include <errno.h>
 #include <linux/libfdt.h>
 #include <linux/libfdt_env.h>
+#include <mapmem.h>
 #include <memalign.h>
 #include <asm/global_data.h>
 #include <asm-generic/sections.h>
@@ -452,7 +453,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	char *saddr;
 	unsigned long addr;
 	efi_status_t r;
-	void *fdt_addr;
+	unsigned long fdt_addr;
+	void *fdt;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -469,11 +471,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return CMD_RET_USAGE;
 
 	if (argc > 2) {
-		fdt_addr = (void *)simple_strtoul(argv[2], NULL, 16);
+		fdt_addr = simple_strtoul(argv[2], NULL, 16);
 		if (!fdt_addr && *argv[2] != '0')
 			return CMD_RET_USAGE;
 		/* Install device tree */
-		r = efi_install_fdt(fdt_addr);
+		fdt = map_sysmem(fdt_addr, 0);
+		r = efi_install_fdt(fdt);
 		if (r != EFI_SUCCESS) {
 			printf("ERROR: failed to install device tree\n");
 			return CMD_RET_FAILURE;
@@ -492,7 +495,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			addr = simple_strtoul(saddr, NULL, 16);
 		else
 			addr = CONFIG_SYS_LOAD_ADDR;
-		memcpy((char *)addr, __efi_helloworld_begin, size);
+		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
 	} else
 #endif
 	if (IS_ENABLED(CONFIG_BOOTEFI_TEST) && !strcmp(argv[1], "test")) {
@@ -538,7 +541,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	printf("## Starting EFI application at %08lx ...\n", addr);
-	r = do_bootefi_exec((void *)addr, bootefi_device_path,
+	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
 			    bootefi_image_path);
 	printf("## Application terminated, r = %lu\n",
 	       r & ~EFI_ERROR_MASK);
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Simon Glass]

At present this function takes a pointer as its argument, then passes this
to efi_allocate_pages(), which actually takes an address. It uses casts,
which are not supported on sandbox.

Also the function calculates the FDT size rounded up to the neared EFI
page size, then its caller recalculates the size and adds a bit more to
it.

This function is much better written as something that works with
addresses only, and returns both the address and the size of the relocated
FDT.

Also, copy_fdt() returns NULL on error, but really should propagate the
error from efi_allocate_pages(). To do this it needs to return an
efi_status_t, not a void *.

Update the code in this way, so that it is easier to follow, and also
supports sandbox.

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

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

 cmd/bootefi.c | 78 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index b62635f448..5ef2d8499c 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -119,17 +119,30 @@ static void set_load_options(struct efi_loaded_image *loaded_image_info,
 	loaded_image_info->load_options_size = size * 2;
 }
 
-static void *copy_fdt(void *fdt)
+/**
+ * copy_fdt() - Copy the device tree to a new location available to EFI
+ *
+ * The FDT is relocated into a suitable location within the EFI memory map.
+ * An additional 12KB is added to the space in case the device tree needs to be
+ * expanded later with fdt_open_into().
+ *
+ * @fdt_addr: On entry, address of start of FDT. On exit, address of relocated
+ *	FDT start
+ * @fdt_sizep: Returns new size of FDT, including
+ * @return new relocated address of FDT
+ */
+static efi_status_t copy_fdt(ulong *fdt_addrp, ulong *fdt_sizep)
 {
-	u64 fdt_size = fdt_totalsize(fdt);
 	unsigned long fdt_ram_start = -1L, fdt_pages;
+	efi_status_t ret = 0;
+	void *fdt, *new_fdt;
 	u64 new_fdt_addr;
-	void *new_fdt;
+	uint fdt_size;
 	int i;
 
-        for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
-                u64 ram_start = gd->bd->bi_dram[i].start;
-                u64 ram_size = gd->bd->bi_dram[i].size;
+	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
+		u64 ram_start = gd->bd->bi_dram[i].start;
+		u64 ram_size = gd->bd->bi_dram[i].size;
 
 		if (!ram_size)
 			continue;
@@ -142,30 +155,37 @@ static void *copy_fdt(void *fdt)
 	 * Give us at least 4KB of breathing room in case the device tree needs
 	 * to be expanded later. Round up to the nearest EFI page boundary.
 	 */
-	fdt_size += 4096;
+	fdt = map_sysmem(*fdt_addrp, 0);
+	fdt_size = fdt_totalsize(fdt);
+	fdt_size += 4096 * 3;
 	fdt_size = ALIGN(fdt_size + EFI_PAGE_SIZE - 1, EFI_PAGE_SIZE);
 	fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
 
 	/* Safe fdt location is at 127MB */
 	new_fdt_addr = fdt_ram_start + (127 * 1024 * 1024) + fdt_size;
-	if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
-			       EFI_RUNTIME_SERVICES_DATA, fdt_pages,
-			       &new_fdt_addr) != EFI_SUCCESS) {
+	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
+				 EFI_RUNTIME_SERVICES_DATA, fdt_pages,
+				 &new_fdt_addr);
+	if (ret != EFI_SUCCESS) {
 		/* If we can't put it there, put it somewhere */
 		new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size);
-		if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
-				       EFI_RUNTIME_SERVICES_DATA, fdt_pages,
-				       &new_fdt_addr) != EFI_SUCCESS) {
+		ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
+					 EFI_RUNTIME_SERVICES_DATA, fdt_pages,
+					 &new_fdt_addr);
+		if (ret != EFI_SUCCESS) {
 			printf("ERROR: Failed to reserve space for FDT\n");
-			return NULL;
+			goto done;
 		}
 	}
 
-	new_fdt = (void*)(ulong)new_fdt_addr;
+	new_fdt = map_sysmem(new_fdt_addr, fdt_size);
 	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
 	fdt_set_totalsize(new_fdt, fdt_size);
 
-	return new_fdt;
+	*fdt_addrp = new_fdt_addr;
+	*fdt_sizep = fdt_size;
+done:
+	return ret;
 }
 
 static efi_status_t efi_do_enter(
@@ -215,22 +235,27 @@ static efi_status_t efi_carve_out_dt_rsv(void *fdt)
 	return EFI_SUCCESS;
 }
 
-static efi_status_t efi_install_fdt(void *fdt)
+static efi_status_t efi_install_fdt(ulong fdt_addr)
 {
 	bootm_headers_t img = { 0 };
 	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
 	efi_status_t ret;
+	void *fdt;
 
+	fdt = map_sysmem(fdt_addr, 0);
 	if (fdt_check_header(fdt)) {
 		printf("ERROR: invalid device tree\n");
 		return EFI_INVALID_PARAMETER;
 	}
 
 	/* Prepare fdt for payload */
-	fdt = copy_fdt(fdt);
-	if (!fdt)
-		return EFI_OUT_OF_RESOURCES;
+	ret = copy_fdt(&fdt_addr, &fdt_size);
+	if (ret)
+		return ret;
 
+	unmap_sysmem(fdt);
+	fdt = map_sysmem(fdt_addr, 0);
+	fdt_size = fdt_totalsize(fdt);
 	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
 		printf("ERROR: failed to process device tree\n");
 		return EFI_LOAD_ERROR;
@@ -247,14 +272,13 @@ static efi_status_t efi_install_fdt(void *fdt)
 		return EFI_OUT_OF_RESOURCES;
 
 	/* And reserve the space in the memory map */
-	fdt_start = ((ulong)fdt) & ~EFI_PAGE_MASK;
-	fdt_end = ((ulong)fdt) + fdt_totalsize(fdt);
-	fdt_size = (fdt_end - fdt_start) + EFI_PAGE_MASK;
+	fdt_start = fdt_addr;
+	fdt_end = fdt_addr + fdt_size;
 	fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
-	/* Give a bootloader the chance to modify the device tree */
-	fdt_pages += 2;
+
 	ret = efi_add_memory_map(fdt_start, fdt_pages,
 				 EFI_BOOT_SERVICES_DATA, true);
+
 	return ret;
 }
 
@@ -454,7 +478,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	unsigned long addr;
 	efi_status_t r;
 	unsigned long fdt_addr;
-	void *fdt;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -475,8 +498,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (!fdt_addr && *argv[2] != '0')
 			return CMD_RET_USAGE;
 		/* Install device tree */
-		fdt = map_sysmem(fdt_addr, 0);
-		r = efi_install_fdt(fdt);
+		r = efi_install_fdt(fdt_addr);
 		if (r != EFI_SUCCESS) {
 			printf("ERROR: failed to install device tree\n");
 			return CMD_RET_FAILURE;
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> At present this function takes a pointer as its argument, then passes this
> to efi_allocate_pages(), which actually takes an address. It uses casts,
> which are not supported on sandbox.

I think this is the big API misunderstanding that caused our 
disagreements: efi_allocate_pages(uint64_t *memory) gets a uint64_t 
*address as input parameter, but uses the same field to actually return 
a void * pointer. This is the function that really converts between 
virtual and physical address space.

This is the explicit wording of the spec[1] page 168:

   The AllocatePages() function allocates the requested number of pages 
and returns a pointer to the base address of the page range in the 
location referenced by Memory.

So yes, we have to cast. There is no other way around it without 
completely creating a trainwreck of the API.


Alex

[1] 
http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> At present this function takes a pointer as its argument, then passes this
>> to efi_allocate_pages(), which actually takes an address. It uses casts,
>> which are not supported on sandbox.
>
>
> I think this is the big API misunderstanding that caused our disagreements:
> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
> parameter, but uses the same field to actually return a void * pointer. This
> is the function that really converts between virtual and physical address
> space.
>
> This is the explicit wording of the spec[1] page 168:
>
>   The AllocatePages() function allocates the requested number of pages and
> returns a pointer to the base address of the page range in the location
> referenced by Memory.

The code at present uses *Memory as the address on both input and
output. The spec is confusing, but I suspect that is what it meant.

>
> So yes, we have to cast. There is no other way around it without completely
> creating a trainwreck of the API.

efi_allocate_pages_ext() can do this though. We don't need to copy the
API to efi_allocate_pages().

Of course this is future possible work, we don't need to do it now.

>
>
> Alex
>
> [1]
> http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf
>

Regards,
Simon

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Alexander Graf]

On 06/21/2018 04:01 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> At present this function takes a pointer as its argument, then passes this
>>> to efi_allocate_pages(), which actually takes an address. It uses casts,
>>> which are not supported on sandbox.
>>
>> I think this is the big API misunderstanding that caused our disagreements:
>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>> parameter, but uses the same field to actually return a void * pointer. This
>> is the function that really converts between virtual and physical address
>> space.
>>
>> This is the explicit wording of the spec[1] page 168:
>>
>>    The AllocatePages() function allocates the requested number of pages and
>> returns a pointer to the base address of the page range in the location
>> referenced by Memory.
> The code at present uses *Memory as the address on both input and
> output. The spec is confusing, but I suspect that is what it meant.

The spec means *Memory on IN is an address, on OUT is a pointer. It's 
quite clear on that. Since the functions is available to payloads, we 
should follow that semantic.

>> So yes, we have to cast. There is no other way around it without completely
>> creating a trainwreck of the API.
> efi_allocate_pages_ext() can do this though. We don't need to copy the
> API to efi_allocate_pages().

Yikes. There's no way we'll create a frankenstein not-really-almost EFI 
API inside U-Boot. Either we stick to the standard or we don't.


Alex

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Simon Glass]

Hi Alex,

On 21 June 2018 at 04:13, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> At present this function takes a pointer as its argument, then passes
>>>> this
>>>> to efi_allocate_pages(), which actually takes an address. It uses casts,
>>>> which are not supported on sandbox.
>>>
>>>
>>> I think this is the big API misunderstanding that caused our
>>> disagreements:
>>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>>> parameter, but uses the same field to actually return a void * pointer.
>>> This
>>> is the function that really converts between virtual and physical address
>>> space.
>>>
>>> This is the explicit wording of the spec[1] page 168:
>>>
>>>    The AllocatePages() function allocates the requested number of pages
>>> and
>>> returns a pointer to the base address of the page range in the location
>>> referenced by Memory.
>>
>> The code at present uses *Memory as the address on both input and
>> output. The spec is confusing, but I suspect that is what it meant.
>
>
> The spec means *Memory on IN is an address, on OUT is a pointer. It's quite
> clear on that. Since the functions is available to payloads, we should
> follow that semantic.
>
>>> So yes, we have to cast. There is no other way around it without
>>> completely
>>> creating a trainwreck of the API.
>>
>> efi_allocate_pages_ext() can do this though. We don't need to copy the
>> API to efi_allocate_pages().
>
>
> Yikes. There's no way we'll create a frankenstein not-really-almost EFI API
> inside U-Boot. Either we stick to the standard or we don't.

The API is the _ext() function, right? We can rename the internal
function if you like.

In any case I think you have this confused. From the spec:

     "Pointer to a physical address. On input, the way in which the
address is used depends on the value of Type. See “Description” for
more information. On output the address is set to the base of the page
range that was allocated. See “Related Definitions.”"

The parameter does not turn into a pointer on exit. It is an address,
just as it is on input. What am I missing?

Regards,
Simon

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 04:13, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>> At present this function takes a pointer as its argument, then passes
>>>>> this
>>>>> to efi_allocate_pages(), which actually takes an address. It uses casts,
>>>>> which are not supported on sandbox.
>>>>
>>>> I think this is the big API misunderstanding that caused our
>>>> disagreements:
>>>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>>>> parameter, but uses the same field to actually return a void * pointer.
>>>> This
>>>> is the function that really converts between virtual and physical address
>>>> space.
>>>>
>>>> This is the explicit wording of the spec[1] page 168:
>>>>
>>>>     The AllocatePages() function allocates the requested number of pages
>>>> and
>>>> returns a pointer to the base address of the page range in the location
>>>> referenced by Memory.
>>> The code at present uses *Memory as the address on both input and
>>> output. The spec is confusing, but I suspect that is what it meant.
>>
>> The spec means *Memory on IN is an address, on OUT is a pointer. It's quite
>> clear on that. Since the functions is available to payloads, we should
>> follow that semantic.
>>
>>>> So yes, we have to cast. There is no other way around it without
>>>> completely
>>>> creating a trainwreck of the API.
>>> efi_allocate_pages_ext() can do this though. We don't need to copy the
>>> API to efi_allocate_pages().
>>
>> Yikes. There's no way we'll create a frankenstein not-really-almost EFI API
>> inside U-Boot. Either we stick to the standard or we don't.
> The API is the _ext() function, right? We can rename the internal
> function if you like.
>
> In any case I think you have this confused. From the spec:
>
>       "Pointer to a physical address. On input, the way in which the
> address is used depends on the value of Type. See “Description” for
> more information. On output the address is set to the base of the page
> range that was allocated. See “Related Definitions.”"
>
> The parameter does not turn into a pointer on exit. It is an address,
> just as it is on input. What am I missing?

Just keep reading. A few lines further down:

   The AllocatePages() function allocates the requested number of pages 
and returns a pointer to the base address of the page range in the 
location referenced by Memory.

the spec explicitly says the function returns *a pointer to the base 
address*. It doesn't return an address. It returns a pointer.

Either way, I've applied the patch that calls map_sysmem() inside 
efi_allocate_pages() to efi-next.


Alex

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:26, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 04:13, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>
>>>>>> At present this function takes a pointer as its argument, then passes
>>>>>> this
>>>>>> to efi_allocate_pages(), which actually takes an address. It uses
>>>>>> casts,
>>>>>> which are not supported on sandbox.
>>>>>
>>>>>
>>>>> I think this is the big API misunderstanding that caused our
>>>>> disagreements:
>>>>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>>>>> parameter, but uses the same field to actually return a void * pointer.
>>>>> This
>>>>> is the function that really converts between virtual and physical
>>>>> address
>>>>> space.
>>>>>
>>>>> This is the explicit wording of the spec[1] page 168:
>>>>>
>>>>>     The AllocatePages() function allocates the requested number of
>>>>> pages
>>>>> and
>>>>> returns a pointer to the base address of the page range in the location
>>>>> referenced by Memory.
>>>>
>>>> The code at present uses *Memory as the address on both input and
>>>> output. The spec is confusing, but I suspect that is what it meant.
>>>
>>>
>>> The spec means *Memory on IN is an address, on OUT is a pointer. It's
>>> quite
>>> clear on that. Since the functions is available to payloads, we should
>>> follow that semantic.
>>>
>>>>> So yes, we have to cast. There is no other way around it without
>>>>> completely
>>>>> creating a trainwreck of the API.
>>>>
>>>> efi_allocate_pages_ext() can do this though. We don't need to copy the
>>>> API to efi_allocate_pages().
>>>
>>>
>>> Yikes. There's no way we'll create a frankenstein not-really-almost EFI
>>> API
>>> inside U-Boot. Either we stick to the standard or we don't.
>>
>> The API is the _ext() function, right? We can rename the internal
>> function if you like.
>>
>> In any case I think you have this confused. From the spec:
>>
>>       "Pointer to a physical address. On input, the way in which the
>> address is used depends on the value of Type. See “Description” for
>> more information. On output the address is set to the base of the page
>> range that was allocated. See “Related Definitions.”"
>>
>> The parameter does not turn into a pointer on exit. It is an address,
>> just as it is on input. What am I missing?
>
>
> Just keep reading. A few lines further down:
>
>   The AllocatePages() function allocates the requested number of pages and
> returns a pointer to the base address of the page range in the location
> referenced by Memory.
>
> the spec explicitly says the function returns *a pointer to the base
> address*. It doesn't return an address. It returns a pointer.

I think we must be talking at cross-purposes. Perhaps the spec is
ambiguous since I read it one way and you read it another. From my
side, it 'returns a pointer to the base address' says that the base
address is written to the pointer, but perhaps they mean what you
think they mean? But if so, it should be void **, not uint64_t *.

In any case it doesn't matter. It returns a 64-bit value which is both
a pointer and an address. There is no distinction from the EFI side.
From the U-Boot sandbox side, we must provide a pointer (both on input
and output) since EFI does not understand our internal RAM buffer
offsets.

>
> Either way, I've applied the patch that calls map_sysmem() inside
> efi_allocate_pages() to efi-next.

Which patch is that? Have I reviewed it?

Regards,
Simon

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Alexander Graf]

On 22.06.18 21:31, Simon Glass wrote:
> Hi Alex,
> 
> On 22 June 2018 at 06:26, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>
>>> Hi Alex,
>>>
>>> On 21 June 2018 at 04:13, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> At present this function takes a pointer as its argument, then passes
>>>>>>> this
>>>>>>> to efi_allocate_pages(), which actually takes an address. It uses
>>>>>>> casts,
>>>>>>> which are not supported on sandbox.
>>>>>>
>>>>>>
>>>>>> I think this is the big API misunderstanding that caused our
>>>>>> disagreements:
>>>>>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>>>>>> parameter, but uses the same field to actually return a void * pointer.
>>>>>> This
>>>>>> is the function that really converts between virtual and physical
>>>>>> address
>>>>>> space.
>>>>>>
>>>>>> This is the explicit wording of the spec[1] page 168:
>>>>>>
>>>>>>     The AllocatePages() function allocates the requested number of
>>>>>> pages
>>>>>> and
>>>>>> returns a pointer to the base address of the page range in the location
>>>>>> referenced by Memory.
>>>>>
>>>>> The code at present uses *Memory as the address on both input and
>>>>> output. The spec is confusing, but I suspect that is what it meant.
>>>>
>>>>
>>>> The spec means *Memory on IN is an address, on OUT is a pointer. It's
>>>> quite
>>>> clear on that. Since the functions is available to payloads, we should
>>>> follow that semantic.
>>>>
>>>>>> So yes, we have to cast. There is no other way around it without
>>>>>> completely
>>>>>> creating a trainwreck of the API.
>>>>>
>>>>> efi_allocate_pages_ext() can do this though. We don't need to copy the
>>>>> API to efi_allocate_pages().
>>>>
>>>>
>>>> Yikes. There's no way we'll create a frankenstein not-really-almost EFI
>>>> API
>>>> inside U-Boot. Either we stick to the standard or we don't.
>>>
>>> The API is the _ext() function, right? We can rename the internal
>>> function if you like.
>>>
>>> In any case I think you have this confused. From the spec:
>>>
>>>       "Pointer to a physical address. On input, the way in which the
>>> address is used depends on the value of Type. See “Description” for
>>> more information. On output the address is set to the base of the page
>>> range that was allocated. See “Related Definitions.”"
>>>
>>> The parameter does not turn into a pointer on exit. It is an address,
>>> just as it is on input. What am I missing?
>>
>>
>> Just keep reading. A few lines further down:
>>
>>   The AllocatePages() function allocates the requested number of pages and
>> returns a pointer to the base address of the page range in the location
>> referenced by Memory.
>>
>> the spec explicitly says the function returns *a pointer to the base
>> address*. It doesn't return an address. It returns a pointer.
> 
> I think we must be talking at cross-purposes. Perhaps the spec is
> ambiguous since I read it one way and you read it another. From my
> side, it 'returns a pointer to the base address' says that the base
> address is written to the pointer, but perhaps they mean what you
> think they mean? But if so, it should be void **, not uint64_t *.

The problem is that void ** is wrong for the IN path, so I assume they
had to decide on one and went with uint64_t * because that's always
bigger. Sizeof(void*) might be smaller than uint64_t on 32bit platforms.

> In any case it doesn't matter. It returns a 64-bit value which is both
> a pointer and an address. There is no distinction from the EFI side.
> From the U-Boot sandbox side, we must provide a pointer (both on input
> and output) since EFI does not understand our internal RAM buffer
> offsets.

Yes, I guess we agree by now :).

>> Either way, I've applied the patch that calls map_sysmem() inside
>> efi_allocate_pages() to efi-next.
> 
> Which patch is that? Have I reviewed it?

It's this beauty:


https://github.com/agraf/u-boot/commit/6f2ac2061ea499c4d23f407798b96b868cb2c3b4


Alex

[U-Boot] [PATCH v8 22/30] efi: sandbox: Tidy up copy_fdt() to work with sandbox

[Simon Glass]

Hi Alex,

On 23 June 2018 at 01:24, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 22.06.18 21:31, Simon Glass wrote:
>> Hi Alex,
>>
>> On 22 June 2018 at 06:26, Alexander Graf <agraf@suse.de> wrote:
>>> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 21 June 2018 at 04:13, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 18 June 2018 at 09:00, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> At present this function takes a pointer as its argument, then passes
>>>>>>>> this
>>>>>>>> to efi_allocate_pages(), which actually takes an address. It uses
>>>>>>>> casts,
>>>>>>>> which are not supported on sandbox.
>>>>>>>
>>>>>>>
>>>>>>> I think this is the big API misunderstanding that caused our
>>>>>>> disagreements:
>>>>>>> efi_allocate_pages(uint64_t *memory) gets a uint64_t *address as input
>>>>>>> parameter, but uses the same field to actually return a void * pointer.
>>>>>>> This
>>>>>>> is the function that really converts between virtual and physical
>>>>>>> address
>>>>>>> space.
>>>>>>>
>>>>>>> This is the explicit wording of the spec[1] page 168:
>>>>>>>
>>>>>>>     The AllocatePages() function allocates the requested number of
>>>>>>> pages
>>>>>>> and
>>>>>>> returns a pointer to the base address of the page range in the location
>>>>>>> referenced by Memory.
>>>>>>
>>>>>> The code at present uses *Memory as the address on both input and
>>>>>> output. The spec is confusing, but I suspect that is what it meant.
>>>>>
>>>>>
>>>>> The spec means *Memory on IN is an address, on OUT is a pointer. It's
>>>>> quite
>>>>> clear on that. Since the functions is available to payloads, we should
>>>>> follow that semantic.
>>>>>
>>>>>>> So yes, we have to cast. There is no other way around it without
>>>>>>> completely
>>>>>>> creating a trainwreck of the API.
>>>>>>
>>>>>> efi_allocate_pages_ext() can do this though. We don't need to copy the
>>>>>> API to efi_allocate_pages().
>>>>>
>>>>>
>>>>> Yikes. There's no way we'll create a frankenstein not-really-almost EFI
>>>>> API
>>>>> inside U-Boot. Either we stick to the standard or we don't.
>>>>
>>>> The API is the _ext() function, right? We can rename the internal
>>>> function if you like.
>>>>
>>>> In any case I think you have this confused. From the spec:
>>>>
>>>>       "Pointer to a physical address. On input, the way in which the
>>>> address is used depends on the value of Type. See “Description” for
>>>> more information. On output the address is set to the base of the page
>>>> range that was allocated. See “Related Definitions.”"
>>>>
>>>> The parameter does not turn into a pointer on exit. It is an address,
>>>> just as it is on input. What am I missing?
>>>
>>>
>>> Just keep reading. A few lines further down:
>>>
>>>   The AllocatePages() function allocates the requested number of pages and
>>> returns a pointer to the base address of the page range in the location
>>> referenced by Memory.
>>>
>>> the spec explicitly says the function returns *a pointer to the base
>>> address*. It doesn't return an address. It returns a pointer.
>>
>> I think we must be talking at cross-purposes. Perhaps the spec is
>> ambiguous since I read it one way and you read it another. From my
>> side, it 'returns a pointer to the base address' says that the base
>> address is written to the pointer, but perhaps they mean what you
>> think they mean? But if so, it should be void **, not uint64_t *.
>
> The problem is that void ** is wrong for the IN path, so I assume they
> had to decide on one and went with uint64_t * because that's always
> bigger. Sizeof(void*) might be smaller than uint64_t on 32bit platforms.
>
>> In any case it doesn't matter. It returns a 64-bit value which is both
>> a pointer and an address. There is no distinction from the EFI side.
>> From the U-Boot sandbox side, we must provide a pointer (both on input
>> and output) since EFI does not understand our internal RAM buffer
>> offsets.
>
> Yes, I guess we agree by now :).
>
>>> Either way, I've applied the patch that calls map_sysmem() inside
>>> efi_allocate_pages() to efi-next.
>>
>> Which patch is that? Have I reviewed it?
>
> It's this beauty:
>
>
> https://github.com/agraf/u-boot/commit/6f2ac2061ea499c4d23f407798b96b868cb2c3b4

OK thanks, I replied on that. I do think the address handling is
confusing, but we can worry about that in separate patches.

Regards,
Simon

[U-Boot] [PATCH v8 23/30] efi: Drop error return in efi_carve_out_dt_rsv()

[Simon Glass]

This function currently returns an error code, but never uses it. There is
no function comment so it is not obvious why. Presuambly the error is not
important.

Update the function to explain its purpose and why it ignores the error.
Drop the useful error return value.

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

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 5ef2d8499c..7c41b641f6 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -214,8 +214,16 @@ static efi_status_t efi_run_in_el2(EFIAPI efi_status_t (*entry)(
 }
 #endif
 
-/* Carve out DT reserved memory ranges */
-static efi_status_t efi_carve_out_dt_rsv(void *fdt)
+/*
+ * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
+ *
+ * The mem_rsv entries of the FDT are added to the memory map. Any failures are
+ * ignored because this is not critical and we would rather continue to try to
+ * boot.
+ *
+ * @fdt: Pointer to device tree
+ */
+static void efi_carve_out_dt_rsv(void *fdt)
 {
 	int nr_rsv, i;
 	uint64_t addr, size, pages;
@@ -228,11 +236,10 @@ static efi_status_t efi_carve_out_dt_rsv(void *fdt)
 			continue;
 
 		pages = ALIGN(size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT;
-		efi_add_memory_map(addr, pages, EFI_RESERVED_MEMORY_TYPE,
-				   false);
+		if (!efi_add_memory_map(addr, pages, EFI_RESERVED_MEMORY_TYPE,
+					false))
+			printf("FDT memrsv map %d: Failed to add to map\n", i);
 	}
-
-	return EFI_SUCCESS;
 }
 
 static efi_status_t efi_install_fdt(ulong fdt_addr)
@@ -261,10 +268,7 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
 		return EFI_LOAD_ERROR;
 	}
 
-	if (efi_carve_out_dt_rsv(fdt) != EFI_SUCCESS) {
-		printf("ERROR: failed to carve out memory\n");
-		return EFI_LOAD_ERROR;
-	}
+	efi_carve_out_dt_rsv(fdt);
 
 	/* Link to it in the efi tables */
 	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 23/30] efi: Drop error return in efi_carve_out_dt_rsv()

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> This function currently returns an error code, but never uses it. There is
> no function comment so it is not obvious why. Presuambly the error is not
> important.
>
> Update the function to explain its purpose and why it ignores the error.
> Drop the useful error return value.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[U-Boot] [U-Boot, v8, 23/30] efi: Drop error return in efi_carve_out_dt_rsv()

[Alexander Graf]

> This function currently returns an error code, but never uses it. There is
> no function comment so it is not obvious why. Presuambly the error is not
> important.
> 
> Update the function to explain its purpose and why it ignores the error.
> Drop the useful error return value.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v8 24/30] efi: Adjust memory handling to support sandbox

[Simon Glass]

Sandbox does not support direct casts between addresses and pointers,
since it uses an emulated RAM buffer rather than directly using host
RAM.

The current EFI code uses an integer type for addresses, but treats them
like pointers.

Update the code to maintain a (reasonably) clear separation between the
two, so that sandbox can work.

Unfortunately there remains an inconsistency between the arguments of
allocate_pages() and allocate_pool(). One takes an address and one takes
a pointer. Partly this seems to be defined by the boot services API itself
but it would be fairly easy to update the functions in efi_memory.c to be
consistent. However, this is a larger change and needs discussion.

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

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

 lib/efi_loader/efi_boottime.c     | 13 +++++++++++--
 lib/efi_loader/efi_image_loader.c |  3 ++-
 lib/efi_loader/efi_memory.c       | 17 +++++++++++++----
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 50d311548e..aefafc3fba 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -14,6 +14,7 @@
 #include <u-boot/crc.h>
 #include <bootm.h>
 #include <inttypes.h>
+#include <mapmem.h>
 #include <watchdog.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -305,9 +306,17 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
 						  uint64_t *memory)
 {
 	efi_status_t r;
+	uint64_t addr;
 
 	EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory);
-	r = efi_allocate_pages(type, memory_type, pages, memory);
+	if (memory_type == EFI_ALLOCATE_MAX_ADDRESS ||
+	    memory_type == EFI_ALLOCATE_ADDRESS)
+		addr = *memory == -1ULL ? *memory :
+			map_to_sysmem((void *)(uintptr_t)*memory);
+	else
+		addr = 0;
+	r = efi_allocate_pages(type, memory_type, pages, &addr);
+	*memory = (uintptr_t)map_sysmem(addr, pages << EFI_PAGE_SHIFT);
 	return EFI_EXIT(r);
 }
 
@@ -328,7 +337,7 @@ static efi_status_t EFIAPI efi_free_pages_ext(uint64_t memory,
 	efi_status_t r;
 
 	EFI_ENTRY("%" PRIx64 ", 0x%zx", memory, pages);
-	r = efi_free_pages(memory, pages);
+	r = efi_free_pages(map_to_sysmem((void *)memory), pages);
 	return EFI_EXIT(r);
 }
 
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index ecdb77e5b6..d5dd8864d7 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -9,6 +9,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <mapmem.h>
 #include <pe.h>
 
 const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
@@ -301,7 +302,7 @@ void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info)
 	/* Run through relocations */
 	if (efi_loader_relocate(rel, rel_size, efi_reloc,
 				(unsigned long)image_base) != EFI_SUCCESS) {
-		efi_free_pages((uintptr_t) efi_reloc,
+		efi_free_pages(map_to_sysmem(efi_reloc),
 			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
 		return NULL;
 	}
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index c6410613c7..ad61b723e6 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -345,7 +345,7 @@ void *efi_alloc(uint64_t len, int memory_type)
 	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
 			       &ret);
 	if (r == EFI_SUCCESS)
-		return (void*)(uintptr_t)ret;
+		return map_sysmem(ret, len);
 
 	return NULL;
 }
@@ -412,15 +412,17 @@ efi_status_t efi_free_pool(void *buffer)
 {
 	efi_status_t r;
 	struct efi_pool_allocation *alloc;
+	uintptr_t addr;
 
 	if (buffer == NULL)
 		return EFI_INVALID_PARAMETER;
 
 	alloc = container_of(buffer, struct efi_pool_allocation, data);
 	/* Sanity check, was the supplied address returned by allocate_pool */
-	assert(((uintptr_t)alloc & EFI_PAGE_MASK) == 0);
+	addr = map_to_sysmem(alloc);
+	assert((addr & EFI_PAGE_MASK) == 0);
 
-	r = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
+	r = efi_free_pages(addr, alloc->num_pages);
 
 	return r;
 }
@@ -471,7 +473,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 			struct efi_mem_list *lmem;
 
 			lmem = list_entry(lhandle, struct efi_mem_list, link);
-			*memory_map = lmem->desc;
+			memory_map->type = lmem->desc.type;
+			memory_map->reserved = 0;
+			memory_map->physical_start = (uintptr_t)map_sysmem(
+				lmem->desc.physical_start,
+				memory_map->num_pages << EFI_PAGE_SHIFT);
+			memory_map->virtual_start = memory_map->physical_start;
+			memory_map->num_pages = lmem->desc.num_pages;
+			memory_map->attribute = lmem->desc.attribute;
 			memory_map--;
 		}
 	}
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 24/30] efi: Adjust memory handling to support sandbox

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> Sandbox does not support direct casts between addresses and pointers,
> since it uses an emulated RAM buffer rather than directly using host
> RAM.
>
> The current EFI code uses an integer type for addresses, but treats them
> like pointers.
>
> Update the code to maintain a (reasonably) clear separation between the
> two, so that sandbox can work.
>
> Unfortunately there remains an inconsistency between the arguments of
> allocate_pages() and allocate_pool(). One takes an address and one takes
> a pointer. Partly this seems to be defined by the boot services API itself
> but it would be fairly easy to update the functions in efi_memory.c to be
> consistent. However, this is a larger change and needs discussion.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

This also seems to be fallout of a misinterpretation of the API :)


Alex

[U-Boot] [PATCH v8 24/30] efi: Adjust memory handling to support sandbox

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:03, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> Sandbox does not support direct casts between addresses and pointers,
>> since it uses an emulated RAM buffer rather than directly using host
>> RAM.
>>
>> The current EFI code uses an integer type for addresses, but treats them
>> like pointers.
>>
>> Update the code to maintain a (reasonably) clear separation between the
>> two, so that sandbox can work.
>>
>> Unfortunately there remains an inconsistency between the arguments of
>> allocate_pages() and allocate_pool(). One takes an address and one takes
>> a pointer. Partly this seems to be defined by the boot services API itself
>> but it would be fairly easy to update the functions in efi_memory.c to be
>> consistent. However, this is a larger change and needs discussion.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>

> This also seems to be fallout of a misinterpretation of the API :)

Well I suppose it's not important.

But AllocatePages() returns an address, but AllocatePool() returns a
pointer. I'm not sure why?

Regards,
Simon

[U-Boot] [PATCH v8 24/30] efi: Adjust memory handling to support sandbox

[Alexander Graf]

On 06/21/2018 04:01 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:03, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>> Sandbox does not support direct casts between addresses and pointers,
>>> since it uses an emulated RAM buffer rather than directly using host
>>> RAM.
>>>
>>> The current EFI code uses an integer type for addresses, but treats them
>>> like pointers.
>>>
>>> Update the code to maintain a (reasonably) clear separation between the
>>> two, so that sandbox can work.
>>>
>>> Unfortunately there remains an inconsistency between the arguments of
>>> allocate_pages() and allocate_pool(). One takes an address and one takes
>>> a pointer. Partly this seems to be defined by the boot services API itself
>>> but it would be fairly easy to update the functions in efi_memory.c to be
>>> consistent. However, this is a larger change and needs discussion.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> This also seems to be fallout of a misinterpretation of the API :)
> Well I suppose it's not important.
>
> But AllocatePages() returns an address, but AllocatePool() returns a
> pointer. I'm not sure why?

AllocatePages returns a pointer.


Alex

[U-Boot] [PATCH v8 24/30] efi: Adjust memory handling to support sandbox

[Simon Glass]

Hi Alex,

On 21 June 2018 at 04:14, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:03, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>
>>>> Sandbox does not support direct casts between addresses and pointers,
>>>> since it uses an emulated RAM buffer rather than directly using host
>>>> RAM.
>>>>
>>>> The current EFI code uses an integer type for addresses, but treats them
>>>> like pointers.
>>>>
>>>> Update the code to maintain a (reasonably) clear separation between the
>>>> two, so that sandbox can work.
>>>>
>>>> Unfortunately there remains an inconsistency between the arguments of
>>>> allocate_pages() and allocate_pool(). One takes an address and one takes
>>>> a pointer. Partly this seems to be defined by the boot services API
>>>> itself
>>>> but it would be fairly easy to update the functions in efi_memory.c to
>>>> be
>>>> consistent. However, this is a larger change and needs discussion.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>>
>>> This also seems to be fallout of a misinterpretation of the API :)
>>
>> Well I suppose it's not important.
>>
>> But AllocatePages() returns an address, but AllocatePool() returns a
>> pointer. I'm not sure why?
>
>
> AllocatePages returns a pointer.

It is declared:

IN OUT EFI_PHYSICAL_ADDRESS *Memory

So it returns an address, a EFI_PHYSICAL_ADDRESS, which is a uint64_t,
which is an address, not a pointer. This is how it is used in U-Boot
too.

The fact that it is declared as EFI_PHYSICAL_ADDRESS * is simply so it
can return a value. That is how things work in C. You cannot return a
value in an arg unless you make that are a pointer.

Regards,
Simon

[U-Boot] [PATCH v8 25/30] efi: Add more debugging for memory allocations

[Simon Glass]

Add some more verbose debugging when doing memory allocations. This might
help to find bugs later.

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

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

 lib/efi_loader/efi_boottime.c | 18 ++++++++++++++++++
 lib/efi_loader/efi_memory.c   | 22 +++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index aefafc3fba..2a41eb13aa 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -315,8 +315,12 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
 			map_to_sysmem((void *)(uintptr_t)*memory);
 	else
 		addr = 0;
+	debug("   input ptr %lx, addr %lx\n", (unsigned long)*memory,
+	      (unsigned long)addr);
 	r = efi_allocate_pages(type, memory_type, pages, &addr);
 	*memory = (uintptr_t)map_sysmem(addr, pages << EFI_PAGE_SHIFT);
+	debug("*  output addr %lx, ptr %lx\n", (unsigned long)addr,
+	      (unsigned long)*memory);
 	return EFI_EXIT(r);
 }
 
@@ -364,11 +368,25 @@ static efi_status_t EFIAPI efi_get_memory_map_ext(
 					uint32_t *descriptor_version)
 {
 	efi_status_t r;
+	int i, entries;
 
 	EFI_ENTRY("%p, %p, %p, %p, %p", memory_map_size, memory_map,
 		  map_key, descriptor_size, descriptor_version);
 	r = efi_get_memory_map(memory_map_size, memory_map, map_key,
 			       descriptor_size, descriptor_version);
+	entries = *memory_map_size / sizeof(struct efi_mem_desc);
+	debug("   memory_map_size=%zx (%lx entries)\n", *memory_map_size,
+	      (ulong)(*memory_map_size / sizeof(struct efi_mem_desc)));
+	if (memory_map) {
+		for (i = 0; i < entries; i++) {
+			struct efi_mem_desc *desc = &memory_map[i];
+
+			debug("   type %d, phys %lx, virt %lx, num_pages %lx, attribute %lx\n",
+			      desc->type, (ulong)desc->physical_start,
+			      (ulong)desc->virtual_start,
+			      (ulong)desc->num_pages, (ulong)desc->attribute);
+		}
+	}
 	return EFI_EXIT(r);
 }
 
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index ad61b723e6..856caa4a40 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -149,6 +149,24 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 	return EFI_CARVE_LOOP_AGAIN;
 }
 
+static void efi_mem_print(const char *name)
+{
+	struct list_head *lhandle;
+
+	debug("   %s: memory map\n", name);
+	list_for_each(lhandle, &efi_mem) {
+		struct efi_mem_list *lmem = list_entry(lhandle,
+			struct efi_mem_list, link);
+		struct efi_mem_desc *desc = &lmem->desc;
+
+		debug("   type %d, phys %lx, virt %lx, num_pages %lx, attribute %lx\n",
+		      desc->type, (ulong)desc->physical_start,
+		      (ulong)desc->virtual_start, (ulong)desc->num_pages,
+		      (ulong)desc->attribute);
+	}
+	debug("\n");
+}
+
 uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 			    bool overlap_only_ram)
 {
@@ -237,6 +255,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 
 	/* And make sure memory is listed in descending order */
 	efi_mem_sort();
+	efi_mem_print(__func__);
 
 	return start;
 }
@@ -453,7 +472,8 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 		map_entries++;
 
 	map_size = map_entries * sizeof(struct efi_mem_desc);
-
+	debug("%s: map_size %lx, provided_map_size %lx\n", __func__,
+	      (ulong)map_size, (ulong)provided_map_size);
 	*memory_map_size = map_size;
 
 	if (provided_map_size < map_size)
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 25/30] efi: Add more debugging for memory allocations

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> Add some more verbose debugging when doing memory allocations. This might
> help to find bugs later.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Very useful indeed.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[U-Boot] [PATCH v8 25/30] efi: Add more debugging for memory allocations

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> Add some more verbose debugging when doing memory allocations. This might
> help to find bugs later.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   lib/efi_loader/efi_boottime.c | 18 ++++++++++++++++++
>   lib/efi_loader/efi_memory.c   | 22 +++++++++++++++++++++-
>   2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index aefafc3fba..2a41eb13aa 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -315,8 +315,12 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
>   			map_to_sysmem((void *)(uintptr_t)*memory);
>   	else
>   		addr = 0;
> +	debug("   input ptr %lx, addr %lx\n", (unsigned long)*memory,
> +	      (unsigned long)addr);
>   	r = efi_allocate_pages(type, memory_type, pages, &addr);
>   	*memory = (uintptr_t)map_sysmem(addr, pages << EFI_PAGE_SHIFT);
> +	debug("*  output addr %lx, ptr %lx\n", (unsigned long)addr,
> +	      (unsigned long)*memory);

2 nits:

1) On input, *memory is addr, on output *memory is ptr. I don't quite 
understand what the "addr" part above is supposed to do, but I'm fairly 
sure it's just remainders of some previous (incorrect) patch.

2) Please don't put any debugging into _ext functions. I introduced them 
before we had EFI_CALL() which is a much better API for calling EFI 
functions. So sooner or later we'll probably get rid of _ext functions 
altogether and instead just call the EFI functions using EFI_CALL(). 
We'd lose all debugging output then.

>   	return EFI_EXIT(r);
>   }
>   
> @@ -364,11 +368,25 @@ static efi_status_t EFIAPI efi_get_memory_map_ext(
>   					uint32_t *descriptor_version)

Same comment about _ext functions here.

>   {
>   	efi_status_t r;
> +	int i, entries;
>   
>   	EFI_ENTRY("%p, %p, %p, %p, %p", memory_map_size, memory_map,
>   		  map_key, descriptor_size, descriptor_version);
>   	r = efi_get_memory_map(memory_map_size, memory_map, map_key,
>   			       descriptor_size, descriptor_version);
> +	entries = *memory_map_size / sizeof(struct efi_mem_desc);
> +	debug("   memory_map_size=%zx (%lx entries)\n", *memory_map_size,
> +	      (ulong)(*memory_map_size / sizeof(struct efi_mem_desc)));
> +	if (memory_map) {
> +		for (i = 0; i < entries; i++) {
> +			struct efi_mem_desc *desc = &memory_map[i];
> +
> +			debug("   type %d, phys %lx, virt %lx, num_pages %lx, attribute %lx\n",
> +			      desc->type, (ulong)desc->physical_start,
> +			      (ulong)desc->virtual_start,
> +			      (ulong)desc->num_pages, (ulong)desc->attribute);
> +		}
> +	}
>   	return EFI_EXIT(r);
>   }
>   
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index ad61b723e6..856caa4a40 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -149,6 +149,24 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>   	return EFI_CARVE_LOOP_AGAIN;
>   }
>   
> +static void efi_mem_print(const char *name)
> +{
> +	struct list_head *lhandle;
> +
> +	debug("   %s: memory map\n", name);
> +	list_for_each(lhandle, &efi_mem) {
> +		struct efi_mem_list *lmem = list_entry(lhandle,
> +			struct efi_mem_list, link);

Are you sure the compiler is smart enough to optimize out the list 
walking in the debug case?


Alex

> +		struct efi_mem_desc *desc = &lmem->desc;
> +
> +		debug("   type %d, phys %lx, virt %lx, num_pages %lx, attribute %lx\n",
> +		      desc->type, (ulong)desc->physical_start,
> +		      (ulong)desc->virtual_start, (ulong)desc->num_pages,
> +		      (ulong)desc->attribute);
> +	}
> +	debug("\n");
> +}
> +
>   uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
>   			    bool overlap_only_ram)
>   {
> @@ -237,6 +255,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
>   
>   	/* And make sure memory is listed in descending order */
>   	efi_mem_sort();
> +	efi_mem_print(__func__);
>   
>   	return start;
>   }
> @@ -453,7 +472,8 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>   		map_entries++;
>   
>   	map_size = map_entries * sizeof(struct efi_mem_desc);
> -
> +	debug("%s: map_size %lx, provided_map_size %lx\n", __func__,
> +	      (ulong)map_size, (ulong)provided_map_size);
>   	*memory_map_size = map_size;
>   
>   	if (provided_map_size < map_size)

[U-Boot] [PATCH v8 25/30] efi: Add more debugging for memory allocations

[Simon Glass]

Hi Alex,

On 21 June 2018 at 10:45, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> Add some more verbose debugging when doing memory allocations. This might
>> help to find bugs later.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   lib/efi_loader/efi_boottime.c | 18 ++++++++++++++++++
>>   lib/efi_loader/efi_memory.c   | 22 +++++++++++++++++++++-
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index aefafc3fba..2a41eb13aa 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -315,8 +315,12 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int
>> type, int memory_type,
>>                         map_to_sysmem((void *)(uintptr_t)*memory);
>>         else
>>                 addr = 0;
>> +       debug("   input ptr %lx, addr %lx\n", (unsigned long)*memory,
>> +             (unsigned long)addr);
>>         r = efi_allocate_pages(type, memory_type, pages, &addr);
>>         *memory = (uintptr_t)map_sysmem(addr, pages << EFI_PAGE_SHIFT);
>> +       debug("*  output addr %lx, ptr %lx\n", (unsigned long)addr,
>> +             (unsigned long)*memory);
>
>
> 2 nits:
>
> 1) On input, *memory is addr, on output *memory is ptr. I don't quite
> understand what the "addr" part above is supposed to do, but I'm fairly sure
> it's just remainders of some previous (incorrect) patch.

I don't want to raise this misconception in another thread.

>
> 2) Please don't put any debugging into _ext functions. I introduced them
> before we had EFI_CALL() which is a much better API for calling EFI
> functions. So sooner or later we'll probably get rid of _ext functions
> altogether and instead just call the EFI functions using EFI_CALL(). We'd
> lose all debugging output then.

OK, let's worry about this patch later, if we can get things agreed and landed.

>
>>         return EFI_EXIT(r);
>>   }
>>   @@ -364,11 +368,25 @@ static efi_status_t EFIAPI efi_get_memory_map_ext(
>>                                         uint32_t *descriptor_version)
>
>
> Same comment about _ext functions here.

OK.

>
>
>>   {
>>         efi_status_t r;
>> +       int i, entries;
>>         EFI_ENTRY("%p, %p, %p, %p, %p", memory_map_size, memory_map,
>>                   map_key, descriptor_size, descriptor_version);
>>         r = efi_get_memory_map(memory_map_size, memory_map, map_key,
>>                                descriptor_size, descriptor_version);
>> +       entries = *memory_map_size / sizeof(struct efi_mem_desc);
>> +       debug("   memory_map_size=%zx (%lx entries)\n", *memory_map_size,
>> +             (ulong)(*memory_map_size / sizeof(struct efi_mem_desc)));
>> +       if (memory_map) {
>> +               for (i = 0; i < entries; i++) {
>> +                       struct efi_mem_desc *desc = &memory_map[i];
>> +
>> +                       debug("   type %d, phys %lx, virt %lx, num_pages
>> %lx, attribute %lx\n",
>> +                             desc->type, (ulong)desc->physical_start,
>> +                             (ulong)desc->virtual_start,
>> +                             (ulong)desc->num_pages,
>> (ulong)desc->attribute);
>> +               }
>> +       }
>>         return EFI_EXIT(r);
>>   }
>>   diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index ad61b723e6..856caa4a40 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -149,6 +149,24 @@ static s64 efi_mem_carve_out(struct efi_mem_list
>> *map,
>>         return EFI_CARVE_LOOP_AGAIN;
>>   }
>>   +static void efi_mem_print(const char *name)
>> +{
>> +       struct list_head *lhandle;
>> +
>> +       debug("   %s: memory map\n", name);
>> +       list_for_each(lhandle, &efi_mem) {
>> +               struct efi_mem_list *lmem = list_entry(lhandle,
>> +                       struct efi_mem_list, link);
>
>
> Are you sure the compiler is smart enough to optimize out the list walking
> in the debug case?

Yes it does for me, but I suppose it is not guaranteed that all
compilers would. In any case, I don't think it matters if an old
compiler is a bit crap and makes things a little slower.

[...]

Regards,
Simon

[U-Boot] [PATCH v8 26/30] efi_loader: Use compiler constants for image loader

[Simon Glass]

From: Alexander Graf <agraf@suse.de>

The EFI image loader tries to determine which target architecture we're
working with to only load PE binaries that match.

So far this has worked based on CONFIG defines, because the target CPU
was always indicated by a config define. With sandbox however, this is
not longer true as all sandbox targets only encompass a single CONFIG
option and so we need to use compiler defines to determine the CPU
architecture.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

 lib/efi_loader/efi_image_loader.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index d5dd8864d7..7e281f55e4 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -20,25 +20,25 @@ const efi_guid_t efi_simple_file_system_protocol_guid =
 const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
 
 static int machines[] = {
-#if defined(CONFIG_ARM64)
+#if defined(__aarch64__)
 	IMAGE_FILE_MACHINE_ARM64,
-#elif defined(CONFIG_ARM)
+#elif defined(__arm__)
 	IMAGE_FILE_MACHINE_ARM,
 	IMAGE_FILE_MACHINE_THUMB,
 	IMAGE_FILE_MACHINE_ARMNT,
 #endif
 
-#if defined(CONFIG_X86_64)
+#if defined(__x86_64__)
 	IMAGE_FILE_MACHINE_AMD64,
-#elif defined(CONFIG_X86)
+#elif defined(__i386__)
 	IMAGE_FILE_MACHINE_I386,
 #endif
 
-#if defined(CONFIG_CPU_RISCV_32)
+#if defined(__riscv) && (__riscv_xlen == 32)
 	IMAGE_FILE_MACHINE_RISCV32,
 #endif
 
-#if defined(CONFIG_CPU_RISCV_64)
+#if defined(__riscv) && (__riscv_xlen == 64)
 	IMAGE_FILE_MACHINE_RISCV64,
 #endif
 	0 };
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 27/30] efi_loader: efi_allocate_pages is too restrictive

[Simon Glass]

From: Heinrich Schuchardt <xypron.glpk@gmx.de>

When running on the sandbox the stack is not necessarily at a higher memory
address than the highest free memory.

There is no reason why the checking of the highest memory address should be
more restrictive for EFI_ALLOCATE_ANY_PAGES than for
EFI_ALLOCATE_MAX_ADDRESS.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
[agraf: use -1ULL instead]
Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

 lib/efi_loader/efi_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 856caa4a40..7804adacd2 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -315,7 +315,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
 	switch (type) {
 	case EFI_ALLOCATE_ANY_PAGES:
 		/* Any page */
-		addr = efi_find_free_memory(len, gd->start_addr_sp);
+		addr = efi_find_free_memory(len, -1ULL);
 		if (!addr) {
 			r = EFI_NOT_FOUND;
 			break;
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 28/30] efi_loader: Disable miniapps on sandbox

[Simon Glass]

From: Alexander Graf <agraf@suse.de>

In the sandbox environment we can not easily build efi stub binaries
right now, so let's disable the respective test cases for the efi
selftest suite.

Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

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

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index 4fe404d88d..bf5c8199cb 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -41,7 +41,7 @@ endif
 
 # TODO: As of v2018.01 the relocation code for the EFI application cannot
 # be built on x86_64.
-ifeq ($(CONFIG_X86_64),)
+ifeq ($(CONFIG_X86_64)$(CONFIG_SANDBOX),)
 
 ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST),)
 
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 29/30] efi_loader: Pass address to fs_read()

[Simon Glass]

From: Alexander Graf <agraf@suse.de>

The fs_read() function wants to get an address rather than the
pointer to a buffer.

So let's convert the passed buffer from pointer back a the address
to make efi_loader on sandbox happier.

Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

 lib/efi_loader/efi_file.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index e6a15bcb52..2107730ba5 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -9,6 +9,7 @@
 #include <charset.h>
 #include <efi_loader.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <fs.h>
 
 /* GUID for file system information */
@@ -232,8 +233,10 @@ static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
 		void *buffer)
 {
 	loff_t actread;
+	/* fs_read expects buffer address, not pointer */
+	uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);
 
-	if (fs_read(fh->path, (ulong)buffer, fh->offset,
+	if (fs_read(fh->path, buffer_addr, fh->offset,
 		    *buffer_size, &actread))
 		return EFI_DEVICE_ERROR;
 
-- 
2.18.0.rc1.244.gcf134e6275-goog

[U-Boot] [PATCH v8 29/30] efi_loader: Pass address to fs_read()

[Alexander Graf]

On 06/18/2018 04:08 PM, Simon Glass wrote:
> From: Alexander Graf <agraf@suse.de>
>
> The fs_read() function wants to get an address rather than the
> pointer to a buffer.
>
> So let's convert the passed buffer from pointer back a the address
> to make efi_loader on sandbox happier.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   lib/efi_loader/efi_file.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index e6a15bcb52..2107730ba5 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -9,6 +9,7 @@
>   #include <charset.h>
>   #include <efi_loader.h>
>   #include <malloc.h>
> +#include <mapmem.h>
>   #include <fs.h>
>   
>   /* GUID for file system information */
> @@ -232,8 +233,10 @@ static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
>   		void *buffer)
>   {
>   	loff_t actread;
> +	/* fs_read expects buffer address, not pointer */
> +	uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);

As you've seen with your patch on the stack based map_to_sysmem() 
calculation, map_to_sysmem() really should only be used on pointers that 
we are sure come from RAM. With EFI binaries, that isn't true because of 
the stack, but it might be true due to other reasons too on real 
hardware, such as direct read/write to/from flash.

I think it's much safer to stay in pointer land once we passed the addr 
-> ptr boundary. Going from ptr -> addr is usually a recipe for disaster.


Alex

[U-Boot] [PATCH v8 29/30] efi_loader: Pass address to fs_read()

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:08, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> From: Alexander Graf <agraf@suse.de>
>>
>> The fs_read() function wants to get an address rather than the
>> pointer to a buffer.
>>
>> So let's convert the passed buffer from pointer back a the address
>> to make efi_loader on sandbox happier.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   lib/efi_loader/efi_file.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
>> index e6a15bcb52..2107730ba5 100644
>> --- a/lib/efi_loader/efi_file.c
>> +++ b/lib/efi_loader/efi_file.c
>> @@ -9,6 +9,7 @@
>>   #include <charset.h>
>>   #include <efi_loader.h>
>>   #include <malloc.h>
>> +#include <mapmem.h>
>>   #include <fs.h>
>>     /* GUID for file system information */
>> @@ -232,8 +233,10 @@ static efi_status_t file_read(struct file_handle *fh,
>> u64 *buffer_size,
>>                 void *buffer)
>>   {
>>         loff_t actread;
>> +       /* fs_read expects buffer address, not pointer */
>> +       uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);
>
>
> As you've seen with your patch on the stack based map_to_sysmem()
> calculation, map_to_sysmem() really should only be used on pointers that we
> are sure come from RAM. With EFI binaries, that isn't true because of the
> stack, but it might be true due to other reasons too on real hardware, such
> as direct read/write to/from flash.
>
> I think it's much safer to stay in pointer land once we passed the addr ->
> ptr boundary. Going from ptr -> addr is usually a recipe for disaster.

We actually have no choice but to support this. As mentioned elsewhere
addresses are the main currency in U-Boot and we should not look to
convert it to use pointers. They are much less enjoyable to work with.
The above patch is pretty simple.

Regards,
Simon

[U-Boot] [PATCH v8 30/30] efi: sandbox: Enable selftest command

[Simon Glass]

Enable this for sandbox since it passes now.

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

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

 configs/sandbox_defconfig | 1 +
 lib/efi_selftest/Kconfig  | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 2fc84a16c9..aa15d4acb9 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -200,3 +200,4 @@ CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
 CONFIG_UT_ENV=y
 CONFIG_UT_OVERLAY=y
+CONFIG_CMD_BOOTEFI_SELFTEST=y
diff --git a/lib/efi_selftest/Kconfig b/lib/efi_selftest/Kconfig
index b52696778d..59f9f36801 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 && !SANDBOX
+	depends on CMD_BOOTEFI
 	imply FAT
 	imply FAT_WRITE
 	help
-- 
2.18.0.rc1.244.gcf134e6275-goog