[U-Boot] [PATCH v2 00/11] sandbox: efi_loader support

[U-Boot] [PATCH v2 00/11] sandbox: efi_loader support

[Alexander Graf]

This patch set augments Simon's patch set for efi_loader support
in sandbox[1], but follows a different memory allocation scheme.

Instead of keeping U-Boot (physical) addresses in the EFI memory
map, this patch set makes the EFI memory map contain host virtual
(virtual) addresses. That way most logic "just works" and all EFI
interfaces automatically gain sandbox awareness.

With this patch set in place, I can run a good chunk of the selftest
suite as well as efi binaries compiled using gnu-efi.

Alex

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=49832

v1 -> v2:

  - only compile efi_add_known_memory if efi_loader is enabled
  - clarify address vs pointer in fs_read patch
  - include mapmem.h

Alexander Graf (7):
  efi_loader: Use compiler constants for image loader
  efi_loader: Use map_sysmem() in bootefi command
  efi.h: Do not use config options
  efi_loader: Allow SMBIOS tables in highmem
  sandbox: Map host memory for efi_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 (3):
  efi: sandbox: Add distroboot support
  efi: sandbox: Add relocation constants
  efi: sandbox: Enable EFI loader for sandbox

 arch/sandbox/cpu/cpu.c            | 20 ++++++++++++++++++++
 cmd/bootefi.c                     | 13 ++++++++-----
 include/config_distro_bootcmd.h   | 13 +++++++++++++
 include/efi.h                     | 17 ++++-------------
 lib/efi/Makefile                  |  4 ++--
 lib/efi_loader/Kconfig            |  2 +-
 lib/efi_loader/efi_file.c         |  5 ++++-
 lib/efi_loader/efi_image_loader.c | 12 ++++++------
 lib/efi_loader/efi_memory.c       |  2 +-
 lib/efi_loader/efi_runtime.c      | 12 ++++++++++++
 lib/efi_loader/efi_smbios.c       | 11 +++++++++--
 lib/efi_selftest/Makefile         |  2 +-
 12 files changed, 81 insertions(+), 32 deletions(-)

-- 
2.12.3

[U-Boot] [PATCH v2 01/11] efi: sandbox: Add distroboot support

[Alexander Graf]

From: Simon Glass <sjg@chromium.org>

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>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/config_distro_bootcmd.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

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

[U-Boot] [PATCH v2 02/11] efi: sandbox: Add relocation constants

[Alexander Graf]

From: Simon Glass <sjg@chromium.org>

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>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_runtime.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 4874eb602f..388dfb9840 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -62,6 +62,18 @@ struct dyn_sym {
 #define R_ABSOLUTE	R_RISCV_64
 #define SYM_INDEX	32
 #endif
+
+/* For sandbox we only support 64-bit x86 at present */
+#elif defined(CONFIG_SANDBOX)
+/*
+ * TODO(sjg at chromium.org): Consider providing a way to enable sandbox features
+ * based on the host architecture
+ */
+# ifndef __x86_64__
+#  warning "sandbox EFI support is only tested on 64-bit x86"
+# endif
+#define R_RELATIVE	8
+#define R_MASK		0xffffffffULL
 #else
 #error Need to add relocation awareness
 #endif
-- 
2.12.3

[U-Boot] [PATCH v2 03/11] efi_loader: Use compiler constants for image loader

[Alexander Graf]

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>
---
 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 ecdb77e5b6..fdf40a62c8 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -19,25 +19,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.12.3

[U-Boot] [PATCH v2 04/11] efi_loader: Use map_sysmem() in bootefi command

[Alexander Graf]

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>
---
 cmd/bootefi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index f55a40dc84..a86a2bd4a9 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>
@@ -389,7 +390,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	unsigned long addr;
 	char *saddr;
 	efi_status_t r;
-	void *fdt_addr;
+	unsigned long fdt_addr;
+	void *fdt;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -406,11 +408,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;
@@ -429,7 +432,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
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
@@ -475,7 +478,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.12.3

[U-Boot] [PATCH v2 05/11] efi.h: Do not use config options

[Alexander Graf]

Currently efi.h determines a few bits of its environment according to
config options. This falls apart with the efi stub support which may
result in efi.h getting pulled into the stub as well as real U-Boot
code. In that case, one may be 32bit while the other one is 64bit.

This patch changes the conditionals to use compiler provided defines
instead. That way we always adhere to the build environment we're in
and the definitions adjust automatically.

Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
 include/efi.h    | 17 ++++-------------
 lib/efi/Makefile |  4 ++--
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index e30a3c51c6..826d484977 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -19,12 +19,12 @@
 #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 */
+/* EFI on x86_64 uses the Microsoft ABI which is not the default for GCC */
+#ifdef __x86_64__
 #define EFIAPI __attribute__((ms_abi))
 #else
 #define EFIAPI asmlinkage
-#endif
+#endif /* __x86_64__ */
 
 struct efi_device_path;
 
@@ -32,16 +32,7 @@ typedef struct {
 	u8 b[16];
 } efi_guid_t;
 
-#define EFI_BITS_PER_LONG	BITS_PER_LONG
-
-/*
- * With 64-bit EFI stub, EFI_BITS_PER_LONG has to be 64. EFI_STUB is set
- * in lib/efi/Makefile, when building the stub.
- */
-#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB)
-#undef EFI_BITS_PER_LONG
-#define EFI_BITS_PER_LONG	64
-#endif
+#define EFI_BITS_PER_LONG	(sizeof(long) * 8)
 
 /* Bit mask for EFI status code with error */
 #define EFI_ERROR_MASK (1UL << (EFI_BITS_PER_LONG - 1))
diff --git a/lib/efi/Makefile b/lib/efi/Makefile
index 18d081ac46..ece7907227 100644
--- a/lib/efi/Makefile
+++ b/lib/efi/Makefile
@@ -7,9 +7,9 @@ obj-$(CONFIG_EFI_STUB) += efi_info.o
 
 CFLAGS_REMOVE_efi_stub.o := -mregparm=3 \
 	$(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32)
-CFLAGS_efi_stub.o := -fpic -fshort-wchar -DEFI_STUB
+CFLAGS_efi_stub.o := -fpic -fshort-wchar
 CFLAGS_REMOVE_efi.o := -mregparm=3 \
 	$(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32)
-CFLAGS_efi.o := -fpic -fshort-wchar -DEFI_STUB
+CFLAGS_efi.o := -fpic -fshort-wchar
 
 extra-$(CONFIG_EFI_STUB) += efi_stub.o efi.o
-- 
2.12.3

[U-Boot] [PATCH v2 06/11] efi_loader: Allow SMBIOS tables in highmem

[Alexander Graf]

We try hard to make sure that SMBIOS tables live in the lower 32bit.
However, when we can not find any space at all there, we should not
error out but instead just fall back to map them in the full address
space instead.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_smbios.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 7c3fc8af0b..932f7582ec 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -26,8 +26,15 @@ efi_status_t efi_smbios_register(void)
 	/* Reserve 4kiB page for SMBIOS */
 	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
 				 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
-	if (ret != EFI_SUCCESS)
-		return ret;
+
+	if (ret != EFI_SUCCESS) {
+		/* Could not find space in lowmem, use highmem instead */
+		ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+					 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
+
+		if (ret != EFI_SUCCESS)
+			return ret;
+	}
 
 	/*
 	 * Generate SMBIOS tables - we know that efi_allocate_pages() returns
-- 
2.12.3

[U-Boot] [PATCH v2 07/11] sandbox: Map host memory for efi_loader

[Alexander Graf]

With efi_loader we do not control payload applications, so we can not
teach them about the difference between virtual and physical addresses.

Instead, let's just always map host virtual addresses in the efi memory
map. That way we can be sure that all memory allocation functions always
return consumable pointers.

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

---

v1 -> v2:

  - only compile efi_add_known_memory if efi_loader is enabled
---
 arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index cde0b055a6..23d8b70648 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -5,6 +5,7 @@
 #define DEBUG
 #include <common.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <errno.h>
 #include <linux/libfdt.h>
 #include <os.h>
@@ -177,3 +178,22 @@ void longjmp(jmp_buf jmp, int ret)
 	while (1)
 		;
 }
+
+#ifdef CONFIG_EFI_LOADER
+
+/*
+ * In sandbox, we don't have a 1:1 map, so we need to expose
+ * process addresses instead of U-Boot addresses
+ */
+void efi_add_known_memory(void)
+{
+	u64 ram_start = (uintptr_t)map_sysmem(0, gd->ram_size);
+	u64 ram_size = gd->ram_size;
+	u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
+	u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
+
+	efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
+			   false);
+}
+
+#endif
-- 
2.12.3

