[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[Alexander Graf]

This patch set augments Simon's patch set for efi_loader support
in sandbox[1], but cuts off the memory allocation scheme at a different
point.

According to the UEFI spec, efi_allocate_pages() takes a uint64_t *
argument. Via this argument, we get a physical address as input, but
emit a pointer as output.

With this patch set in place, I can successfully run the selftest suite
as well as an aarch64 grub.efi binary. X86_64 grub.efi doesn't work
because that one requires inl instructions to work.

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

v2 -> v3:

  - removed: efi_loader: Pass address to fs_read()
  - new: fs: Convert fs_read/write to take buffer instead of address
  - new: efi_loader: Introduce ms abi vararg helpers
  - new: sandbox: Enable 1:1 map
  - new: distro: Move to compiler based target architecture determination
  - new: efi_loader: Move to compiler based target architecture determination
  - new: sandbox: Allow to execute from RAM
  - new: sandbox: Fix setjmp/longjmp

v3 -> v4:

  - remove 1:1 map again
  - switch to U-Boot addresses exposed in memory tables
  - new: elf: Move x86 reloc defines to common elf.h
  - new: sandbox: Always allocate aligned buffers
  - new: efi_loader: Expose U-Boot addresses in memory map for sandbox

Alexander Graf (16):
  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
  fs: Convert fs_read/write to take buffer instead of address
  efi_loader: Introduce ms abi vararg helpers
  distro: Move to compiler based target architecture determination
  efi_loader: Move to compiler based target architecture determination
  sandbox: Fix setjmp/longjmp
  elf: Move x86 reloc defines to common elf.h
  efi_loader: Use common elf.h reloc defines
  sandbox: Allow to execute from RAM
  sandbox: Always allocate aligned buffers
  efi_loader: Expose U-Boot addresses in memory map for sandbox

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

Simon Glass (4):
  efi: sandbox: Add distroboot support
  efi: sandbox: Add relocation constants
  efi: sandbox: Enable EFI loader for sandbox
  efi: sandbox: Adjust memory usage for sandbox

 arch/sandbox/cpu/cpu.c                | 20 ++++++++++++----
 arch/sandbox/cpu/os.c                 | 39 +++++++++++++++---------------
 arch/sandbox/include/asm/setjmp.h     |  4 +++-
 arch/x86/include/asm/elf.h            | 45 -----------------------------------
 arch/x86/lib/reloc_ia32_efi.c         |  1 -
 arch/x86/lib/reloc_x86_64_efi.c       |  1 -
 board/BuR/common/common.c             |  2 +-
 board/gdsys/p1022/controlcenterd-id.c | 10 ++++----
 cmd/bootefi.c                         | 13 ++++++----
 cmd/mvebu/bubt.c                      |  4 ++--
 common/splash_source.c                |  4 +++-
 drivers/bootcount/bootcount_ext.c     | 12 +++++-----
 drivers/fpga/zynqpl.c                 |  8 ++++---
 fs/fs.c                               | 20 ++++++++--------
 include/config_distro_bootcmd.h       | 17 ++++++++-----
 include/efi.h                         | 25 ++++++++++---------
 include/elf.h                         | 35 +++++++++++++++++++++++++++
 include/fs.h                          | 12 +++++-----
 include/os.h                          | 19 +++++++++++++++
 lib/efi/Makefile                      |  4 ++--
 lib/efi_loader/Kconfig                |  2 +-
 lib/efi_loader/efi_boottime.c         | 36 ++++++++++++++--------------
 lib/efi_loader/efi_file.c             |  6 ++---
 lib/efi_loader/efi_image_loader.c     | 12 +++++-----
 lib/efi_loader/efi_memory.c           | 28 ++++++++++++++--------
 lib/efi_loader/efi_runtime.c          | 21 ++++++++--------
 lib/efi_loader/efi_smbios.c           | 11 +++++++--
 lib/efi_selftest/Makefile             |  2 +-
 28 files changed, 228 insertions(+), 185 deletions(-)
 delete mode 100644 arch/x86/include/asm/elf.h

-- 
2.12.3

[U-Boot] [PATCH v4 01/21] 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 v4 02/21] 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 v4 03/21] 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] [U-Boot, v4, 03/21] 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>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 04/21] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 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] [U-Boot, v4, 04/21] 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 05/21] 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 v4 06/21] 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 v4 06/21] efi_loader: Allow SMBIOS tables in highmem

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, 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.

Does this actually happen?

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

Regards,
Simon

[U-Boot] [PATCH v4 06/21] efi_loader: Allow SMBIOS tables in highmem

[Alexander Graf]

On 06/21/2018 04:01 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:23, 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.
> Does this actually happen?

No, but it can. It really doesn't need to be part of this patch set either.

The main target where things like this can happen would be systems like 
an AMD Seattle, which does not have any RAM mapped <4GB.


Alex

[U-Boot] [PATCH v4 06/21] efi_loader: Allow SMBIOS tables in highmem

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:38, Alexander Graf <agraf@suse.de> wrote:
>
> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:23, 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.
>>
>> Does this actually happen?
>
>
> No, but it can. It really doesn't need to be part of this patch set either.
>
> The main target where things like this can happen would be systems like an AMD Seattle, which does not have any RAM mapped <4GB.

Ah OK. Then I think you should add that info to the commit message so
it is clear the problem we are solving.

Regards,
Simon

[U-Boot] [U-Boot, v4, 06/21] 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>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 07/21] 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

v3 -> v4:

  - don't compile efi mapping code in for spl
---
 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..29dac8dfda 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)
 		;
 }
+
+#if CONFIG_IS_ENABLED(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 v4 07/21] sandbox: Map host memory for efi_loader

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, 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
>
> v3 -> v4:
>
>   - don't compile efi mapping code in for spl
> ---
>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

I don't want to use this patch. As mentioned EFI memory allocation
should use addresses, and not internal sandbox pointers.

Regards,
Simon

[U-Boot] [PATCH v4 08/21] 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] [U-Boot, v4, 08/21] 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>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 09/21] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 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] [U-Boot, v4, 09/21] 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 10/21] fs: Convert fs_read/write to take buffer instead of address

[Alexander Graf]

The fs_read() and fs_write() functions are internal interfaces that
naturally want to get pointers as arguments. Most users so far even
have pointers and explicitly cast them into integers just to be able
to pass them into the function.

Convert them over to instead take a pointer argument for the buffer.
That way any sandbox mapping gets greatly simplified and users of
the API intuitively know what to do.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 board/BuR/common/common.c             |  2 +-
 board/gdsys/p1022/controlcenterd-id.c | 10 +++++-----
 cmd/mvebu/bubt.c                      |  4 ++--
 common/splash_source.c                |  4 +++-
 drivers/bootcount/bootcount_ext.c     | 12 ++++++------
 drivers/fpga/zynqpl.c                 |  8 +++++---
 fs/fs.c                               | 20 ++++++++++----------
 include/fs.h                          | 12 ++++++------
 lib/efi_loader/efi_file.c             |  6 ++----
 9 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/board/BuR/common/common.c b/board/BuR/common/common.c
index 9df19791c2..ab9d9c51cf 100644
--- a/board/BuR/common/common.c
+++ b/board/BuR/common/common.c
@@ -269,7 +269,7 @@ static int load_devicetree(void)
 		puts("load_devicetree: set_blk_dev failed.\n");
 		return -1;
 	}
-	rc = fs_read(dtbname, (u32)dtbaddr, 0, 0, &dtbsize);
+	rc = fs_read(dtbname, (u_char *)dtbaddr, 0, 0, &dtbsize);
 #endif
 	if (rc == 0) {
 		gd->fdt_blob = (void *)dtbaddr;
diff --git a/board/gdsys/p1022/controlcenterd-id.c b/board/gdsys/p1022/controlcenterd-id.c
index 7e082dff05..2f01f7b7eb 100644
--- a/board/gdsys/p1022/controlcenterd-id.c
+++ b/board/gdsys/p1022/controlcenterd-id.c
@@ -874,7 +874,7 @@ static struct key_program *load_key_chunk(const char *ifname,
 
 	if (fs_set_blk_dev(ifname, dev_part_str, fs_type))
 		goto failure;
-	if (fs_read(path, (ulong)buf, 0, 12, &i) < 0)
+	if (fs_read(path, buf, 0, 12, &i) < 0)
 		goto failure;
 	if (i < 12)
 		goto failure;
@@ -890,7 +890,7 @@ static struct key_program *load_key_chunk(const char *ifname,
 		goto failure;
 	if (fs_set_blk_dev(ifname, dev_part_str, fs_type))
 		goto failure;
-	if (fs_read(path, (ulong)result, 0,
+	if (fs_read(path, result, 0,
 		    sizeof(struct key_program) + header.code_size, &i) < 0)
 		goto failure;
 	if (i <= 0)
@@ -1019,7 +1019,7 @@ static int second_stage_init(void)
 	struct key_program *hmac_blob = NULL;
 	const char *image_path = "/ccdm.itb";
 	char *mac_path = NULL;
-	ulong image_addr;
+	u8 *image_addr;
 	loff_t image_size;
 	uint32_t err;
 
@@ -1059,7 +1059,7 @@ static int second_stage_init(void)
 	strcat(mac_path, mac_suffix);
 
 	/* read image from mmcdev (ccdm.itb) */
-	image_addr = (ulong)get_image_location();
+	image_addr = get_image_location();
 	if (fs_set_blk_dev("mmc", mmcdev, FS_TYPE_EXT))
 		goto failure;
 	if (fs_read(image_path, image_addr, 0, 0, &image_size) < 0)
@@ -1077,7 +1077,7 @@ static int second_stage_init(void)
 		puts("corrupted mac file\n");
 		goto failure;
 	}
-	if (check_hmac(hmac_blob, (u8 *)image_addr, image_size)) {
+	if (check_hmac(hmac_blob, image_addr, image_size)) {
 		puts("image integrity could not be verified\n");
 		goto failure;
 	}
diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index b4d371f305..29fff898fa 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -209,7 +209,7 @@ static size_t mmc_read_file(const char *file_name)
 	}
 
 	/* Perfrom file read */
-	rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
+	rc = fs_read(file_name, (void *)get_load_addr(), 0, 0, &act_read);
 	if (rc)
 		return 0;
 
@@ -392,7 +392,7 @@ static size_t usb_read_file(const char *file_name)
 	}
 
 	/* Perfrom file read */
-	rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
+	rc = fs_read(file_name, (void *)get_load_addr(), 0, 0, &act_read);
 	if (rc)
 		return 0;
 
diff --git a/common/splash_source.c b/common/splash_source.c
index 62763b9ebd..79dbea12fc 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -11,6 +11,7 @@
 #include <fs.h>
 #include <fdt_support.h>
 #include <image.h>
+#include <mapmem.h>
 #include <nand.h>
 #include <sata.h>
 #include <spi.h>
@@ -252,7 +253,8 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
 	}
 
 	splash_select_fs_dev(location);
-	res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
+	res = fs_read(splash_file, map_sysmem(bmp_load_addr, bmp_size),
+		      0, 0, &actread);
 
 out:
 	if (location->ubivol != NULL)
diff --git a/drivers/bootcount/bootcount_ext.c b/drivers/bootcount/bootcount_ext.c
index 075e590896..4a46f17c15 100644
--- a/drivers/bootcount/bootcount_ext.c
+++ b/drivers/bootcount/bootcount_ext.c
@@ -24,10 +24,10 @@ void bootcount_store(ulong a)
 	buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
 	buf[0] = BC_MAGIC;
 	buf[1] = (a & 0xff);
-	unmap_sysmem(buf);
 
-	ret = fs_write(CONFIG_SYS_BOOTCOUNT_EXT_NAME,
-		       CONFIG_SYS_BOOTCOUNT_ADDR, 0, 2, &len);
+	ret = fs_write(CONFIG_SYS_BOOTCOUNT_EXT_NAME, buf, 0, 2, &len);
+
+	unmap_sysmem(buf);
 	if (ret != 0)
 		puts("Error storing bootcount\n");
 }
@@ -44,14 +44,14 @@ ulong bootcount_load(void)
 		return 0;
 	}
 
