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

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

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

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

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

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

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

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

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

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

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

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

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

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

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