[U-Boot] [PATCH v2 08/11] efi_loader: efi_allocate_pages is too restrictive

[Alexander Graf]

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>
---
 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 ec66af98ea..ce29bcc6a3 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -295,7 +295,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.12.3

[U-Boot] [PATCH v2 09/11] efi_loader: Disable miniapps on sandbox

[Alexander Graf]

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>
---
 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.12.3

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Alexander Graf]

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>

---

v1 -> v2:

  - Clarify address vs pointer
  - include mapmem.h
---
 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.12.3

[U-Boot] [PATCH v2 11/11] efi: sandbox: Enable EFI loader for sandbox

[Alexander Graf]

From: Simon Glass <sjg@chromium.org>

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>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 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.12.3

[U-Boot] [PATCH v2 04/11] efi_loader: Use map_sysmem() in bootefi command

[Simon Glass]

On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
> 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>
> ---
>  cmd/bootefi.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

Looks right to me. You might consider doing the map lower down in
efi_load_pe() but since you don't ever log the address, I suppose it
is OK.

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

[U-Boot] [PATCH v2 03/11] efi_loader: Use compiler constants for image loader

[Simon Glass]

Hi Alex,

On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
> 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>
> ---
>  lib/efi_loader/efi_image_loader.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>

NAK to this for now until we figure out a response to my concerns
previously expressed.

Regards,
Simon

[U-Boot] [PATCH v2 09/11] efi_loader: Disable miniapps on sandbox

[Simon Glass]

On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
> 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>
> ---
>  lib/efi_selftest/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Simon Glass]

On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
> 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>
>
> ---
>
> v1 -> v2:
>
>   - Clarify address vs pointer
>   - include mapmem.h
> ---
>  lib/efi_loader/efi_file.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

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

[U-Boot] [PATCH v2 06/11] efi_loader: Allow SMBIOS tables in highmem

[Simon Glass]

Hi Alex,

On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
> We try hard to make sure that SMBIOS tables live in the lower 32bit.
> However, when we can not find any space at all there, we should not
> error out but instead just fall back to map them in the full address
> space instead.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  lib/efi_loader/efi_smbios.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Does that actually work? I thought the addresses in the table were
always 32-bit?

Regards,
Simon

[U-Boot] [PATCH v2 07/11] sandbox: Map host memory for efi_loader

[Simon Glass]

Hi Alex,

On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
> With efi_loader we do not control payload applications, so we can not
> teach them about the difference between virtual and physical addresses.
>
> Instead, let's just always map host virtual addresses in the efi memory
> map. That way we can be sure that all memory allocation functions always
> return consumable pointers.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> v1 -> v2:
>
>   - only compile efi_add_known_memory if efi_loader is enabled
> ---
>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

NAK.

You should not point sandbox pointers into the EFI tables. I know it
looks like a clever shortcut, but it is not correct. It will mess up
logging and debugging, since those pointers bear no easily accessible
relationship to U-Boot address.

Please start from my v7 patch. I'm happy to help do this correctly.
But, again, I think it should come after we have basic sandbox EFI
support applied.