-	ret = fs_read(CONFIG_SYS_BOOTCOUNT_EXT_NAME, CONFIG_SYS_BOOTCOUNT_ADDR,
-		      0, 2, &len_read);
+	buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
+
+	ret = fs_read(CONFIG_SYS_BOOTCOUNT_EXT_NAME, buf, 0, 2, &len_read);
 	if (ret != 0 || len_read != 2) {
 		puts("Error loading bootcount\n");
 		return 0;
 	}
 
-	buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
 	if (buf[0] == BC_MAGIC)
 		ret = buf[1];
 
diff --git a/drivers/fpga/zynqpl.c b/drivers/fpga/zynqpl.c
index fd37d18c7f..2fba77d45f 100644
--- a/drivers/fpga/zynqpl.c
+++ b/drivers/fpga/zynqpl.c
@@ -431,7 +431,7 @@ static int zynq_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
 	if (fs_set_blk_dev(interface, dev_part, fstype))
 		return FPGA_FAIL;
 
-	if (fs_read(filename, (u32) buf, pos, blocksize, &actread) < 0)
+	if (fs_read(filename, (void *)buf, pos, blocksize, &actread) < 0)
 		return FPGA_FAIL;
 
 	if (zynq_validate_bitstream(desc, buf, bsize, blocksize, &swap,
@@ -454,10 +454,12 @@ static int zynq_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
 			return FPGA_FAIL;
 
 		if (bsize > blocksize) {
-			if (fs_read(filename, (u32) buf, pos, blocksize, &actread) < 0)
+			if (fs_read(filename, (void *)buf, pos, blocksize,
+				    &actread) < 0)
 				return FPGA_FAIL;
 		} else {
-			if (fs_read(filename, (u32) buf, pos, bsize, &actread) < 0)
+			if (fs_read(filename, (void *)buf, pos, bsize,
+				    &actread) < 0)
 				return FPGA_FAIL;
 		}
 	} while (bsize > blocksize);
diff --git a/fs/fs.c b/fs/fs.c
index 33808d549e..27ce9259d2 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -402,20 +402,17 @@ int fs_size(const char *filename, loff_t *size)
 	return ret;
 }
 
-int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
+int fs_read(const char *filename, void *buf, loff_t offset, loff_t len,
 	    loff_t *actread)
 {
 	struct fstype_info *info = fs_get_info(fs_type);
-	void *buf;
 	int ret;
 
 	/*
 	 * We don't actually know how many bytes are being read, since len==0
 	 * means read the whole file.
 	 */
-	buf = map_sysmem(addr, len);
 	ret = info->read(filename, buf, offset, len, actread);
-	unmap_sysmem(buf);
 
 	/* If we requested a specific number of bytes, check we got it */
 	if (ret == 0 && len && *actread != len)
@@ -425,16 +422,13 @@ int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
 	return ret;
 }
 