Regards,
Simon

[U-Boot] [PATCH v2 06/11] efi_loader: Allow SMBIOS tables in highmem

[Alexander Graf]

On 14.06.18 21:01, Simon Glass wrote:
> Hi Alex,
> 
> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>> We try hard to make sure that SMBIOS tables live in the lower 32bit.
>> However, when we can not find any space at all there, we should not
>> error out but instead just fall back to map them in the full address
>> space instead.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  lib/efi_loader/efi_smbios.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> Does that actually work? I thought the addresses in the table were
> always 32-bit?


There is only a single table reference which indeed is 32bit:
se->struct_table_address.

That address however is unused in the EFI case usually, because the
SMBIOS information is already encapsulated in a table, so there's no
need to search through address space for a _DMI_ entry:

  https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5122


Alex

[U-Boot] [PATCH v2 07/11] sandbox: Map host memory for efi_loader

[Alexander Graf]

On 14.06.18 21:02, Simon Glass wrote:
> Hi Alex,
> 
> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>> With efi_loader we do not control payload applications, so we can not
>> teach them about the difference between virtual and physical addresses.
>>
>> Instead, let's just always map host virtual addresses in the efi memory
>> map. That way we can be sure that all memory allocation functions always
>> return consumable pointers.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> v1 -> v2:
>>
>>   - only compile efi_add_known_memory if efi_loader is enabled
>> ---
>>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
> 
> NAK.
> 
> You should not point sandbox pointers into the EFI tables. I know it
> looks like a clever shortcut, but it is not correct. It will mess up
> logging and debugging, since those pointers bear no easily accessible
> relationship to U-Boot address.
> 
> Please start from my v7 patch. I'm happy to help do this correctly.
> But, again, I think it should come after we have basic sandbox EFI
> support applied.

I don't want to play ping pong with you here. NAK on your approach until
I see it properly executing selftest.

So either we drive this forward or we don't. Your choice.


Alex

[U-Boot] [PATCH v2 07/11] sandbox: Map host memory for efi_loader

[Heinrich Schuchardt]

On 06/14/2018 09:02 PM, Simon Glass wrote:
> Hi Alex,
> 
> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>> With efi_loader we do not control payload applications, so we can not
>> teach them about the difference between virtual and physical addresses.
>>
>> Instead, let's just always map host virtual addresses in the efi memory
>> map. That way we can be sure that all memory allocation functions always
>> return consumable pointers.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> v1 -> v2:
>>
>>   - only compile efi_add_known_memory if efi_loader is enabled
>> ---
>>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
> 
> NAK.
> 
> You should not point sandbox pointers into the EFI tables. I know it
> looks like a clever shortcut, but it is not correct. It will mess up
> logging and debugging, since those pointers bear no easily accessible
> relationship to U-Boot address.

Hello Simon,

why do we need this Babylonic confusion of addresses which do not point
to valid memory and pointers that are not valid addresses where
everybody is getting lost?

Simply use only addresses with there mmap'ed values and get rid of all
conversions. Use your board file to adjust kernel_addr_r and the like to
the address range that Linux offered you.

Best regards

Heinrich

> 
> Please start from my v7 patch. I'm happy to help do this correctly.
> But, again, I think it should come after we have basic sandbox EFI
> support applied.
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH v2 06/11] efi_loader: Allow SMBIOS tables in highmem

[Heinrich Schuchardt]

On 06/14/2018 09:13 PM, Alexander Graf wrote:
> 
> 
> On 14.06.18 21:01, Simon Glass wrote:
>> Hi Alex,
>>
>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>> We try hard to make sure that SMBIOS tables live in the lower 32bit.
>>> However, when we can not find any space at all there, we should not
>>> error out but instead just fall back to map them in the full address
>>> space instead.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  lib/efi_loader/efi_smbios.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> Does that actually work? I thought the addresses in the table were
>> always 32-bit?
> 
> 
> There is only a single table reference which indeed is 32bit:
> se->struct_table_address.
> 
> That address however is unused in the EFI case usually, because the
> SMBIOS information is already encapsulated in a table, so there's no
> need to search through address space for a _DMI_ entry:
> 
>   https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5122
> 
> 
> Alex
> 
We still have an open issue with E820 memory reservations not observed
by Linux because they are not mirrored in the memory map. Could the same
happen with smbios?

Regards

Heinrich

[U-Boot] [PATCH v2 06/11] efi_loader: Allow SMBIOS tables in highmem

[Alexander Graf]

On 14.06.18 21:26, Heinrich Schuchardt wrote:
> On 06/14/2018 09:13 PM, Alexander Graf wrote:
>>
>>
>> On 14.06.18 21:01, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>>> We try hard to make sure that SMBIOS tables live in the lower 32bit.
>>>> However, when we can not find any space at all there, we should not
>>>> error out but instead just fall back to map them in the full address
>>>> space instead.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>  lib/efi_loader/efi_smbios.c | 11 +++++++++--
>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> Does that actually work? I thought the addresses in the table were
>>> always 32-bit?
>>
>>
>> There is only a single table reference which indeed is 32bit:
>> se->struct_table_address.
>>
>> That address however is unused in the EFI case usually, because the
>> SMBIOS information is already encapsulated in a table, so there's no
>> need to search through address space for a _DMI_ entry:
>>
>>   https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5122
>>
>>
>> Alex
>>
> We still have an open issue with E820 memory reservations not observed
> by Linux because they are not mirrored in the memory map. Could the same
> happen with smbios?

The SMBIOS tables get allocated from the EFI pool in
lib/efi_loader/efi_smbios.c.

So the e820 patch fell through the cracks?


Alex

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Alexander Graf]

On 14.06.18 21:01, Simon Glass wrote:
> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>> 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>
>>
>> ---
>>
>> v1 -> v2:
>>
>>   - Clarify address vs pointer
>>   - include mapmem.h
>> ---
>>  lib/efi_loader/efi_file.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

I actually think that this patch tackles the problem the wrong way
around. I've cooked up another one that converts fs_read() and
fs_write() to instead take a pointer - which really is what most users
of the API want in the first place:


https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f


Alex

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Simon Glass]

Hi Alex,

On 14 June 2018 at 13:51, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 14.06.18 21:01, Simon Glass wrote:
>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>> 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>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>   - Clarify address vs pointer
>>>   - include mapmem.h
>>> ---
>>>  lib/efi_loader/efi_file.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>
> I actually think that this patch tackles the problem the wrong way
> around. I've cooked up another one that converts fs_read() and
> fs_write() to instead take a pointer - which really is what most users
> of the API want in the first place:
>
>
> https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f

Actually this is a pretty good exposition of why we don't want to use
pointers everywhere. U-Boot uses addresses all over the place. Even a
search for something as specific as "ulong addr" gets over 200
matches. If we go down this path we will end up changing a pretty
fundamental part of U-Boot, and I don't think it is a good idea to do
that. Addresses are easy to understand, can be added/subtracted
without pointer arithmetic, can be easily converted to pointers as
needed, can be 32-bits long on sandbox, etc.

So I think we should step back from the abyss here and stick with the
way sandbox currently deals with addresses. It works well, is pretty
easy to understand, allows debugging which is just as easy on sandbox
as other archs (since they both uses addresses until basically the
final access), the addresses are typically small values that start at
0 which much is easier to read than (e.g.) 00007f1b58c8b008.

Here for example is the output from starting U-Boot with debugging in
board_f.c (something I have turned on a lot when refactoring and
debugging the init sequence):

U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)

DRAM:  Monitor len: 00395AB0
Ram size: 08000000
Ram top: 08000000
Reserving 3670k for U-Boot at: 07c6a000
Reserving 32776k for malloc() at: 05c68000
Reserving 120 Bytes for Board Info at: 05c67f88
Reserving 472 Bytes for Global Data at: 05c67db0
Reserving 4352 Bytes for FDT at: 05c66cb0
Reserving 0x3c8 Bytes for bootstage at: 05c668e8

RAM Configuration:
Bank #0: 0 128 MiB

DRAM:  128 MiB
New Stack Pointer is: 05c668d0
Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8
Relocation Offset is: 07c6a000
Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0


If we use pointers we get things like this:

Reserving 3670k for U-Boot at: 00007f1b58c8b008

I just don't want to deal with 64-bit addresses when debugging things,
and there really is no point. The map_sysmem() function provides a
nice and easy way to cast an address to a pointer, and it works on all
architectures.

Regards,
Simon

[U-Boot] [PATCH v2 07/11] sandbox: Map host memory for efi_loader

[Simon Glass]

Hi Heinrich,

On 14 June 2018 at 13:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/14/2018 09:02 PM, Simon Glass wrote:
>> Hi Alex,
>>
>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>> With efi_loader we do not control payload applications, so we can not
>>> teach them about the difference between virtual and physical addresses.
>>>
>>> Instead, let's just always map host virtual addresses in the efi memory
>>> map. That way we can be sure that all memory allocation functions always
>>> return consumable pointers.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>   - only compile efi_add_known_memory if efi_loader is enabled
>>> ---
>>>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>
>> NAK.
>>
>> You should not point sandbox pointers into the EFI tables. I know it
>> looks like a clever shortcut, but it is not correct. It will mess up
>> logging and debugging, since those pointers bear no easily accessible
>> relationship to U-Boot address.
>
> Hello Simon,
>
> why do we need this Babylonic confusion of addresses which do not point
> to valid memory and pointers that are not valid addresses where
> everybody is getting lost?
>
> Simply use only addresses with there mmap'ed values and get rid of all
> conversions. Use your board file to adjust kernel_addr_r and the like to
> the address range that Linux offered you.

No one is getting lost. It has been working fine for a long time.
Sandbox was introduced almost 7 years ago. It has been a huge benefit
in terms of productivity and testing.

We are not necessarily running on Linux, but in any case, we get ugly
addresses which expose the underlying machine architecture, which is
not relevant for tests, and just introduces confusion. Sandbox is
intended to be pure U-Boot, just like you might boot up on a 32-bit
ARM or x86 machine.

The conversions existing in any case, since we must case between
addresses and pointers. This just makes it explicit in a convenient,
type-safe manner.

Regards,
Simon

[U-Boot] [PATCH v2 07/11] sandbox: Map host memory for efi_loader

[Simon Glass]

Hi Alex,

On 14 June 2018 at 13:15, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 14.06.18 21:02, Simon Glass wrote:
>> Hi Alex,
>>
>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>> With efi_loader we do not control payload applications, so we can not
>>> teach them about the difference between virtual and physical addresses.
>>>
>>> Instead, let's just always map host virtual addresses in the efi memory
>>> map. That way we can be sure that all memory allocation functions always
>>> return consumable pointers.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>   - only compile efi_add_known_memory if efi_loader is enabled
>>> ---
>>>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>
>> NAK.
>>
>> You should not point sandbox pointers into the EFI tables. I know it
>> looks like a clever shortcut, but it is not correct. It will mess up
>> logging and debugging, since those pointers bear no easily accessible
>> relationship to U-Boot address.
>>
>> Please start from my v7 patch. I'm happy to help do this correctly.
>> But, again, I think it should come after we have basic sandbox EFI
>> support applied.
>
> I don't want to play ping pong with you here. NAK on your approach until
> I see it properly executing selftest.

I think you just did :-)

But if you are asking for me to pull together a patch that gets that
far, then OK. I can see that you are not convinced it would work, or
be easy to follow, and I have not proven that yet. I was just hoping
to take things in incremental steps since this has been outstanding
for so long.

>
> So either we drive this forward or we don't. Your choice.