-int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
+int fs_write(const char *filename, void *buf, loff_t offset, loff_t len,
 	     loff_t *actwrite)
 {
 	struct fstype_info *info = fs_get_info(fs_type);
-	void *buf;
 	int ret;
 
-	buf = map_sysmem(addr, len);
 	ret = info->write(filename, buf, offset, len, actwrite);
-	unmap_sysmem(buf);
 
 	if (ret < 0 && len != *actwrite) {
 		printf("** Unable to write file %s **\n", filename);
@@ -529,6 +523,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	int ret;
 	unsigned long time;
 	char *ep;
+	void *buf;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -567,9 +562,11 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	else
 		pos = 0;
 
+	buf = map_sysmem(addr, bytes);
 	time = get_timer(0);
-	ret = fs_read(filename, addr, pos, bytes, &len_read);
+	ret = fs_read(filename, buf, pos, bytes, &len_read);
 	time = get_timer(time);
+	unmap_sysmem(buf);
 	if (ret < 0)
 		return 1;
 
@@ -623,6 +620,7 @@ int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	loff_t len;
 	int ret;
 	unsigned long time;
+	void *buf;
 
 	if (argc < 6 || argc > 7)
 		return CMD_RET_USAGE;
@@ -638,9 +636,11 @@ int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	else
 		pos = 0;
 
+	buf = map_sysmem(addr, bytes);
 	time = get_timer(0);
-	ret = fs_write(filename, addr, pos, bytes, &len);
+	ret = fs_write(filename, buf, pos, bytes, &len);
 	time = get_timer(time);
+	unmap_sysmem(buf);
 	if (ret < 0)
 		return 1;
 
diff --git a/include/fs.h b/include/fs.h
index 163da103b4..647b0c2ed2 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -76,27 +76,27 @@ int fs_size(const char *filename, loff_t *size);
  * Note that not all filesystem types support either/both offset!=0 or len!=0.
  *
  * @filename: Name of file to read from
- * @addr: The address to read into
+ * @buf: The buffer to read into
  * @offset: The offset in file to read from
  * @len: The number of bytes to read. Maybe 0 to read entire file
  * @actread: Returns the actual number of bytes read
  * @return 0 if ok with valid *actread, -1 on error conditions
  */
-int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
+int fs_read(const char *filename, void *buf, loff_t offset, loff_t len,
 	    loff_t *actread);
 
 /*
  * fs_write - Write file to the partition previously set by fs_set_blk_dev()
  * Note that not all filesystem types support offset!=0.
  *
- * @filename: Name of file to read from
- * @addr: The address to read into
- * @offset: The offset in file to read from. Maybe 0 to write to start of file
+ * @filename: Name of file to write to
+ * @buf: The buffer to read from
+ * @offset: The offset in file to write to. Maybe 0 to write to start of file
  * @len: The number of bytes to write
  * @actwrite: Returns the actual number of bytes written
  * @return 0 if ok with valid *actwrite, -1 on error conditions
  */
-int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
+int fs_write(const char *filename, void *buf, loff_t offset, loff_t len,
 	     loff_t *actwrite);
 
 /*
diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index e6a15bcb52..7bd061f395 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -233,8 +233,7 @@ static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
 {
 	loff_t actread;
 
-	if (fs_read(fh->path, (ulong)buffer, fh->offset,
-		    *buffer_size, &actread))
+	if (fs_read(fh->path, buffer, fh->offset, *buffer_size, &actread))
 		return EFI_DEVICE_ERROR;
 
 	*buffer_size = actread;
@@ -363,8 +362,7 @@ static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file,
 		goto error;
 	}
 
-	if (fs_write(fh->path, (ulong)buffer, fh->offset, *buffer_size,
-		     &actwrite)) {
+	if (fs_write(fh->path, buffer, fh->offset, *buffer_size, &actwrite)) {
 		ret = EFI_DEVICE_ERROR;
 		goto error;
 	}
-- 
2.12.3

[U-Boot] [PATCH v4 10/21] fs: Convert fs_read/write to take buffer instead of address

[Simon Glass]

kOn 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> The fs_read() and fs_write() functions are internal interfaces that
> naturally want to get pointers as arguments. Most users so far even
> have pointers and explicitly cast them into integers just to be able
> to pass them into the function.
>
> Convert them over to instead take a pointer argument for the buffer.
> That way any sandbox mapping gets greatly simplified and users of
> the API intuitively know what to do.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  board/BuR/common/common.c             |  2 +-
>  board/gdsys/p1022/controlcenterd-id.c | 10 +++++-----
>  cmd/mvebu/bubt.c                      |  4 ++--
>  common/splash_source.c                |  4 +++-
>  drivers/bootcount/bootcount_ext.c     | 12 ++++++------
>  drivers/fpga/zynqpl.c                 |  8 +++++---
>  fs/fs.c                               | 20 ++++++++++----------
>  include/fs.h                          | 12 ++++++------
>  lib/efi_loader/efi_file.c             |  6 ++----
>  9 files changed, 40 insertions(+), 38 deletions(-)

As mentioned before, we should not change this API. There is no need -
U-Boot uses addresses, and this just expands the scope of the sandbox
private address.

Regards,
Simon

[U-Boot] [PATCH v4 10/21] fs: Convert fs_read/write to take buffer instead of address

[Tom Rini]

On Wed, Jun 20, 2018 at 08:02:09PM -0600, Simon Glass wrote:
> kOn 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> > The fs_read() and fs_write() functions are internal interfaces that
> > naturally want to get pointers as arguments. Most users so far even
> > have pointers and explicitly cast them into integers just to be able
> > to pass them into the function.
> >
> > Convert them over to instead take a pointer argument for the buffer.
> > That way any sandbox mapping gets greatly simplified and users of
> > the API intuitively know what to do.
> >
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> > ---
> >  board/BuR/common/common.c             |  2 +-
> >  board/gdsys/p1022/controlcenterd-id.c | 10 +++++-----
> >  cmd/mvebu/bubt.c                      |  4 ++--
> >  common/splash_source.c                |  4 +++-
> >  drivers/bootcount/bootcount_ext.c     | 12 ++++++------
> >  drivers/fpga/zynqpl.c                 |  8 +++++---
> >  fs/fs.c                               | 20 ++++++++++----------
> >  include/fs.h                          | 12 ++++++------
> >  lib/efi_loader/efi_file.c             |  6 ++----
> >  9 files changed, 40 insertions(+), 38 deletions(-)
> 
> As mentioned before, we should not change this API. There is no need -
> U-Boot uses addresses, and this just expands the scope of the sandbox
> private address.

What we have, in either case, is imperfect.  We're basically hiding some
funky details for sandbox support inside of fs_read/fs_write.  That with
this patch we're moving more of the sandbox wrappers around is something
I don't see as a positive change.  We also don't drop all casts in the
callers.  Some go away, some get changed, at least one gets added.  In
the end, I don't think this patch is a win.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180621/088ac31e/attachment.sig>

[U-Boot] [PATCH v4 11/21] efi_loader: Introduce ms abi vararg helpers

[Alexander Graf]

Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
abi though, so we also need to make sure we use x86_64 varargs helpers.

This patch introduces generic efi vararg helpers that adhere to the
respective EFI ABI. That way we can deal with them properly from efi
loader code and properly interpret variable arguments.

This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
on x86_64 for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/efi.h                 |  8 ++++++++
 lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index 826d484977..7be7798d8d 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -22,8 +22,16 @@
 /* EFI on x86_64 uses the Microsoft ABI which is not the default for GCC */
 #ifdef __x86_64__
 #define EFIAPI __attribute__((ms_abi))
+#define efi_va_list __builtin_ms_va_list
+#define efi_va_start __builtin_ms_va_start
+#define efi_va_arg __builtin_va_arg
+#define efi_va_end __builtin_ms_va_end
 #else
 #define EFIAPI asmlinkage
+#define efi_va_list va_list
+#define efi_va_start va_start
+#define efi_va_arg va_arg
+#define efi_va_end va_end
 #endif /* __x86_64__ */
 
 struct efi_device_path;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 50d311548e..404743fe01 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2273,7 +2273,7 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 {
 	EFI_ENTRY("%p", handle);
 
-	va_list argptr;
+	efi_va_list argptr;
 	const efi_guid_t *protocol;
 	void *protocol_interface;
 	efi_status_t r = EFI_SUCCESS;
@@ -2282,12 +2282,12 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 	if (!handle)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (;;) {
-		protocol = va_arg(argptr, efi_guid_t*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
 		if (!protocol)
 			break;
-		protocol_interface = va_arg(argptr, void*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		r = EFI_CALL(efi_install_protocol_interface(
 						handle, protocol,
 						EFI_NATIVE_INTERFACE,
@@ -2296,19 +2296,19 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 			break;
 		i++;
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 	if (r == EFI_SUCCESS)
 		return EFI_EXIT(r);
 
 	/* If an error occurred undo all changes. */
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (; i; --i) {
-		protocol = va_arg(argptr, efi_guid_t*);
-		protocol_interface = va_arg(argptr, void*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		EFI_CALL(efi_uninstall_protocol_interface(handle, protocol,
 							  protocol_interface));
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 
 	return EFI_EXIT(r);
 }
@@ -2332,7 +2332,7 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
 {
 	EFI_ENTRY("%p", handle);
 
-	va_list argptr;
+	efi_va_list argptr;
 	const efi_guid_t *protocol;
 	void *protocol_interface;
 	efi_status_t r = EFI_SUCCESS;
@@ -2341,12 +2341,12 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
 	if (!handle)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (;;) {
-		protocol = va_arg(argptr, efi_guid_t*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
 		if (!protocol)
 			break;
-		protocol_interface = va_arg(argptr, void*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		r = EFI_CALL(efi_uninstall_protocol_interface(
 						handle, protocol,
 						protocol_interface));
@@ -2354,20 +2354,20 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
 			break;
 		i++;
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 	if (r == EFI_SUCCESS)
 		return EFI_EXIT(r);
 
 	/* If an error occurred undo all changes. */
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (; i; --i) {
-		protocol = va_arg(argptr, efi_guid_t*);
-		protocol_interface = va_arg(argptr, void*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		EFI_CALL(efi_install_protocol_interface(&handle, protocol,
 							EFI_NATIVE_INTERFACE,
 							protocol_interface));
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 
 	return EFI_EXIT(r);
 }
-- 
2.12.3

[U-Boot] [PATCH v4 11/21] efi_loader: Introduce ms abi vararg helpers

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
> abi though, so we also need to make sure we use x86_64 varargs helpers.
>
> This patch introduces generic efi vararg helpers that adhere to the
> respective EFI ABI. That way we can deal with them properly from efi
> loader code and properly interpret variable arguments.
>
> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
> on x86_64 for me.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  include/efi.h                 |  8 ++++++++
>  lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------
>  2 files changed, 26 insertions(+), 18 deletions(-)

I thought this was a bug in gcc. Should we include this workaround
only for older gcc versions?

Regards,
Simon

[U-Boot] [PATCH v4 11/21] efi_loader: Introduce ms abi vararg helpers

[Alexander Graf]

On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>
>> This patch introduces generic efi vararg helpers that adhere to the
>> respective EFI ABI. That way we can deal with them properly from efi
>> loader code and properly interpret variable arguments.
>>
>> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
>> on x86_64 for me.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   include/efi.h                 |  8 ++++++++
>>   lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------
>>   2 files changed, 26 insertions(+), 18 deletions(-)
> I thought this was a bug in gcc. Should we include this workaround
> only for older gcc versions?

The bug is something different - it's about using unannotated helper 
functions on an ms_abi declared vararg list. This patch really just 
implements expected behavior regardless of that bug.


Alex

[U-Boot] [U-Boot, v4, 11/21] efi_loader: Introduce ms abi vararg helpers

[Alexander Graf]

> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
> abi though, so we also need to make sure we use x86_64 varargs helpers.
> 
> This patch introduces generic efi vararg helpers that adhere to the
> respective EFI ABI. That way we can deal with them properly from efi
> loader code and properly interpret variable arguments.
> 
> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
> on x86_64 for me.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [U-Boot, v4, 11/21] efi_loader: Introduce ms abi vararg helpers

[Bin Meng]

Hi Alex,

On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>
>> This patch introduces generic efi vararg helpers that adhere to the
>> respective EFI ABI. That way we can deal with them properly from efi
>> loader code and properly interpret variable arguments.
>>
>> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
>> on x86_64 for me.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> Thanks, applied to efi-next
>

I applied this patch on my x86 tree and tested there, but
qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
am missing?

Regards,
Bin

[U-Boot] [U-Boot, v4, 11/21] efi_loader: Introduce ms abi vararg helpers

[Alexander Graf]

On 06/23/2018 10:37 AM, Bin Meng wrote:
> Hi Alex,
>
> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
>>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>>
>>> This patch introduces generic efi vararg helpers that adhere to the
>>> respective EFI ABI. That way we can deal with them properly from efi
>>> loader code and properly interpret variable arguments.
>>>
>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
>>> on x86_64 for me.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Thanks, applied to efi-next
>>
> I applied this patch on my x86 tree and tested there, but
> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
> am missing?

Where does it fail? There is basically this pitfall and setjmp/longjmp 
that can easily go wrong.


Alex

[U-Boot] [U-Boot, v4, 11/21] efi_loader: Introduce ms abi vararg helpers

[Bin Meng]

Hi Alex,

On Tue, Jun 26, 2018 at 12:47 AM, Alexander Graf <agraf@suse.de> wrote:
> On 06/23/2018 10:37 AM, Bin Meng wrote:
>>
>> Hi Alex,
>>
>> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the
>>>> ms
>>>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>>>
>>>> This patch introduces generic efi vararg helpers that adhere to the
>>>> respective EFI ABI. That way we can deal with them properly from efi
>>>> loader code and properly interpret variable arguments.
>>>>
>>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi
>>>> selftests
>>>> on x86_64 for me.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> Thanks, applied to efi-next
>>>
>> I applied this patch on my x86 tree and tested there, but
>> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
>> am missing?
>
>
> Where does it fail? There is basically this pitfall and setjmp/longjmp that
> can easily go wrong.

It just reboots without printing any error message. You can reproduce
this in the latest u-boot/master.

Regards,
Bin

[U-Boot] [U-Boot, v4, 11/21] efi_loader: Introduce ms abi vararg helpers

[Alexander Graf]

On 06/26/2018 03:51 AM, Bin Meng wrote:
> Hi Alex,
>
> On Tue, Jun 26, 2018 at 12:47 AM, Alexander Graf <agraf@suse.de> wrote:
>> On 06/23/2018 10:37 AM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the
>>>>> ms
>>>>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>>>>
>>>>> This patch introduces generic efi vararg helpers that adhere to the
>>>>> respective EFI ABI. That way we can deal with them properly from efi
>>>>> loader code and properly interpret variable arguments.
>>>>>
>>>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi
>>>>> selftests
>>>>> on x86_64 for me.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Thanks, applied to efi-next
>>>>
>>> I applied this patch on my x86 tree and tested there, but
>>> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
>>> am missing?
>>
>> Where does it fail? There is basically this pitfall and setjmp/longjmp that
>> can easily go wrong.
> It just reboots without printing any error message. You can reproduce
> this in the latest u-boot/master.

How do you run this? I tried the qemu x86_64 target, but it complains 
about a missing serial driver. I guess I need to pass some DT in?

Alex

$ qemu-system-x86_64 -nographic -pflash u-boot.rom -m 1G
WARNING: Image format was not specified for 'u-boot.rom' and probing 
guessed raw.
          Automatically detecting the format is dangerous for raw 
images, write operations on block 0 will be restricted.
          Specify the 'raw' format explicitly to remove the restrictions.

U-Boot SPL 2018.07-rc2-00066-gebea1f8367 (Jun 26 2018 - 13:13:34 +0200)
CPU: x86_64, vendor AMD, device 663h
Trying to boot from SPI
Jumping to 64-bit U-Boot: Note many features are missing
  No serial driver found
QEMU: Terminated

[U-Boot] [U-Boot, v4, 11/21] efi_loader: Introduce ms abi vararg helpers

[Bin Meng]

Hi Alex,

On Tue, Jun 26, 2018 at 7:18 PM, Alexander Graf <agraf@suse.de> wrote:
> On 06/26/2018 03:51 AM, Bin Meng wrote:
>>
>> Hi Alex,
>>
>> On Tue, Jun 26, 2018 at 12:47 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/23/2018 10:37 AM, Bin Meng wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow
>>>>>> the
>>>>>> ms
>>>>>> abi though, so we also need to make sure we use x86_64 varargs
>>>>>> helpers.
>>>>>>
>>>>>> This patch introduces generic efi vararg helpers that adhere to the
>>>>>> respective EFI ABI. That way we can deal with them properly from efi
>>>>>> loader code and properly interpret variable arguments.
>>>>>>
>>>>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi
>>>>>> selftests
>>>>>> on x86_64 for me.
>>>>>>
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>
>>>>> Thanks, applied to efi-next
>>>>>
>>>> I applied this patch on my x86 tree and tested there, but
>>>> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
>>>> am missing?
>>>
>>>
>>> Where does it fail? There is basically this pitfall and setjmp/longjmp
>>> that
>>> can easily go wrong.
>>
>> It just reboots without printing any error message. You can reproduce
>> this in the latest u-boot/master.
>
>
> How do you run this? I tried the qemu x86_64 target, but it complains about
> a missing serial driver. I guess I need to pass some DT in?
>

That's weird. I never saw such boot logs before. There is no need to pass DT in.

> Alex
>
> $ qemu-system-x86_64 -nographic -pflash u-boot.rom -m 1G

I was using '-bios' instead of '-pflash', but I just tested '-pflash'
and it worked too. I am using QEMU 2.5.0 (the one shipped on Ubuntu
16.04)

> WARNING: Image format was not specified for 'u-boot.rom' and probing guessed
> raw.
>          Automatically detecting the format is dangerous for raw images,
> write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
>
> U-Boot SPL 2018.07-rc2-00066-gebea1f8367 (Jun 26 2018 - 13:13:34 +0200)
> CPU: x86_64, vendor AMD, device 663h
> Trying to boot from SPI
> Jumping to 64-bit U-Boot: Note many features are missing
>  No serial driver found
> QEMU: Terminated
>

Regards,
Bin

[U-Boot] [PATCH v4 12/21] 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 v4 13/21] distro: Move to compiler based target architecture determination

[Alexander Graf]

Thanks to CONFIG_SANDBOX, we can not rely on config options to tell us
what CPU architecture we're running on.

The compiler however does know that, so let's just move the ifdefs over
to compiler based defines rather than kconfig based options.

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

---

v3 -> v4:

  - Compile fix for dts
---
 include/config_distro_bootcmd.h | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 1bd79ae3b8..c35a42f6c5 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -245,35 +245,27 @@
 #if defined(CONFIG_CMD_DHCP)
 #if defined(CONFIG_EFI_LOADER)
 /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
-#if defined(CONFIG_ARM64)
+#if defined(__aarch64__)
 #define BOOTENV_EFI_PXE_ARCH "0xb"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00011:UNDI:003000"
-#elif defined(CONFIG_ARM)
+#elif defined(__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 */
+#elif defined(__x86_64__)
 #define BOOTENV_EFI_PXE_ARCH "0x7"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
-#elif defined(CONFIG_CPU_RISCV_32)
+#elif defined(__i386__)
+#define BOOTENV_EFI_PXE_ARCH "0x0"
+#define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00000:UNDI:003000"
+#elif defined(__riscv) && (__riscv_xlen == 32)
 #define BOOTENV_EFI_PXE_ARCH "0x19"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00025:UNDI:003000"
-#elif defined(CONFIG_CPU_RISCV_64)
+#elif defined(__riscv) && (__riscv_xlen == 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"
+#elif defined(__ASSEMBLY__)
+#define BOOTENV_EFI_PXE_ARCH "0xff"
+#define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00255:UNDI:003000"
 #else
 #error Please specify an EFI client identifier
 #endif
-- 
2.12.3

[U-Boot] [PATCH v4 14/21] efi_loader: Move to compiler based target architecture determination

[Alexander Graf]

Thanks to CONFIG_SANDBOX, we can not rely on config options to tell us
what CPU architecture we're running on.

The compiler however does know that, so let's just move the ifdefs over
to compiler based defines rather than kconfig based options.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_runtime.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 388dfb9840..bc44e43745 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -32,18 +32,18 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);
  * TODO(sjg at chromium.org): These defines and structs should come from the elf
  * header for each arch (or a generic header) rather than being repeated here.
  */
-#if defined(CONFIG_ARM64)
+#if defined(__aarch64__)
 #define R_RELATIVE	1027
 #define R_MASK		0xffffffffULL
 #define IS_RELA		1
-#elif defined(CONFIG_ARM)
+#elif defined(__arm__)
 #define R_RELATIVE	23
 #define R_MASK		0xffULL
-#elif defined(CONFIG_X86)
+#elif defined(__x86_64__) || defined(__i386__)
 #include <asm/elf.h>
 #define R_RELATIVE	R_386_RELATIVE
 #define R_MASK		0xffULL
-#elif defined(CONFIG_RISCV)
+#elif defined(__riscv)
 #include <elf.h>
 #define R_RELATIVE	R_RISCV_RELATIVE
 #define R_MASK		0xffULL
@@ -55,25 +55,15 @@ struct dyn_sym {
 	u32 foo2;
 	u32 foo3;
 };
-#ifdef CONFIG_CPU_RISCV_32
+#if (__riscv_xlen == 32)
 #define R_ABSOLUTE	R_RISCV_32
 #define SYM_INDEX	8
-#else
+#elif (__riscv_xlen == 64)
 #define R_ABSOLUTE	R_RISCV_64
 #define SYM_INDEX	32
+#else
+#error unknown riscv target
 #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] [U-Boot, v4, 14/21] efi_loader: Move to compiler based target architecture determination

[Alexander Graf]

> Thanks to CONFIG_SANDBOX, we can not rely on config options to tell us
> what CPU architecture we're running on.
> 
> The compiler however does know that, so let's just move the ifdefs over
> to compiler based defines rather than kconfig based options.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 15/21] sandbox: Fix setjmp/longjmp

[Alexander Graf]

In sandbox, longjmp returns to itself in an endless loop. Cut this through
by calling the real OS function.

Setjmp on the other hand must not return. So here we have to call the OS
setjmp function straight from the code where the setjmp call happens.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/sandbox/cpu/cpu.c            |  5 -----
 arch/sandbox/cpu/os.c             | 20 ++------------------
 arch/sandbox/include/asm/setjmp.h |  4 +++-
 3 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 29dac8dfda..9087026d71 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -167,11 +167,6 @@ ulong timer_get_boot_us(void)
 	return (count - base_count) / 1000;
 }
 
-int setjmp(jmp_buf jmp)
-{
-	return os_setjmp((ulong *)jmp, sizeof(*jmp));
-}
-
 void longjmp(jmp_buf jmp, int ret)
 {
 	os_longjmp((ulong *)jmp, ret);
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 5839932b00..9cfdc9f31f 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -630,24 +630,8 @@ void os_localtime(struct rtc_time *rt)
 	rt->tm_isdst = tm->tm_isdst;
 }
 
-int os_setjmp(ulong *jmp, int size)
-{
-	jmp_buf dummy;
-
-	/*
-	 * We cannot rely on the struct name that jmp_buf uses, so use a
-	 * local variable here
-	 */
-	if (size < sizeof(dummy)) {
-		printf("setjmp: jmpbuf is too small (%d bytes, need %d)\n",
-		       size, sizeof(jmp_buf));
-		return -ENOSPC;
-	}
-
-	return setjmp((struct __jmp_buf_tag *)jmp);
-}
-
 void os_longjmp(ulong *jmp, int ret)
 {
-	longjmp((struct __jmp_buf_tag *)jmp, ret);
+	/* Call the OS longjmp function directly */
+	_longjmp((struct __jmp_buf_tag *)jmp, ret);
 }
diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h
index 1fe37c91cc..e103d170e0 100644
--- a/arch/sandbox/include/asm/setjmp.h
+++ b/arch/sandbox/include/asm/setjmp.h
@@ -24,7 +24,9 @@ struct jmp_buf_data {
 
 typedef struct jmp_buf_data jmp_buf[1];
 
-int setjmp(jmp_buf jmp);
+/* Call the OS setjmp function directly, so that we don't introduce returns */
+#define setjmp _setjmp
+int _setjmp(jmp_buf jmp);
 __noreturn void longjmp(jmp_buf jmp, int ret);
 
 #endif /* _SETJMP_H_ */
-- 
2.12.3

[U-Boot] [PATCH v4 15/21] sandbox: Fix setjmp/longjmp

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> In sandbox, longjmp returns to itself in an endless loop. Cut this through
> by calling the real OS function.
>
> Setjmp on the other hand must not return. So here we have to call the OS
> setjmp function straight from the code where the setjmp call happens.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c            |  5 -----
>  arch/sandbox/cpu/os.c             | 20 ++------------------
>  arch/sandbox/include/asm/setjmp.h |  4 +++-
>  3 files changed, 5 insertions(+), 24 deletions(-)
>

I think we do need to do something like this. But I cannot find the
man page for _longjmp(). Are you sure this is a valid function on all
systems? Or is it Linux only?

Regards.
Simon

[U-Boot] [PATCH v4 15/21] sandbox: Fix setjmp/longjmp

[Alexander Graf]

On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> In sandbox, longjmp returns to itself in an endless loop. Cut this through
>> by calling the real OS function.
>>
>> Setjmp on the other hand must not return. So here we have to call the OS
>> setjmp function straight from the code where the setjmp call happens.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   arch/sandbox/cpu/cpu.c            |  5 -----
>>   arch/sandbox/cpu/os.c             | 20 ++------------------
>>   arch/sandbox/include/asm/setjmp.h |  4 +++-
>>   3 files changed, 5 insertions(+), 24 deletions(-)
>>
> I think we do need to do something like this. But I cannot find the
> man page for _longjmp(). Are you sure this is a valid function on all
> systems? Or is it Linux only?

I'm afraid it may be Linux (glibc) only.

I guess the alternative to this would be to not use system 
setjmp/longjmp at all and instead use the in-U-Boot versions of them.


Alex

[U-Boot] [PATCH v4 15/21] sandbox: Fix setjmp/longjmp

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:41, Alexander Graf <agraf@suse.de> wrote:
>
> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> In sandbox, longjmp returns to itself in an endless loop. Cut this through
>>> by calling the real OS function.
>>>
>>> Setjmp on the other hand must not return. So here we have to call the OS
>>> setjmp function straight from the code where the setjmp call happens.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>   arch/sandbox/cpu/cpu.c            |  5 -----
>>>   arch/sandbox/cpu/os.c             | 20 ++------------------
>>>   arch/sandbox/include/asm/setjmp.h |  4 +++-
>>>   3 files changed, 5 insertions(+), 24 deletions(-)
>>>
>> I think we do need to do something like this. But I cannot find the
>> man page for _longjmp(). Are you sure this is a valid function on all
>> systems? Or is it Linux only?
>
>
> I'm afraid it may be Linux (glibc) only.
>
> I guess the alternative to this would be to not use system setjmp/longjmp at all and instead use the in-U-Boot versions of them.

So can we update this patch to check for Linux glibc? Failing that,
perhaps setjmp() could return a negative error value? Then (in other
patches) the caller can make a note that longjmp() cannot be used, and
will return errors from things that need to call it.

Regards,
Simon

[U-Boot] [PATCH v4 15/21] sandbox: Fix setjmp/longjmp

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:41, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>>>> In sandbox, longjmp returns to itself in an endless loop. Cut this through
>>>> by calling the real OS function.
>>>>
>>>> Setjmp on the other hand must not return. So here we have to call the OS
>>>> setjmp function straight from the code where the setjmp call happens.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>    arch/sandbox/cpu/cpu.c            |  5 -----
>>>>    arch/sandbox/cpu/os.c             | 20 ++------------------
>>>>    arch/sandbox/include/asm/setjmp.h |  4 +++-
>>>>    3 files changed, 5 insertions(+), 24 deletions(-)
>>>>
>>> I think we do need to do something like this. But I cannot find the
>>> man page for _longjmp(). Are you sure this is a valid function on all
>>> systems? Or is it Linux only?
>>
>> I'm afraid it may be Linux (glibc) only.
>>
>> I guess the alternative to this would be to not use system setjmp/longjmp at all and instead use the in-U-Boot versions of them.
> So can we update this patch to check for Linux glibc? Failing that,
> perhaps setjmp() could return a negative error value? Then (in other
> patches) the caller can make a note that longjmp() cannot be used, and
> will return errors from things that need to call it.

I actually think there is a much easier way. We can just directly export 
the symbols for libc's setjmp/longjmp functions in the sandbox setjmp.h 
header. That way we avoid all the nasty problems we've encountered so 
far. I've cooked up a patch and it seems to work fine.


Alex

[U-Boot] [PATCH v4 16/21] elf: Move x86 reloc defines to common elf.h

[Alexander Graf]

We need to know about x86 relocation definitions even in cases where
we don't officially build against the x86 target, such as with sandbox.

So let's move the x86 definitions into the common elf header, where all
other architectures already have them.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/include/asm/elf.h      | 45 -----------------------------------------
 arch/x86/lib/reloc_ia32_efi.c   |  1 -
 arch/x86/lib/reloc_x86_64_efi.c |  1 -
 include/elf.h                   | 35 ++++++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 47 deletions(-)
 delete mode 100644 arch/x86/include/asm/elf.h

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
deleted file mode 100644
index 7ae9c7d6da..0000000000
--- a/arch/x86/include/asm/elf.h
+++ /dev/null
@@ -1,45 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
-/*
- * Brought in from Linux 4.1, removed things not useful to U-Boot.
- * The definitions perhaps came from the GNU Library which is GPL.
- */
-
-#ifndef _ASM_X86_ELF_H
-#define _ASM_X86_ELF_H
-
-/* ELF register definitions */
-#define R_386_NONE	0
-#define R_386_32	1
-#define R_386_PC32	2
-#define R_386_GOT32	3
-#define R_386_PLT32	4
-#define R_386_COPY	5
-#define R_386_GLOB_DAT	6
-#define R_386_JMP_SLOT	7
-#define R_386_RELATIVE	8
-#define R_386_GOTOFF	9
-#define R_386_GOTPC	10
-#define R_386_NUM	11
-
-/* x86-64 relocation types */
-#define R_X86_64_NONE		0	/* No reloc */
-#define R_X86_64_64		1	/* Direct 64 bit  */
-#define R_X86_64_PC32		2	/* PC relative 32 bit signed */
-#define R_X86_64_GOT32		3	/* 32 bit GOT entry */
-#define R_X86_64_PLT32		4	/* 32 bit PLT address */
-#define R_X86_64_COPY		5	/* Copy symbol at runtime */
-#define R_X86_64_GLOB_DAT	6	/* Create GOT entry */
-#define R_X86_64_JUMP_SLOT	7	/* Create PLT entry */
-#define R_X86_64_RELATIVE	8	/* Adjust by program base */
-/* 32 bit signed pc relative offset to GOT */
-#define R_X86_64_GOTPCREL	9
-#define R_X86_64_32		10	/* Direct 32 bit zero extended */
-#define R_X86_64_32S		11	/* Direct 32 bit sign extended */
-#define R_X86_64_16		12	/* Direct 16 bit zero extended */
-#define R_X86_64_PC16		13	/* 16 bit sign extended pc relative */
-#define R_X86_64_8		14	/* Direct 8 bit sign extended  */
-#define R_X86_64_PC8		15	/* 8 bit sign extended pc relative */
-
-#define R_X86_64_NUM		16
-
-#endif
diff --git a/arch/x86/lib/reloc_ia32_efi.c b/arch/x86/lib/reloc_ia32_efi.c
index e838af3b70..f0bd2dbd57 100644
--- a/arch/x86/lib/reloc_ia32_efi.c
+++ b/arch/x86/lib/reloc_ia32_efi.c
@@ -10,7 +10,6 @@
 #include <common.h>
 #include <efi.h>
 #include <elf.h>
-#include <asm/elf.h>
 
 efi_status_t _relocate(long ldbase, Elf32_Dyn *dyn, efi_handle_t image,
 		       struct efi_system_table *systab)
diff --git a/arch/x86/lib/reloc_x86_64_efi.c b/arch/x86/lib/reloc_x86_64_efi.c
index 34c5b2ed3f..adc80ea82a 100644
--- a/arch/x86/lib/reloc_x86_64_efi.c
+++ b/arch/x86/lib/reloc_x86_64_efi.c
@@ -12,7 +12,6 @@
 #include <common.h>
 #include <efi.h>
 #include <elf.h>
-#include <asm/elf.h>
 
 efi_status_t _relocate(long ldbase, Elf64_Dyn *dyn, efi_handle_t image,
 		       struct efi_system_table *systab)
diff --git a/include/elf.h b/include/elf.h
index 0d3845e063..6802428ac4 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -550,6 +550,41 @@ unsigned long elf_hash(const unsigned char *name);
 
 #endif /* __ASSEMBLER */
 
+/* ELF register definitions */
+#define R_386_NONE	0
+#define R_386_32	1
+#define R_386_PC32	2
+#define R_386_GOT32	3
+#define R_386_PLT32	4
+#define R_386_COPY	5
+#define R_386_GLOB_DAT	6
+#define R_386_JMP_SLOT	7
+#define R_386_RELATIVE	8
+#define R_386_GOTOFF	9
+#define R_386_GOTPC	10
+#define R_386_NUM	11
+
+/* x86-64 relocation types */
+#define R_X86_64_NONE		0	/* No reloc */
+#define R_X86_64_64		1	/* Direct 64 bit  */
+#define R_X86_64_PC32		2	/* PC relative 32 bit signed */
+#define R_X86_64_GOT32		3	/* 32 bit GOT entry */
+#define R_X86_64_PLT32		4	/* 32 bit PLT address */
+#define R_X86_64_COPY		5	/* Copy symbol at runtime */
+#define R_X86_64_GLOB_DAT	6	/* Create GOT entry */
+#define R_X86_64_JUMP_SLOT	7	/* Create PLT entry */
+#define R_X86_64_RELATIVE	8	/* Adjust by program base */
+/* 32 bit signed pc relative offset to GOT */
+#define R_X86_64_GOTPCREL	9
+#define R_X86_64_32		10	/* Direct 32 bit zero extended */
+#define R_X86_64_32S		11	/* Direct 32 bit sign extended */
+#define R_X86_64_16		12	/* Direct 16 bit zero extended */
+#define R_X86_64_PC16		13	/* 16 bit sign extended pc relative */
+#define R_X86_64_8		14	/* Direct 8 bit sign extended  */
+#define R_X86_64_PC8		15	/* 8 bit sign extended pc relative */
+
+#define R_X86_64_NUM		16
+
 /*
  * XXX - PowerPC defines really don't belong in here,
  * but we'll put them in for simplicity.
-- 
2.12.3

[U-Boot] [U-Boot, v4, 16/21] elf: Move x86 reloc defines to common elf.h

[Alexander Graf]

> We need to know about x86 relocation definitions even in cases where
> we don't officially build against the x86 target, such as with sandbox.
> 
> So let's move the x86 definitions into the common elf header, where all
> other architectures already have them.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 17/21] efi_loader: Use common elf.h reloc defines

[Alexander Graf]

Now that elf.h contains relocation defines for all architectures
we care about, let's just include it unconditionally and refer to
the defines.

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

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index bc44e43745..dd3ff8ad23 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -8,6 +8,7 @@
 #include <common.h>
 #include <command.h>
 #include <dm.h>
+#include <elf.h>
 #include <efi_loader.h>
 #include <rtc.h>
 
@@ -33,18 +34,16 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);
  * header for each arch (or a generic header) rather than being repeated here.
  */
 #if defined(__aarch64__)
-#define R_RELATIVE	1027
+#define R_RELATIVE	R_AARCH64_RELATIVE
 #define R_MASK		0xffffffffULL
 #define IS_RELA		1
 #elif defined(__arm__)
-#define R_RELATIVE	23
+#define R_RELATIVE	R_ARM_RELATIVE
 #define R_MASK		0xffULL
 #elif defined(__x86_64__) || defined(__i386__)
-#include <asm/elf.h>
 #define R_RELATIVE	R_386_RELATIVE
 #define R_MASK		0xffULL
 #elif defined(__riscv)
-#include <elf.h>
 #define R_RELATIVE	R_RISCV_RELATIVE
 #define R_MASK		0xffULL
 #define IS_RELA		1
-- 
2.12.3

[U-Boot] [U-Boot, v4, 17/21] efi_loader: Use common elf.h reloc defines

[Alexander Graf]

> Now that elf.h contains relocation defines for all architectures
> we care about, let's just include it unconditionally and refer to
> the defines.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 18/21] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

From: Simon Glass <sjg@chromium.org>

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.

Signed-off-by: Simon Glass <sjg@chromium.org>
[agraf: Remove map_sysmem() call]
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_memory.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index ce29bcc6a3..19492df518 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -9,6 +9,7 @@
 #include <efi_loader.h>
 #include <inttypes.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <watchdog.h>
 #include <linux/list_sort.h>
 
@@ -496,14 +497,13 @@ __weak void efi_add_known_memory(void)
 	}
 }
 
-int efi_memory_init(void)
+/* Add memory regions for U-Boot's memory and for the runtime services code */
+static void add_u_boot_and_runtime(void)
 {
 	unsigned long runtime_start, runtime_end, runtime_pages;
 	unsigned long uboot_start, uboot_pages;
 	unsigned long uboot_stack_size = 16 * 1024 * 1024;
 
-	efi_add_known_memory();
-
 	/* Add U-Boot */
 	uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
 	uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
@@ -516,6 +516,14 @@ int efi_memory_init(void)
 	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
 	efi_add_memory_map(runtime_start, runtime_pages,
 			   EFI_RUNTIME_SERVICES_CODE, false);
+}
+
+int efi_memory_init(void)
+{
+	efi_add_known_memory();
+
+	if (!IS_ENABLED(CONFIG_SANDBOX))
+		add_u_boot_and_runtime();
 
 #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
 	/* Request a 32bit 64MB bounce buffer region */
-- 
2.12.3

[U-Boot] [PATCH v4 18/21] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> From: Simon Glass <sjg@chromium.org>
>
> 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.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> [agraf: Remove map_sysmem() call]
> Signed-off-by: Alexander Graf <agraf@suse.de>

I'm a bit confused about all the duplicate patches here. I didn't
think I had a map_sysmem() in my version?

Regards,
Simon

[U-Boot] [U-Boot, v4, 18/21] efi: sandbox: Adjust memory usage for sandbox

[Alexander Graf]

> From: Simon Glass <sjg@chromium.org>
> 
> 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.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> [agraf: Remove map_sysmem() call]
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 19/21] sandbox: Allow to execute from RAM

[Alexander Graf]

With efi_loader, we may want to execute payloads from RAM. By default,
permissions on the RAM region don't allow us to execute from there though.

So whenever we get into the efi_loader case, let's mark RAM as executable.
That way we still protect normal cases, but allow for efi binaries to
directly get executed from within RAM.

For this, we hook into the already existing allow_unaligned() call which
also transitions the system over into semantics required by the UEFI
specification.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/sandbox/cpu/cpu.c | 14 ++++++++++++++
 arch/sandbox/cpu/os.c  | 14 ++++++++++++++
 include/os.h           | 19 +++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 9087026d71..641b66a0a7 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -192,3 +192,17 @@ void efi_add_known_memory(void)
 }
 
 #endif
+
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+
+void allow_unaligned(void)
+{
+	int r;
+
+	r = os_mprotect(gd->arch.ram_buf, gd->ram_size,
+			OS_PROT_READ | OS_PROT_WRITE | OS_PROT_EXEC);
+
+	assert(!r);
+}
+
+#endif
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 9cfdc9f31f..9fa79a8843 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -183,6 +183,20 @@ void *os_realloc(void *ptr, size_t length)
 	return buf;
 }
 
+int os_mprotect(void *ptr, size_t length, int prot)
+{
+	int p = 0;
+
+	if (prot & OS_PROT_READ)
+		p |= PROT_READ;
+	if (prot & OS_PROT_WRITE)
+		p |= PROT_WRITE;
+	if (prot & OS_PROT_EXEC)
+		p |= PROT_EXEC;
+
+	return mprotect(ptr, length, p);
+}
+
 void os_usleep(unsigned long usec)
 {
 	usleep(usec);
diff --git a/include/os.h b/include/os.h
index c8e0f52d30..d451e12064 100644
--- a/include/os.h
+++ b/include/os.h
@@ -157,6 +157,25 @@ void os_free(void *ptr);
 void *os_realloc(void *ptr, size_t length);
 
 /**
+ * Modify protection of a memory region
+ *
+ * This function changes the memory protection scheme of a given memory
+ * region. Using it you can for example allow execution of memory that
+ * would otherwise prohibit it.
+ *
+ * \param ptr		Pointer to memory region to modify
+ * \param length	New length for memory block
+ * \param prot		New protection scheme (ORed OS_PROT_ values)
+ * \return 0 on success, -1 otherwise.
+ */
+int os_mprotect(void *ptr, size_t length, int prot);
+
+/* Defines for "prot" in os_mprotect() */
+#define OS_PROT_READ	0x1
+#define OS_PROT_WRITE	0x2
+#define OS_PROT_EXEC	0x4
+
+/**
  * Access to the usleep function of the os
  *
  * \param usec Time to sleep in micro seconds
-- 
2.12.3

[U-Boot] [PATCH v4 19/21] sandbox: Allow to execute from RAM

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> With efi_loader, we may want to execute payloads from RAM. By default,
> permissions on the RAM region don't allow us to execute from there though.
>
> So whenever we get into the efi_loader case, let's mark RAM as executable.
> That way we still protect normal cases, but allow for efi binaries to
> directly get executed from within RAM.
>
> For this, we hook into the already existing allow_unaligned() call which
> also transitions the system over into semantics required by the UEFI
> specification.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c | 14 ++++++++++++++
>  arch/sandbox/cpu/os.c  | 14 ++++++++++++++
>  include/os.h           | 19 +++++++++++++++++++
>  3 files changed, 47 insertions(+)
>

What is this patch actually for? Does it make something work that did
not before? Where is it called?

Regards.
Simon

[U-Boot] [PATCH v4 19/21] sandbox: Allow to execute from RAM

[Alexander Graf]

On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> With efi_loader, we may want to execute payloads from RAM. By default,
>> permissions on the RAM region don't allow us to execute from there though.
>>
>> So whenever we get into the efi_loader case, let's mark RAM as executable.
>> That way we still protect normal cases, but allow for efi binaries to
>> directly get executed from within RAM.
>>
>> For this, we hook into the already existing allow_unaligned() call which
>> also transitions the system over into semantics required by the UEFI
>> specification.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   arch/sandbox/cpu/cpu.c | 14 ++++++++++++++
>>   arch/sandbox/cpu/os.c  | 14 ++++++++++++++
>>   include/os.h           | 19 +++++++++++++++++++
>>   3 files changed, 47 insertions(+)
>>
> What is this patch actually for? Does it make something work that did
> not before? Where is it called?

At least on aarch64 executing from the RAM region fails on the first 
instruction you call inside it, because it's not mapped with PROT_EXEC. 
I think not mapping it with PROT_EXEC is a good thing in the normal 
sandbox use case, but for EFI we need to run from RAM ;).

So yes, this patch makes that work. It's called from allow_unaligned() 
which gets called from the bootefi command function.

Alex

[U-Boot] [PATCH v4 19/21] sandbox: Allow to execute from RAM

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:44, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> With efi_loader, we may want to execute payloads from RAM. By default,
>>> permissions on the RAM region don't allow us to execute from there
>>> though.
>>>
>>> So whenever we get into the efi_loader case, let's mark RAM as
>>> executable.
>>> That way we still protect normal cases, but allow for efi binaries to
>>> directly get executed from within RAM.
>>>
>>> For this, we hook into the already existing allow_unaligned() call which
>>> also transitions the system over into semantics required by the UEFI
>>> specification.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>   arch/sandbox/cpu/cpu.c | 14 ++++++++++++++
>>>   arch/sandbox/cpu/os.c  | 14 ++++++++++++++
>>>   include/os.h           | 19 +++++++++++++++++++
>>>   3 files changed, 47 insertions(+)
>>>
>> What is this patch actually for? Does it make something work that did
>> not before? Where is it called?
>
>
> At least on aarch64 executing from the RAM region fails on the first
> instruction you call inside it, because it's not mapped with PROT_EXEC. I
> think not mapping it with PROT_EXEC is a good thing in the normal sandbox
> use case, but for EFI we need to run from RAM ;).
>
> So yes, this patch makes that work. It's called from allow_unaligned() which
> gets called from the bootefi command function.

OK I see. This is so that sandbox running on an ARM platform can
actually load and run an EFI application, right?

Can you please make this code and commit message ARM-specific? Also,
just call your code on start-up in cpu.c. We don't need to connect to
an allow_unaligned() thing, which is a misnomer in this case.

Regards,
Simon

[U-Boot] [PATCH v4 19/21] sandbox: Allow to execute from RAM

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:44, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>>>> With efi_loader, we may want to execute payloads from RAM. By default,
>>>> permissions on the RAM region don't allow us to execute from there
>>>> though.
>>>>
>>>> So whenever we get into the efi_loader case, let's mark RAM as
>>>> executable.
>>>> That way we still protect normal cases, but allow for efi binaries to
>>>> directly get executed from within RAM.
>>>>
>>>> For this, we hook into the already existing allow_unaligned() call which
>>>> also transitions the system over into semantics required by the UEFI
>>>> specification.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>    arch/sandbox/cpu/cpu.c | 14 ++++++++++++++
>>>>    arch/sandbox/cpu/os.c  | 14 ++++++++++++++
>>>>    include/os.h           | 19 +++++++++++++++++++
>>>>    3 files changed, 47 insertions(+)
>>>>
>>> What is this patch actually for? Does it make something work that did
>>> not before? Where is it called?
>>
>> At least on aarch64 executing from the RAM region fails on the first
>> instruction you call inside it, because it's not mapped with PROT_EXEC. I
>> think not mapping it with PROT_EXEC is a good thing in the normal sandbox
>> use case, but for EFI we need to run from RAM ;).
>>
>> So yes, this patch makes that work. It's called from allow_unaligned() which
>> gets called from the bootefi command function.
> OK I see. This is so that sandbox running on an ARM platform can
> actually load and run an EFI application, right?
>
> Can you please make this code and commit message ARM-specific? Also,

Not, it's not ARM specific. You may hit this on any system that actually 
checks for PROT_EXEC.

> just call your code on start-up in cpu.c. We don't need to connect to
> an allow_unaligned() thing, which is a misnomer in this case.

In that case we can just tell mmap() to map RAM with PROC_EXEC.


Alex

[U-Boot] [PATCH v4 20/21] sandbox: Always allocate aligned buffers

[Alexander Graf]

In some code paths we check whether host virtual addresses are sane.
That only works if at least alignments between host and U-Boot address
spaces match.

So let's always map U-Boot addresses with 64kb alignment. That should
be enough to ensure that the actual RAM ends up in a different page
from the header on all architectures.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/sandbox/cpu/os.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 9fa79a8843..4dc6483922 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -143,14 +143,15 @@ void os_tty_raw(int fd, bool allow_sigs)
 void *os_malloc(size_t length)
 {
 	struct os_mem_hdr *hdr;
+	size_t alloc_length = length + (64 * 1024);
 
-	hdr = mmap(NULL, length + sizeof(*hdr), PROT_READ | PROT_WRITE,
+	hdr = mmap(NULL, alloc_length, PROT_READ | PROT_WRITE,
 		   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 	if (hdr == MAP_FAILED)
 		return NULL;
 	hdr->length = length;
 
-	return hdr + 1;
+	return (void*)hdr + (64 * 1024);
 }
 
 void os_free(void *ptr)
-- 
2.12.3

[U-Boot] [PATCH v4 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Alexander Graf]

We currently expose host addresses in the EFI memory map. That can be
bad if we ever want to use sandbox to boot strap a real kernel, because
then the kernel would fetch its memory table from our host virtual address
map. But to make that use case work, we would need to have full control
over the address space the EFI application sees.

So let's expose only U-Boot addresses to the guest until we get to the
point of allocation. EFI's allocation functions are fun - they can take
U-Boot addresses as input values for hints and return host addresses as
allocation results through the same uint64_t * parameter. So we need to
be extra careful on what to pass in when.

With this patch I am successfully able to run the efi selftest suite as
well as grub.efi on aarch64.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/sandbox/cpu/cpu.c      | 19 -------------------
 lib/efi_loader/efi_memory.c | 12 ++++++------
 2 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 641b66a0a7..be88ab2f1c 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -176,25 +176,6 @@ void longjmp(jmp_buf jmp, int ret)
 
 #if CONFIG_IS_ENABLED(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
-
-#if CONFIG_IS_ENABLED(EFI_LOADER)
-
 void allow_unaligned(void)
 {
 	int r;
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 19492df518..64d2b8f7fa 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -326,7 +326,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
 		/* Reserve that map in our memory maps */
 		ret = efi_add_memory_map(addr, pages, memory_type, true);
 		if (ret == addr) {
-			*memory = addr;
+			*memory = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE);
 		} else {
 			/* Map would overlap, bail out */
 			r = EFI_OUT_OF_RESOURCES;
@@ -360,11 +360,12 @@ void *efi_alloc(uint64_t len, int memory_type)
 efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 {
 	uint64_t r = 0;
+	uint64_t addr = map_to_sysmem((void*)(uintptr_t)memory);
 
-	r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
+	r = efi_add_memory_map(addr, pages, EFI_CONVENTIONAL_MEMORY, false);
 	/* Merging of adjacent free regions is missing */
 
-	if (r == memory)
+	if (r == addr)
 		return EFI_SUCCESS;
 
 	return EFI_NOT_FOUND;
@@ -381,9 +382,9 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
 {
 	efi_status_t r;
-	efi_physical_addr_t t;
 	u64 num_pages = (size + sizeof(struct efi_pool_allocation) +
 			 EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
+	struct efi_pool_allocation *alloc;
 
 	if (size == 0) {
 		*buffer = NULL;
@@ -391,10 +392,9 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
 	}
 
 	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
-			       &t);
+			       (uint64_t*)&alloc);
 
 	if (r == EFI_SUCCESS) {
-		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
 		alloc->num_pages = num_pages;
 		*buffer = alloc->data;
 	}
-- 
2.12.3

[U-Boot] [U-Boot, v4, 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Alexander Graf]

> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
> 
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
> 
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex

[U-Boot] [PATCH v4 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
>
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
>
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c      | 19 -------------------
>  lib/efi_loader/efi_memory.c | 12 ++++++------
>  2 files changed, 6 insertions(+), 25 deletions(-)

Can you please point me to your latest series? I think you have
decided to work on this yourself and pick bits from my series that you
like. This I consider unpleasant behaviour for a maintainer, but
ultimately I'm more interested in getting things resolved than any
procedural issues. Please don't do this to anyone else, though, in the
U-Boot community.

Anyway, at present I'm not sure what state it is in, so please point
me to your latest tree so I can take a look and figure out what has
actually changed from my v9 series.

Regards,
Simon

Regards,
Simon

[U-Boot] [PATCH v4 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Alexander Graf]

Hi Simon,

> Am 23.06.2018 um 06:01 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> We currently expose host addresses in the EFI memory map. That can be
>> bad if we ever want to use sandbox to boot strap a real kernel, because
>> then the kernel would fetch its memory table from our host virtual address
>> map. But to make that use case work, we would need to have full control
>> over the address space the EFI application sees.
>> 
>> So let's expose only U-Boot addresses to the guest until we get to the
>> point of allocation. EFI's allocation functions are fun - they can take
>> U-Boot addresses as input values for hints and return host addresses as
>> allocation results through the same uint64_t * parameter. So we need to
>> be extra careful on what to pass in when.
>> 
>> With this patch I am successfully able to run the efi selftest suite as
>> well as grub.efi on aarch64.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/sandbox/cpu/cpu.c      | 19 -------------------
>> lib/efi_loader/efi_memory.c | 12 ++++++------
>> 2 files changed, 6 insertions(+), 25 deletions(-)
> 
> Can you please point me to your latest series? I think you have
> decided to work on this yourself and pick bits from my series that you
> like.

Believe me, I also picked things that I don't like. But ultimately sandbox is your court while efi_loader is mine. And I'm fairly sure both of us have better things to do than to run in circles.

> This I consider unpleasant behaviour for a maintainer, but
> ultimately I'm more interested in getting things resolved than any
> procedural issues. Please don't do this to anyone else, though, in the
> U-Boot community.

I don't see the problem - it's pretty common in the Linux world. You propose something, I counterpropose, we converge, maintainer decides what to pick up.

> Anyway, at present I'm not sure what state it is in, so please point
> me to your latest tree so I can take a look and figure out what has
> actually changed from my v9 series.

The current tree with v5 applied is here:

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

Branch efi-next at the same location is the base for v5.


Alex

[U-Boot] [PATCH v4 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Simon Glass]

Hi Alex,

On 23 June 2018 at 00:57, Alexander Graf <agraf@suse.de> wrote:
> Hi Simon,
>
> Am 23.06.2018 um 06:01 schrieb Simon Glass <sjg@chromium.org>:
>
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>
> We currently expose host addresses in the EFI memory map. That can be
>
> bad if we ever want to use sandbox to boot strap a real kernel, because
>
> then the kernel would fetch its memory table from our host virtual address
>
> map. But to make that use case work, we would need to have full control
>
> over the address space the EFI application sees.
>
>
> So let's expose only U-Boot addresses to the guest until we get to the
>
> point of allocation. EFI's allocation functions are fun - they can take
>
> U-Boot addresses as input values for hints and return host addresses as
>
> allocation results through the same uint64_t * parameter. So we need to
>
> be extra careful on what to pass in when.
>
>
> With this patch I am successfully able to run the efi selftest suite as
>
> well as grub.efi on aarch64.
>
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> arch/sandbox/cpu/cpu.c      | 19 -------------------
>
> lib/efi_loader/efi_memory.c | 12 ++++++------
>
> 2 files changed, 6 insertions(+), 25 deletions(-)
>
>
> Can you please point me to your latest series? I think you have
> decided to work on this yourself and pick bits from my series that you
> like.
>
>
> Believe me, I also picked things that I don't like. But ultimately sandbox
> is your court while efi_loader is mine. And I'm fairly sure both of us have
> better things to do than to run in circles.
>
> This I consider unpleasant behaviour for a maintainer, but
> ultimately I'm more interested in getting things resolved than any
> procedural issues. Please don't do this to anyone else, though, in the
> U-Boot community.
>
>
> I don't see the problem - it's pretty common in the Linux world. You propose
> something, I counterpropose, we converge, maintainer decides what to pick
> up.

This is certainly not Linux and I like to think we have a kinder and
more supportive environment here. I very seldom call out people on
this list for language and behaviour.

Also you are a maintainer, not another contributor.

>
> Anyway, at present I'm not sure what state it is in, so please point
> me to your latest tree so I can take a look and figure out what has
> actually changed from my v9 series.
>
>
> The current tree with v5 applied is here:
>
> https://github.com/agraf/u-boot/tree/efi-sandbox-v5
>
> Branch efi-next at the same location is the base for v5.

OK well at this stage I'm going to leave it in your hands as I've lost
track of all the patches and have no desire to send a v10 series.

Once everything lands I'll take a look and see if I think anything is
missing. Hopefully we can get sandbox EFi support into master when the
merge window opens.

Regards,
Simon

[U-Boot] [PATCH v4 21/21] efi_loader: Expose U-Boot addresses in memory map for sandbox

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>
> We currently expose host addresses in the EFI memory map. That can be
> bad if we ever want to use sandbox to boot strap a real kernel, because
> then the kernel would fetch its memory table from our host virtual address
> map. But to make that use case work, we would need to have full control
> over the address space the EFI application sees.
>
> So let's expose only U-Boot addresses to the guest until we get to the
> point of allocation. EFI's allocation functions are fun - they can take
> U-Boot addresses as input values for hints and return host addresses as
> allocation results through the same uint64_t * parameter. So we need to
> be extra careful on what to pass in when.
>
> With this patch I am successfully able to run the efi selftest suite as
> well as grub.efi on aarch64.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c      | 19 -------------------
>  lib/efi_loader/efi_memory.c | 12 ++++++------
>  2 files changed, 6 insertions(+), 25 deletions(-)

This seems to compete with a patch from my series, but it's not clear
to be what the overlap is.

>
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index 641b66a0a7..be88ab2f1c 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -176,25 +176,6 @@ void longjmp(jmp_buf jmp, int ret)
>
>  #if CONFIG_IS_ENABLED(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
> -
> -#if CONFIG_IS_ENABLED(EFI_LOADER)
> -
>  void allow_unaligned(void)
>  {
>         int r;
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 19492df518..64d2b8f7fa 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -326,7 +326,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
>                 /* Reserve that map in our memory maps */
>                 ret = efi_add_memory_map(addr, pages, memory_type, true);
>                 if (ret == addr) {
> -                       *memory = addr;
> +                       *memory = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE);

This line does not seem to correspond to the code in your efi-sandbox-v5 tree.

Also if an address is passed in then presumably it needs
map_to_sysmem() before use (e.g. replace the *memory accesses in this
function).  I suspect those code paths have no tests which is why
things work.

Given that you have efi_allocate_pages() takes (and returns) a pointer
(but stored in a uin64_t) I think you are asking for confusion. At the
least this needs a comment to explain what is being returned.

Apart from that I think it looks right.

>                 } else {
>                         /* Map would overlap, bail out */
>                         r = EFI_OUT_OF_RESOURCES;
> @@ -360,11 +360,12 @@ void *efi_alloc(uint64_t len, int memory_type)
>  efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>  {
>         uint64_t r = 0;
> +       uint64_t addr = map_to_sysmem((void*)(uintptr_t)memory);
>
> -       r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
> +       r = efi_add_memory_map(addr, pages, EFI_CONVENTIONAL_MEMORY, false);
>         /* Merging of adjacent free regions is missing */
>
> -       if (r == memory)
> +       if (r == addr)
>                 return EFI_SUCCESS;
>
>         return EFI_NOT_FOUND;
> @@ -381,9 +382,9 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>  efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>  {
>         efi_status_t r;
> -       efi_physical_addr_t t;
>         u64 num_pages = (size + sizeof(struct efi_pool_allocation) +
>                          EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +       struct efi_pool_allocation *alloc;
>
>         if (size == 0) {
>                 *buffer = NULL;
> @@ -391,10 +392,9 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>         }
>
>         r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> -                              &t);
> +                              (uint64_t*)&alloc);
>
>         if (r == EFI_SUCCESS) {
> -               struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
>                 alloc->num_pages = num_pages;
>                 *buffer = alloc->data;
>         }
> --
> 2.12.3
>

Regards,
Simon

[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[Alexander Graf]

On 06/18/2018 05:22 PM, Alexander Graf wrote:
> This patch set augments Simon's patch set for efi_loader support
> in sandbox[1], but cuts off the memory allocation scheme at a different
> point.
>
> According to the UEFI spec, efi_allocate_pages() takes a uint64_t *
> argument. Via this argument, we get a physical address as input, but
> emit a pointer as output.
>
> With this patch set in place, I can successfully run the selftest suite
> as well as an aarch64 grub.efi binary. X86_64 grub.efi doesn't work
> because that one requires inl instructions to work.

I've assembled a quick grub.efi that does work in sandbox as it no 
longer accesses I/O ports directly. Patch for it is below.

   http://csgraf.de/tmp2/grub.efi

When building your own, make sure to exclude coreboot (cb*) modules - 
they seem to do something dirty and segfault for me. The other modules 
seem to work fine for me so far.


Alex


diff --git a/grub-core/kern/i386/tsc.c b/grub-core/kern/i386/tsc.c
index f266eb131..99fc9f7fb 100644
--- a/grub-core/kern/i386/tsc.c
+++ b/grub-core/kern/i386/tsc.c
@@ -68,7 +68,7 @@ grub_tsc_init (void)
  #ifdef GRUB_MACHINE_XEN
    (void) (grub_tsc_calibrate_from_xen () || calibrate_tsc_hardcode());
  #elif defined (GRUB_MACHINE_EFI)
-  (void) (grub_tsc_calibrate_from_pmtimer () || 
grub_tsc_calibrate_from_pit () || grub_tsc_calibrate_from_efi() || 
calibrate_tsc_hardcode());
+  (void) (grub_tsc_calibrate_from_efi() || calibrate_tsc_hardcode());
  #elif defined (GRUB_MACHINE_COREBOOT)
    (void) (grub_tsc_calibrate_from_pmtimer () || 
grub_tsc_calibrate_from_pit () || calibrate_tsc_hardcode());
  #else
diff --git a/include/grub/i386/io.h b/include/grub/i386/io.h
index ae12a3e3d..9fbeef916 100644
--- a/include/grub/i386/io.h
+++ b/include/grub/i386/io.h
@@ -26,47 +26,40 @@ typedef unsigned short int grub_port_t;
  static __inline unsigned char
  grub_inb (unsigned short int port)
  {
-  unsigned char _v;
+  unsigned char _v = 0;

-  __asm__ __volatile__ ("inb %w1,%0":"=a" (_v):"Nd" (port));
    return _v;
  }

  static __inline unsigned short int
  grub_inw (unsigned short int port)
  {
-  unsigned short _v;
+  unsigned short _v = 0;

-  __asm__ __volatile__ ("inw %w1,%0":"=a" (_v):"Nd" (port));
    return _v;
  }

  static __inline unsigned int
  grub_inl (unsigned short int port)
  {
-  unsigned int _v;
+  unsigned int _v = 0;

-  __asm__ __volatile__ ("inl %w1,%0":"=a" (_v):"Nd" (port));
    return _v;
  }

  static __inline void
  grub_outb (unsigned char value, unsigned short int port)
  {
-  __asm__ __volatile__ ("outb %b0,%w1": :"a" (value), "Nd" (port));
  }

  static __inline void
  grub_outw (unsigned short int value, unsigned short int port)
  {
-  __asm__ __volatile__ ("outw %w0,%w1": :"a" (value), "Nd" (port));
-
  }

  static __inline void
  grub_outl (unsigned int value, unsigned short int port)
  {
-  __asm__ __volatile__ ("outl %0,%w1": :"a" (value), "Nd" (port));
  }

  #endif /* _SYS_IO_H */

[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[Simon Glass]

Hi Alex,

On 18 June 2018 at 09:53, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 05:22 PM, Alexander Graf wrote:
>>
>> This patch set augments Simon's patch set for efi_loader support
>> in sandbox[1], but cuts off the memory allocation scheme at a different
>> point.
>>
>> According to the UEFI spec, efi_allocate_pages() takes a uint64_t *
>> argument. Via this argument, we get a physical address as input, but
>> emit a pointer as output.
>>
>> With this patch set in place, I can successfully run the selftest suite
>> as well as an aarch64 grub.efi binary. X86_64 grub.efi doesn't work
>> because that one requires inl instructions to work.
>
>
> I've assembled a quick grub.efi that does work in sandbox as it no longer
> accesses I/O ports directly. Patch for it is below.
>
>   http://csgraf.de/tmp2/grub.efi
>
> When building your own, make sure to exclude coreboot (cb*) modules - they
> seem to do something dirty and segfault for me. The other modules seem to
> work fine for me so far.

OK thanks for that. The binary says this for me:

efi_load_pe: Invalid DOS signature

I'm running on x86_64.

I tried the patch below but it still crashes, presumably because of
the coreboot modules. How do I actually exclude them? I cannot see
anything in ./configure --help

Regards,
Simon

[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[Alexander Graf]

On 06/21/2018 04:44 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:53, Alexander Graf <agraf@suse.de> wrote:
>> On 06/18/2018 05:22 PM, Alexander Graf wrote:
>>> This patch set augments Simon's patch set for efi_loader support
>>> in sandbox[1], but cuts off the memory allocation scheme at a different
>>> point.
>>>
>>> According to the UEFI spec, efi_allocate_pages() takes a uint64_t *
>>> argument. Via this argument, we get a physical address as input, but
>>> emit a pointer as output.
>>>
>>> With this patch set in place, I can successfully run the selftest suite
>>> as well as an aarch64 grub.efi binary. X86_64 grub.efi doesn't work
>>> because that one requires inl instructions to work.
>>
>> I've assembled a quick grub.efi that does work in sandbox as it no longer
>> accesses I/O ports directly. Patch for it is below.
>>
>>    http://csgraf.de/tmp2/grub.efi
>>
>> When building your own, make sure to exclude coreboot (cb*) modules - they
>> seem to do something dirty and segfault for me. The other modules seem to
>> work fine for me so far.
> OK thanks for that. The binary says this for me:
>
> efi_load_pe: Invalid DOS signature
>
> I'm running on x86_64.

Are you using my patch set or yours? In mine this should be fixed.

> I tried the patch below but it still crashes, presumably because of
> the coreboot modules. How do I actually exclude them? I cannot see
> anything in ./configure --help

When you call grub-mkimage you explicitly pass a list of modules to 
include. In that list, just omit any module that starts with cb :)


Alex

[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[Simon Glass]

Hi Alex,

On 21 June 2018 at 03:47, Alexander Graf <agraf@suse.de> wrote:
> On 06/21/2018 04:44 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:53, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/18/2018 05:22 PM, Alexander Graf wrote:
>>>>
>>>> This patch set augments Simon's patch set for efi_loader support
>>>> in sandbox[1], but cuts off the memory allocation scheme at a different
>>>> point.
>>>>
>>>> According to the UEFI spec, efi_allocate_pages() takes a uint64_t *
>>>> argument. Via this argument, we get a physical address as input, but
>>>> emit a pointer as output.
>>>>
>>>> With this patch set in place, I can successfully run the selftest suite
>>>> as well as an aarch64 grub.efi binary. X86_64 grub.efi doesn't work
>>>> because that one requires inl instructions to work.
>>>
>>>
>>> I've assembled a quick grub.efi that does work in sandbox as it no longer
>>> accesses I/O ports directly. Patch for it is below.
>>>
>>>    http://csgraf.de/tmp2/grub.efi
>>>
>>> When building your own, make sure to exclude coreboot (cb*) modules -
>>> they
>>> seem to do something dirty and segfault for me. The other modules seem to
>>> work fine for me so far.
>>
>> OK thanks for that. The binary says this for me:
>>
>> efi_load_pe: Invalid DOS signature
>>
>> I'm running on x86_64.
>
>
> Are you using my patch set or yours? In mine this should be fixed.

I'm using the series at u-boot-dm/efi-working - am I missing something else?

>
>> I tried the patch below but it still crashes, presumably because of
>> the coreboot modules. How do I actually exclude them? I cannot see
>> anything in ./configure --help
>
>
> When you call grub-mkimage you explicitly pass a list of modules to include.
> In that list, just omit any module that starts with cb :)

In my case I am not specifying a list. I'll see if I can do that. I'm
worried there are a lot of modules to find and specify. I cannot find
documentation on what they are.

Regards,
Simon

[U-Boot] [PATCH v4 00/21] sandbox: efi_loader support

[Alexander Graf]

On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:47, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:44 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 09:53, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/18/2018 05:22 PM, Alexander Graf wrote:
>>>>> This patch set augments Simon's patch set for efi_loader support
>>>>> in sandbox[1], but cuts off the memory allocation scheme at a different
>>>>> point.
>>>>>
>>>>> According to the UEFI spec, efi_allocate_pages() takes a uint64_t *
>>>>> argument. Via this argument, we get a physical address as input, but
>>>>> emit a pointer as output.
>>>>>
>>>>> With this patch set in place, I can successfully run the selftest suite
>>>>> as well as an aarch64 grub.efi binary. X86_64 grub.efi doesn't work
>>>>> because that one requires inl instructions to work.
>>>>
>>>> I've assembled a quick grub.efi that does work in sandbox as it no longer
>>>> accesses I/O ports directly. Patch for it is below.
>>>>
>>>>     http://csgraf.de/tmp2/grub.efi
>>>>
>>>> When building your own, make sure to exclude coreboot (cb*) modules -
>>>> they
>>>> seem to do something dirty and segfault for me. The other modules seem to
>>>> work fine for me so far.
>>> OK thanks for that. The binary says this for me:
>>>
>>> efi_load_pe: Invalid DOS signature
>>>
>>> I'm running on x86_64.
>>
>> Are you using my patch set or yours? In mine this should be fixed.
> I'm using the series at u-boot-dm/efi-working - am I missing something else?
>
>>> I tried the patch below but it still crashes, presumably because of
>>> the coreboot modules. How do I actually exclude them? I cannot see
>>> anything in ./configure --help
>>
>> When you call grub-mkimage you explicitly pass a list of modules to include.
>> In that list, just omit any module that starts with cb :)
> In my case I am not specifying a list. I'll see if I can do that. I'm
> worried there are a lot of modules to find and specify. I cannot find
> documentation on what they are.

./grub-mkimage -O x86_64-efi -o grub.efi $(cd grub-core; ls *mod | cut 
-d . -f 1)

is what I usually do to get all modules. In this case you have to | 
egrep -v '^cb' as well.


Alex