I have long wanted EFI to fall into the sandbox testing framework, so
that e.g. 'make tests' will quickly run the EFI tests. I don't think
we are too far away. It doesn't have to happen immediately, but I
predict that when we get it working, we won't look back. It will be
much more convenient than running a separate app or testing on 'real
hardware' and you can set up quite complicated things with it.

Regards,
Simon

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Alexander Graf]

On 14.06.18 23:35, Simon Glass wrote:
> Hi Alex,
> 
> On 14 June 2018 at 13:51, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 14.06.18 21:01, Simon Glass wrote:
>>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>>> 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>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>
>>>>   - Clarify address vs pointer
>>>>   - include mapmem.h
>>>> ---
>>>>  lib/efi_loader/efi_file.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>
>> I actually think that this patch tackles the problem the wrong way
>> around. I've cooked up another one that converts fs_read() and
>> fs_write() to instead take a pointer - which really is what most users
>> of the API want in the first place:
>>
>>
>> https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f
> 
> Actually this is a pretty good exposition of why we don't want to use
> pointers everywhere. U-Boot uses addresses all over the place. Even a
> search for something as specific as "ulong addr" gets over 200
> matches. If we go down this path we will end up changing a pretty
> fundamental part of U-Boot, and I don't think it is a good idea to do
> that. Addresses are easy to understand, can be added/subtracted
> without pointer arithmetic, can be easily converted to pointers as
> needed, can be 32-bits long on sandbox, etc.

I tend to disagree with this. Most code in U-Boot actually consumes
pointers rather than addresses.

> So I think we should step back from the abyss here and stick with the
> way sandbox currently deals with addresses. It works well, is pretty
> easy to understand, allows debugging which is just as easy on sandbox
> as other archs (since they both uses addresses until basically the
> final access), the addresses are typically small values that start at
> 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
> 
> Here for example is the output from starting U-Boot with debugging in
> board_f.c (something I have turned on a lot when refactoring and
> debugging the init sequence):
> 
> U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
> 
> DRAM:  Monitor len: 00395AB0
> Ram size: 08000000
> Ram top: 08000000
> Reserving 3670k for U-Boot at: 07c6a000
> Reserving 32776k for malloc() at: 05c68000
> Reserving 120 Bytes for Board Info at: 05c67f88
> Reserving 472 Bytes for Global Data at: 05c67db0
> Reserving 4352 Bytes for FDT at: 05c66cb0
> Reserving 0x3c8 Bytes for bootstage at: 05c668e8
> 
> RAM Configuration:
> Bank #0: 0 128 MiB
> 
> DRAM:  128 MiB
> New Stack Pointer is: 05c668d0
> Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8
> Relocation Offset is: 07c6a000
> Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
> 
> 
> If we use pointers we get things like this:
> 
> Reserving 3670k for U-Boot at: 00007f1b58c8b008
> 
> I just don't want to deal with 64-bit addresses when debugging things,
> and there really is no point. The map_sysmem() function provides a
> nice and easy way to cast an address to a pointer, and it works on all
> architectures.

Ok, circling back to square 1. The main issue we're facing here is that
most code in U-Boot does not really understand the difference between
virtual and physical addresses - it just simply assumes a 1:1 map.

With sandbox, we do not have a 1:1 map anymore - any address is
somewhere else than its pointer.

We have a few options:

  1) Deal with pointers at random addresses throughout the code

I can see why you don't want this. I find ASLR generated addresses quite
cumbersome to read as well.

  2) Explicitly map RAM at VA 0x10000000, then do 1:1 map

This would be the best of all worlds still IMHO. That way we will have
easily readable addresses (that are identical in 32bit and 64bit), but
can still do a 1:1 map. The only thing we need to do is to introduce a
linker section at 0x10000000.

  3) Keep converting addresses to pointers

I'm afraid if we follow this path, we will always have arguments on
where the correct boundaries are. If you start to enable debugging in
core dm code and print out pointers to your dm objects, you will get
pointer values today as well. Sooner or later we will always end up with
pointers.


Alex

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Simon Glass]

Hi Alex,

On 14 June 2018 at 15:55, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 14.06.18 23:35, Simon Glass wrote:
>> Hi Alex,
>>
>> On 14 June 2018 at 13:51, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 14.06.18 21:01, Simon Glass wrote:
>>>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>>>> 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>
>>>>>
>>>>> ---
>>>>>
>>>>> v1 -> v2:
>>>>>
>>>>>   - Clarify address vs pointer
>>>>>   - include mapmem.h
>>>>> ---
>>>>>  lib/efi_loader/efi_file.c | 5 ++++-
>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>
>>> I actually think that this patch tackles the problem the wrong way
>>> around. I've cooked up another one that converts fs_read() and
>>> fs_write() to instead take a pointer - which really is what most users
>>> of the API want in the first place:
>>>
>>>
>>> https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f
>>
>> Actually this is a pretty good exposition of why we don't want to use
>> pointers everywhere. U-Boot uses addresses all over the place. Even a
>> search for something as specific as "ulong addr" gets over 200
>> matches. If we go down this path we will end up changing a pretty
>> fundamental part of U-Boot, and I don't think it is a good idea to do
>> that. Addresses are easy to understand, can be added/subtracted
>> without pointer arithmetic, can be easily converted to pointers as
>> needed, can be 32-bits long on sandbox, etc.
>
> I tend to disagree with this. Most code in U-Boot actually consumes
> pointers rather than addresses.

That might be true within individual components, but addresses are the
common currency in U-Boot. See for example bootm, all the
image-handling stuff including FIT, commands, device tree address
properties. Pointers are more often used at the lowest level of code.

>
>> So I think we should step back from the abyss here and stick with the
>> way sandbox currently deals with addresses. It works well, is pretty
>> easy to understand, allows debugging which is just as easy on sandbox
>> as other archs (since they both uses addresses until basically the
>> final access), the addresses are typically small values that start at
>> 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
>>
>> Here for example is the output from starting U-Boot with debugging in
>> board_f.c (something I have turned on a lot when refactoring and
>> debugging the init sequence):
>>
>> U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
>>
>> DRAM:  Monitor len: 00395AB0
>> Ram size: 08000000
>> Ram top: 08000000
>> Reserving 3670k for U-Boot at: 07c6a000
>> Reserving 32776k for malloc() at: 05c68000
>> Reserving 120 Bytes for Board Info at: 05c67f88
>> Reserving 472 Bytes for Global Data at: 05c67db0
>> Reserving 4352 Bytes for FDT at: 05c66cb0
>> Reserving 0x3c8 Bytes for bootstage at: 05c668e8
>>
>> RAM Configuration:
>> Bank #0: 0 128 MiB
>>
>> DRAM:  128 MiB
>> New Stack Pointer is: 05c668d0
>> Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8
>> Relocation Offset is: 07c6a000
>> Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
>>
>>
>> If we use pointers we get things like this:
>>
>> Reserving 3670k for U-Boot at: 00007f1b58c8b008
>>
>> I just don't want to deal with 64-bit addresses when debugging things,
>> and there really is no point. The map_sysmem() function provides a
>> nice and easy way to cast an address to a pointer, and it works on all
>> architectures.
>
> Ok, circling back to square 1. The main issue we're facing here is that
> most code in U-Boot does not really understand the difference between
> virtual and physical addresses - it just simply assumes a 1:1 map.

Traditionally that was true, but over the past 5 years quite a bit of
code has been converted to work correctly with sandbox. Also, almost
all *tested* code supports sandbox.

>
> With sandbox, we do not have a 1:1 map anymore - any address is
> somewhere else than its pointer.
>
> We have a few options:
>
>   1) Deal with pointers at random addresses throughout the code
>
> I can see why you don't want this. I find ASLR generated addresses quite
> cumbersome to read as well.

I'm not sure what this means.

>
>   2) Explicitly map RAM at VA 0x10000000, then do 1:1 map
>
> This would be the best of all worlds still IMHO. That way we will have
> easily readable addresses (that are identical in 32bit and 64bit), but
> can still do a 1:1 map. The only thing we need to do is to introduce a
> linker section at 0x10000000.

Linker section or mmap() is essentially the same thing. The former
will presumably just fail to run (bus error?) if the address is not
available. The latter will select a similar, available address.

>
>   3) Keep converting addresses to pointers
>
> I'm afraid if we follow this path, we will always have arguments on
> where the correct boundaries are. If you start to enable debugging in
> core dm code and print out pointers to your dm objects, you will get
> pointer values today as well. Sooner or later we will always end up with
> pointers.

The addresses should generally be converted just before use. Most APIs
should use addresses rather than pointers, particularly those related
to images, loaded data in memory, hardware peripheral addresses and
booting.

Regards,
Simon

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Alexander Graf]

On 06/20/2018 12:03 AM, Simon Glass wrote:
> Hi Alex,
>
> On 14 June 2018 at 15:55, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 14.06.18 23:35, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 14 June 2018 at 13:51, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 14.06.18 21:01, Simon Glass wrote:
>>>>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>> 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>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> v1 -> v2:
>>>>>>
>>>>>>    - Clarify address vs pointer
>>>>>>    - include mapmem.h
>>>>>> ---
>>>>>>   lib/efi_loader/efi_file.c | 5 ++++-
>>>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>> I actually think that this patch tackles the problem the wrong way
>>>> around. I've cooked up another one that converts fs_read() and
>>>> fs_write() to instead take a pointer - which really is what most users
>>>> of the API want in the first place:
>>>>
>>>>
>>>> https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f
>>> Actually this is a pretty good exposition of why we don't want to use
>>> pointers everywhere. U-Boot uses addresses all over the place. Even a
>>> search for something as specific as "ulong addr" gets over 200
>>> matches. If we go down this path we will end up changing a pretty
>>> fundamental part of U-Boot, and I don't think it is a good idea to do
>>> that. Addresses are easy to understand, can be added/subtracted
>>> without pointer arithmetic, can be easily converted to pointers as
>>> needed, can be 32-bits long on sandbox, etc.
>> I tend to disagree with this. Most code in U-Boot actually consumes
>> pointers rather than addresses.
> That might be true within individual components, but addresses are the
> common currency in U-Boot. See for example bootm, all the
> image-handling stuff including FIT, commands, device tree address
> properties. Pointers are more often used at the lowest level of code.

Yes, but all counterparts of fs_read and APIs on the same level use 
pointers. And please take a few minutes to look at its users. 90% of 
them are using pointers too.

As I mentioned elsewhere already, going from address to pointer is 
something reasonable. Going from address to pointer usually a sign of 
bad design ;). If most users of fs_read() are already using pointers, it 
means that either all of its users are wrong or fs_read() is wrong. I 
think the latter is the case.

>
>>> So I think we should step back from the abyss here and stick with the
>>> way sandbox currently deals with addresses. It works well, is pretty
>>> easy to understand, allows debugging which is just as easy on sandbox
>>> as other archs (since they both uses addresses until basically the
>>> final access), the addresses are typically small values that start at
>>> 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
>>>
>>> Here for example is the output from starting U-Boot with debugging in
>>> board_f.c (something I have turned on a lot when refactoring and
>>> debugging the init sequence):
>>>
>>> U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
>>>
>>> DRAM:  Monitor len: 00395AB0
>>> Ram size: 08000000
>>> Ram top: 08000000
>>> Reserving 3670k for U-Boot at: 07c6a000
>>> Reserving 32776k for malloc() at: 05c68000
>>> Reserving 120 Bytes for Board Info at: 05c67f88
>>> Reserving 472 Bytes for Global Data at: 05c67db0
>>> Reserving 4352 Bytes for FDT at: 05c66cb0
>>> Reserving 0x3c8 Bytes for bootstage at: 05c668e8
>>>
>>> RAM Configuration:
>>> Bank #0: 0 128 MiB
>>>
>>> DRAM:  128 MiB
>>> New Stack Pointer is: 05c668d0
>>> Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8
>>> Relocation Offset is: 07c6a000
>>> Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
>>>
>>>
>>> If we use pointers we get things like this:
>>>
>>> Reserving 3670k for U-Boot at: 00007f1b58c8b008
>>>
>>> I just don't want to deal with 64-bit addresses when debugging things,
>>> and there really is no point. The map_sysmem() function provides a
>>> nice and easy way to cast an address to a pointer, and it works on all
>>> architectures.
>> Ok, circling back to square 1. The main issue we're facing here is that
>> most code in U-Boot does not really understand the difference between
>> virtual and physical addresses - it just simply assumes a 1:1 map.
> Traditionally that was true, but over the past 5 years quite a bit of
> code has been converted to work correctly with sandbox. Also, almost
> all *tested* code supports sandbox.

I think the difference is between infrastructure and device code. Most 
device code is still assuming 1:1 maps, but just doesn't get executed in 
sandbox.

>
>> With sandbox, we do not have a 1:1 map anymore - any address is
>> somewhere else than its pointer.
>>
>> We have a few options:
>>
>>    1) Deal with pointers at random addresses throughout the code
>>
>> I can see why you don't want this. I find ASLR generated addresses quite
>> cumbersome to read as well.
> I'm not sure what this means.

Address Space Layout Randomization. It's what causes these weird looking 
addresses ;).

>
>>    2) Explicitly map RAM at VA 0x10000000, then do 1:1 map
>>
>> This would be the best of all worlds still IMHO. That way we will have
>> easily readable addresses (that are identical in 32bit and 64bit), but
>> can still do a 1:1 map. The only thing we need to do is to introduce a
>> linker section at 0x10000000.
> Linker section or mmap() is essentially the same thing. The former
> will presumably just fail to run (bus error?) if the address is not
> available. The latter will select a similar, available address.

No, they're inherently different things. When a process starts, its 
virtual address space is basically empty (apart from the vdso and a tiny 
chunk of ld). The linker is then responsible to ensure that all 
application defined memory regions are mapped. Only after that happened, 
it will map linked libraries (such as glibc, SDL, whatever) at random 
places in address space.

So with a linker section, we're basically guaranteed to always have 
memory live at the same spot. With mmap() there is a good chance we're 
overlapping with something that happened to get mapped there in between.

>
>>    3) Keep converting addresses to pointers
>>
>> I'm afraid if we follow this path, we will always have arguments on
>> where the correct boundaries are. If you start to enable debugging in
>> core dm code and print out pointers to your dm objects, you will get
>> pointer values today as well. Sooner or later we will always end up with
>> pointers.
> The addresses should generally be converted just before use. Most APIs
> should use addresses rather than pointers, particularly those related
> to images, loaded data in memory, hardware peripheral addresses and
> booting.

I don't think that's a reasonable definition for the API requirements to 
be honest. I think addresses make a lot of sense as long as you don't 
want to touch any data inside. You basically use them like a token. Once 
you've gone past that point - so you've run map_sysmem() on that address 
- we should stay in pointer land IMHO.


Alex

[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Simon Glass]

Hi Alex,

On 20 June 2018 at 03:31, Alexander Graf <agraf@suse.de> wrote:
> On 06/20/2018 12:03 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 June 2018 at 15:55, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 14.06.18 23:35, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 14 June 2018 at 13:51, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>> On 14.06.18 21:01, Simon Glass wrote:
>>>>>>
>>>>>> On 14 June 2018 at 12:22, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>> 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>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> v1 -> v2:
>>>>>>>
>>>>>>>    - Clarify address vs pointer
>>>>>>>    - include mapmem.h
>>>>>>> ---
>>>>>>>   lib/efi_loader/efi_file.c | 5 ++++-
>>>>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>
>>>>> I actually think that this patch tackles the problem the wrong way
>>>>> around. I've cooked up another one that converts fs_read() and
>>>>> fs_write() to instead take a pointer - which really is what most users
>>>>> of the API want in the first place:
>>>>>
>>>>>
>>>>>
>>>>> https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f
>>>>
>>>> Actually this is a pretty good exposition of why we don't want to use
>>>> pointers everywhere. U-Boot uses addresses all over the place. Even a
>>>> search for something as specific as "ulong addr" gets over 200
>>>> matches. If we go down this path we will end up changing a pretty
>>>> fundamental part of U-Boot, and I don't think it is a good idea to do
>>>> that. Addresses are easy to understand, can be added/subtracted
>>>> without pointer arithmetic, can be easily converted to pointers as
>>>> needed, can be 32-bits long on sandbox, etc.
>>>
>>> I tend to disagree with this. Most code in U-Boot actually consumes
>>> pointers rather than addresses.
>>
>> That might be true within individual components, but addresses are the
>> common currency in U-Boot. See for example bootm, all the
>> image-handling stuff including FIT, commands, device tree address
>> properties. Pointers are more often used at the lowest level of code.
>
>
> Yes, but all counterparts of fs_read and APIs on the same level use
> pointers. And please take a few minutes to look at its users. 90% of them
> are using pointers too.

Do you mean the callers of fs_read()? That's not the point. Of course
there will be conversion from addresses to pointers, but I am trying
to keep those at the 'edge', away from the core U-Boot code. Your
patch is basically designed to avoid a pointer conversion (which you
can't support) in the filesystem layer. Next you will be doing this in
networking, display, etc. etc. There is just no need for this change.

>
> As I mentioned elsewhere already, going from address to pointer is something
> reasonable. Going from address to pointer usually a sign of bad design ;).
> If most users of fs_read() are already using pointers, it means that either
> all of its users are wrong or fs_read() is wrong. I think the latter is the
> case.

By that logic you would have U-Boot not use addresses at all, and have
pointers everywhere. It just isn't the way the code base is today.

>
>
>>
>>>> So I think we should step back from the abyss here and stick with the
>>>> way sandbox currently deals with addresses. It works well, is pretty
>>>> easy to understand, allows debugging which is just as easy on sandbox
>>>> as other archs (since they both uses addresses until basically the
>>>> final access), the addresses are typically small values that start at
>>>> 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
>>>>
>>>> Here for example is the output from starting U-Boot with debugging in
>>>> board_f.c (something I have turned on a lot when refactoring and
>>>> debugging the init sequence):
>>>>
>>>> U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04
>>>> -0600)
>>>>
>>>> DRAM:  Monitor len: 00395AB0
>>>> Ram size: 08000000
>>>> Ram top: 08000000
>>>> Reserving 3670k for U-Boot at: 07c6a000
>>>> Reserving 32776k for malloc() at: 05c68000
>>>> Reserving 120 Bytes for Board Info at: 05c67f88
>>>> Reserving 472 Bytes for Global Data at: 05c67db0
>>>> Reserving 4352 Bytes for FDT at: 05c66cb0
>>>> Reserving 0x3c8 Bytes for bootstage at: 05c668e8
>>>>
>>>> RAM Configuration:
>>>> Bank #0: 0 128 MiB
>>>>
>>>> DRAM:  128 MiB
>>>> New Stack Pointer is: 05c668d0
>>>> Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8
>>>> Relocation Offset is: 07c6a000
>>>> Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
>>>>
>>>>
>>>> If we use pointers we get things like this:
>>>>
>>>> Reserving 3670k for U-Boot at: 00007f1b58c8b008
>>>>
>>>> I just don't want to deal with 64-bit addresses when debugging things,
>>>> and there really is no point. The map_sysmem() function provides a
>>>> nice and easy way to cast an address to a pointer, and it works on all
>>>> architectures.
>>>
>>> Ok, circling back to square 1. The main issue we're facing here is that
>>> most code in U-Boot does not really understand the difference between
>>> virtual and physical addresses - it just simply assumes a 1:1 map.
>>
>> Traditionally that was true, but over the past 5 years quite a bit of
>> code has been converted to work correctly with sandbox. Also, almost
>> all *tested* code supports sandbox.
>
>
> I think the difference is between infrastructure and device code. Most
> device code is still assuming 1:1 maps, but just doesn't get executed in
> sandbox.

If you mean the drivers, then yes I agree. The drivers typically get
an address and map it to a pointer which they use from then on.

I do have some ideas about executing driver code in sandbox. We
already build a lot of drivers in sandbox, but execute only a very
few. But sandbox could absolutely provide a way to test driver code,
with suitable plumbing. Of course, we need address mapping...:-)

>
>>
>>> With sandbox, we do not have a 1:1 map anymore - any address is
>>> somewhere else than its pointer.
>>>
>>> We have a few options:
>>>
>>>    1) Deal with pointers at random addresses throughout the code
>>>
>>> I can see why you don't want this. I find ASLR generated addresses quite
>>> cumbersome to read as well.
>>
>> I'm not sure what this means.
>
>
> Address Space Layout Randomization. It's what causes these weird looking
> addresses ;).

No, I mean, I am not sure what your comment means. I know what ASLR
is, but I just don't understand your comment as a whole. What problem
are you trying to solve?

>
>>
>>>    2) Explicitly map RAM at VA 0x10000000, then do 1:1 map
>>>
>>> This would be the best of all worlds still IMHO. That way we will have
>>> easily readable addresses (that are identical in 32bit and 64bit), but
>>> can still do a 1:1 map. The only thing we need to do is to introduce a
>>> linker section at 0x10000000.
>>
>> Linker section or mmap() is essentially the same thing. The former
>> will presumably just fail to run (bus error?) if the address is not
>> available. The latter will select a similar, available address.
>
>
> No, they're inherently different things. When a process starts, its virtual
> address space is basically empty (apart from the vdso and a tiny chunk of
> ld). The linker is then responsible to ensure that all application defined
> memory regions are mapped. Only after that happened, it will map linked
> libraries (such as glibc, SDL, whatever) at random places in address space.
>
> So with a linker section, we're basically guaranteed to always have memory
> live at the same spot. With mmap() there is a good chance we're overlapping
> with something that happened to get mapped there in between.

I don't think so. The only calls to mmap() in U-Boot are in os.c so we
can control use of this <4GB address. From my limited testing with
64-bit machines I always get a huge address for things where I don't
specify a hint address.

>
>>
>>>    3) Keep converting addresses to pointers
>>>
>>> I'm afraid if we follow this path, we will always have arguments on
>>> where the correct boundaries are. If you start to enable debugging in
>>> core dm code and print out pointers to your dm objects, you will get
>>> pointer values today as well. Sooner or later we will always end up with
>>> pointers.
>>
>> The addresses should generally be converted just before use. Most APIs
>> should use addresses rather than pointers, particularly those related
>> to images, loaded data in memory, hardware peripheral addresses and
>> booting.
>
>
> I don't think that's a reasonable definition for the API requirements to be
> honest. I think addresses make a lot of sense as long as you don't want to
> touch any data inside. You basically use them like a token. Once you've gone
> past that point - so you've run map_sysmem() on that address - we should
> stay in pointer land IMHO.

Again, by that logic we would never uses addresses in U-Boot.
Everything would be a pointer. Addresses have a number of advantages
over pointers:

- easy to do arithmetic
- don't need casts
- easy to printf() without getting massive 16-digit hex strings all the time
- can be 32-bits wide in sandbox even on a 64-bit machine

Probably some others...you yourself said you cannot do pointer
arithmetic without casting to a uintptr_t first. I don't think that is
true, but it is indicative of the messiness of dealing with pointers.

Regards,
Simon