[U-Boot] [PATCH v2 00/11] sandbox: efi_loader support
This patch set augments Simon's patch set for efi_loader support in sandbox[1], but follows a different memory allocation scheme.
Instead of keeping U-Boot (physical) addresses in the EFI memory map, this patch set makes the EFI memory map contain host virtual (virtual) addresses. That way most logic "just works" and all EFI interfaces automatically gain sandbox awareness.
With this patch set in place, I can run a good chunk of the selftest suite as well as efi binaries compiled using gnu-efi.
Alex
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=49832
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled - clarify address vs pointer in fs_read patch - include mapmem.h
Alexander Graf (7): efi_loader: Use compiler constants for image loader efi_loader: Use map_sysmem() in bootefi command efi.h: Do not use config options efi_loader: Allow SMBIOS tables in highmem sandbox: Map host memory for efi_loader efi_loader: Disable miniapps on sandbox efi_loader: Pass address to fs_read()
Heinrich Schuchardt (1): efi_loader: efi_allocate_pages is too restrictive
Simon Glass (3): efi: sandbox: Add distroboot support efi: sandbox: Add relocation constants efi: sandbox: Enable EFI loader for sandbox
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ cmd/bootefi.c | 13 ++++++++----- include/config_distro_bootcmd.h | 13 +++++++++++++ include/efi.h | 17 ++++------------- lib/efi/Makefile | 4 ++-- lib/efi_loader/Kconfig | 2 +- lib/efi_loader/efi_file.c | 5 ++++- lib/efi_loader/efi_image_loader.c | 12 ++++++------ lib/efi_loader/efi_memory.c | 2 +- lib/efi_loader/efi_runtime.c | 12 ++++++++++++ lib/efi_loader/efi_smbios.c | 11 +++++++++-- lib/efi_selftest/Makefile | 2 +- 12 files changed, 81 insertions(+), 32 deletions(-)
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@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
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@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
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 };
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The EFI image loader tries to determine which target architecture we're working with to only load PE binaries that match.
So far this has worked based on CONFIG defines, because the target CPU was always indicated by a config define. With sandbox however, this is not longer true as all sandbox targets only encompass a single CONFIG option and so we need to use compiler defines to determine the CPU architecture.
Signed-off-by: Alexander Graf agraf@suse.de
lib/efi_loader/efi_image_loader.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
NAK to this for now until we figure out a response to my concerns previously expressed.
Regards, Simon
The bootefi command gets a few addresses as values passed in. In sandbox, these values are in U-Boot address space, so we need to make sure we explicitly call map_sysmem() on them to be able to access them.
Signed-off-by: Alexander Graf agraf@suse.de --- cmd/bootefi.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index f55a40dc84..a86a2bd4a9 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -14,6 +14,7 @@ #include <errno.h> #include <linux/libfdt.h> #include <linux/libfdt_env.h> +#include <mapmem.h> #include <memalign.h> #include <asm/global_data.h> #include <asm-generic/sections.h> @@ -389,7 +390,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned long addr; char *saddr; efi_status_t r; - void *fdt_addr; + unsigned long fdt_addr; + void *fdt;
/* Allow unaligned memory access */ allow_unaligned(); @@ -406,11 +408,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_USAGE;
if (argc > 2) { - fdt_addr = (void *)simple_strtoul(argv[2], NULL, 16); + fdt_addr = simple_strtoul(argv[2], NULL, 16); if (!fdt_addr && *argv[2] != '0') return CMD_RET_USAGE; /* Install device tree */ - r = efi_install_fdt(fdt_addr); + fdt = map_sysmem(fdt_addr, 0); + r = efi_install_fdt(fdt); if (r != EFI_SUCCESS) { printf("ERROR: failed to install device tree\n"); return CMD_RET_FAILURE; @@ -429,7 +432,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) addr = simple_strtoul(saddr, NULL, 16); else addr = CONFIG_SYS_LOAD_ADDR; - memcpy((char *)addr, __efi_helloworld_begin, size); + memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); } else #endif #ifdef CONFIG_CMD_BOOTEFI_SELFTEST @@ -475,7 +478,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
printf("## Starting EFI application at %08lx ...\n", addr); - r = do_bootefi_exec((void *)addr, bootefi_device_path, + r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, bootefi_image_path); printf("## Application terminated, r = %lu\n", r & ~EFI_ERROR_MASK);
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The bootefi command gets a few addresses as values passed in. In sandbox, these values are in U-Boot address space, so we need to make sure we explicitly call map_sysmem() on them to be able to access them.
Signed-off-by: Alexander Graf agraf@suse.de
cmd/bootefi.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
Looks right to me. You might consider doing the map lower down in efi_load_pe() but since you don't ever log the address, I suppose it is OK.
Reviewed-by: Simon Glass sjg@chromium.org
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
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
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
We try hard to make sure that SMBIOS tables live in the lower 32bit. However, when we can not find any space at all there, we should not error out but instead just fall back to map them in the full address space instead.
Signed-off-by: Alexander Graf agraf@suse.de
lib/efi_loader/efi_smbios.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Does that actually work? I thought the addresses in the table were always 32-bit?
Regards, Simon
On 14.06.18 21:01, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
We try hard to make sure that SMBIOS tables live in the lower 32bit. However, when we can not find any space at all there, we should not error out but instead just fall back to map them in the full address space instead.
Signed-off-by: Alexander Graf agraf@suse.de
lib/efi_loader/efi_smbios.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Does that actually work? I thought the addresses in the table were always 32-bit?
There is only a single table reference which indeed is 32bit: se->struct_table_address.
That address however is unused in the EFI case usually, because the SMBIOS information is already encapsulated in a table, so there's no need to search through address space for a _DMI_ entry:
https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5122
Alex
On 06/14/2018 09:13 PM, Alexander Graf wrote:
On 14.06.18 21:01, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
We try hard to make sure that SMBIOS tables live in the lower 32bit. However, when we can not find any space at all there, we should not error out but instead just fall back to map them in the full address space instead.
Signed-off-by: Alexander Graf agraf@suse.de
lib/efi_loader/efi_smbios.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Does that actually work? I thought the addresses in the table were always 32-bit?
There is only a single table reference which indeed is 32bit: se->struct_table_address.
That address however is unused in the EFI case usually, because the SMBIOS information is already encapsulated in a table, so there's no need to search through address space for a _DMI_ entry:
https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5122
Alex
We still have an open issue with E820 memory reservations not observed by Linux because they are not mirrored in the memory map. Could the same happen with smbios?
Regards
Heinrich
On 14.06.18 21:26, Heinrich Schuchardt wrote:
On 06/14/2018 09:13 PM, Alexander Graf wrote:
On 14.06.18 21:01, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
We try hard to make sure that SMBIOS tables live in the lower 32bit. However, when we can not find any space at all there, we should not error out but instead just fall back to map them in the full address space instead.
Signed-off-by: Alexander Graf agraf@suse.de
lib/efi_loader/efi_smbios.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Does that actually work? I thought the addresses in the table were always 32-bit?
There is only a single table reference which indeed is 32bit: se->struct_table_address.
That address however is unused in the EFI case usually, because the SMBIOS information is already encapsulated in a table, so there's no need to search through address space for a _DMI_ entry:
https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5122
Alex
We still have an open issue with E820 memory reservations not observed by Linux because they are not mirrored in the memory map. Could the same happen with smbios?
The SMBIOS tables get allocated from the EFI pool in lib/efi_loader/efi_smbios.c.
So the e820 patch fell through the cracks?
Alex
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses.
Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers.
Signed-off-by: Alexander Graf agraf@suse.de
---
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled --- arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index cde0b055a6..23d8b70648 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -5,6 +5,7 @@ #define DEBUG #include <common.h> #include <dm.h> +#include <efi_loader.h> #include <errno.h> #include <linux/libfdt.h> #include <os.h> @@ -177,3 +178,22 @@ void longjmp(jmp_buf jmp, int ret) while (1) ; } + +#ifdef CONFIG_EFI_LOADER + +/* + * In sandbox, we don't have a 1:1 map, so we need to expose + * process addresses instead of U-Boot addresses + */ +void efi_add_known_memory(void) +{ + u64 ram_start = (uintptr_t)map_sysmem(0, gd->ram_size); + u64 ram_size = gd->ram_size; + u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; + u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; + + efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY, + false); +} + +#endif
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses.
Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
NAK.
You should not point sandbox pointers into the EFI tables. I know it looks like a clever shortcut, but it is not correct. It will mess up logging and debugging, since those pointers bear no easily accessible relationship to U-Boot address.
Please start from my v7 patch. I'm happy to help do this correctly. But, again, I think it should come after we have basic sandbox EFI support applied.
Regards, Simon
On 14.06.18 21:02, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses.
Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
NAK.
You should not point sandbox pointers into the EFI tables. I know it looks like a clever shortcut, but it is not correct. It will mess up logging and debugging, since those pointers bear no easily accessible relationship to U-Boot address.
Please start from my v7 patch. I'm happy to help do this correctly. But, again, I think it should come after we have basic sandbox EFI support applied.
I don't want to play ping pong with you here. NAK on your approach until I see it properly executing selftest.
So either we drive this forward or we don't. Your choice.
Alex
Hi Alex,
On 14 June 2018 at 13:15, Alexander Graf agraf@suse.de wrote:
On 14.06.18 21:02, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses.
Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
NAK.
You should not point sandbox pointers into the EFI tables. I know it looks like a clever shortcut, but it is not correct. It will mess up logging and debugging, since those pointers bear no easily accessible relationship to U-Boot address.
Please start from my v7 patch. I'm happy to help do this correctly. But, again, I think it should come after we have basic sandbox EFI support applied.
I don't want to play ping pong with you here. NAK on your approach until I see it properly executing selftest.
I think you just did :-)
But if you are asking for me to pull together a patch that gets that far, then OK. I can see that you are not convinced it would work, or be easy to follow, and I have not proven that yet. I was just hoping to take things in incremental steps since this has been outstanding for so long.
So either we drive this forward or we don't. Your choice.
I have long wanted EFI to fall into the sandbox testing framework, so that e.g. 'make tests' will quickly run the EFI tests. I don't think we are too far away. It doesn't have to happen immediately, but I predict that when we get it working, we won't look back. It will be much more convenient than running a separate app or testing on 'real hardware' and you can set up quite complicated things with it.
Regards, Simon
On 06/14/2018 09:02 PM, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses.
Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
NAK.
You should not point sandbox pointers into the EFI tables. I know it looks like a clever shortcut, but it is not correct. It will mess up logging and debugging, since those pointers bear no easily accessible relationship to U-Boot address.
Hello Simon,
why do we need this Babylonic confusion of addresses which do not point to valid memory and pointers that are not valid addresses where everybody is getting lost?
Simply use only addresses with there mmap'ed values and get rid of all conversions. Use your board file to adjust kernel_addr_r and the like to the address range that Linux offered you.
Best regards
Heinrich
Please start from my v7 patch. I'm happy to help do this correctly. But, again, I think it should come after we have basic sandbox EFI support applied.
Regards, Simon
Hi Heinrich,
On 14 June 2018 at 13:21, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 06/14/2018 09:02 PM, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
With efi_loader we do not control payload applications, so we can not teach them about the difference between virtual and physical addresses.
Instead, let's just always map host virtual addresses in the efi memory map. That way we can be sure that all memory allocation functions always return consumable pointers.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- only compile efi_add_known_memory if efi_loader is enabled
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
NAK.
You should not point sandbox pointers into the EFI tables. I know it looks like a clever shortcut, but it is not correct. It will mess up logging and debugging, since those pointers bear no easily accessible relationship to U-Boot address.
Hello Simon,
why do we need this Babylonic confusion of addresses which do not point to valid memory and pointers that are not valid addresses where everybody is getting lost?
Simply use only addresses with there mmap'ed values and get rid of all conversions. Use your board file to adjust kernel_addr_r and the like to the address range that Linux offered you.
No one is getting lost. It has been working fine for a long time. Sandbox was introduced almost 7 years ago. It has been a huge benefit in terms of productivity and testing.
We are not necessarily running on Linux, but in any case, we get ugly addresses which expose the underlying machine architecture, which is not relevant for tests, and just introduces confusion. Sandbox is intended to be pure U-Boot, just like you might boot up on a 32-bit ARM or x86 machine.
The conversions existing in any case, since we must case between addresses and pointers. This just makes it explicit in a convenient, type-safe manner.
Regards, Simon
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;
In the sandbox environment we can not easily build efi stub binaries right now, so let's disable the respective test cases for the efi selftest suite.
Signed-off-by: Alexander Graf agraf@suse.de --- lib/efi_selftest/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile index 4fe404d88d..bf5c8199cb 100644 --- a/lib/efi_selftest/Makefile +++ b/lib/efi_selftest/Makefile @@ -41,7 +41,7 @@ endif
# TODO: As of v2018.01 the relocation code for the EFI application cannot # be built on x86_64. -ifeq ($(CONFIG_X86_64),) +ifeq ($(CONFIG_X86_64)$(CONFIG_SANDBOX),)
ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST),)
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
In the sandbox environment we can not easily build efi stub binaries right now, so let's disable the respective test cases for the efi selftest suite.
Signed-off-by: Alexander Graf agraf@suse.de
lib/efi_selftest/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
---
v1 -> v2:
- Clarify address vs pointer - include mapmem.h --- lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index e6a15bcb52..2107730ba5 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -9,6 +9,7 @@ #include <charset.h> #include <efi_loader.h> #include <malloc.h> +#include <mapmem.h> #include <fs.h>
/* GUID for file system information */ @@ -232,8 +233,10 @@ static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size, void *buffer) { loff_t actread; + /* fs_read expects buffer address, not pointer */ + uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);
- if (fs_read(fh->path, (ulong)buffer, fh->offset, + if (fs_read(fh->path, buffer_addr, fh->offset, *buffer_size, &actread)) return EFI_DEVICE_ERROR;
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- Clarify address vs pointer
- include mapmem.h
lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On 14.06.18 21:01, Simon Glass wrote:
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- Clarify address vs pointer
- include mapmem.h
lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I actually think that this patch tackles the problem the wrong way around. I've cooked up another one that converts fs_read() and fs_write() to instead take a pointer - which really is what most users of the API want in the first place:
https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814...
Alex
Hi Alex,
On 14 June 2018 at 13:51, Alexander Graf agraf@suse.de wrote:
On 14.06.18 21:01, Simon Glass wrote:
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- Clarify address vs pointer
- include mapmem.h
lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I actually think that this patch tackles the problem the wrong way around. I've cooked up another one that converts fs_read() and fs_write() to instead take a pointer - which really is what most users of the API want in the first place:
https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814...
Actually this is a pretty good exposition of why we don't want to use pointers everywhere. U-Boot uses addresses all over the place. Even a search for something as specific as "ulong addr" gets over 200 matches. If we go down this path we will end up changing a pretty fundamental part of U-Boot, and I don't think it is a good idea to do that. Addresses are easy to understand, can be added/subtracted without pointer arithmetic, can be easily converted to pointers as needed, can be 32-bits long on sandbox, etc.
So I think we should step back from the abyss here and stick with the way sandbox currently deals with addresses. It works well, is pretty easy to understand, allows debugging which is just as easy on sandbox as other archs (since they both uses addresses until basically the final access), the addresses are typically small values that start at 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
Here for example is the output from starting U-Boot with debugging in board_f.c (something I have turned on a lot when refactoring and debugging the init sequence):
U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
DRAM: Monitor len: 00395AB0 Ram size: 08000000 Ram top: 08000000 Reserving 3670k for U-Boot at: 07c6a000 Reserving 32776k for malloc() at: 05c68000 Reserving 120 Bytes for Board Info at: 05c67f88 Reserving 472 Bytes for Global Data at: 05c67db0 Reserving 4352 Bytes for FDT at: 05c66cb0 Reserving 0x3c8 Bytes for bootstage at: 05c668e8
RAM Configuration: Bank #0: 0 128 MiB
DRAM: 128 MiB New Stack Pointer is: 05c668d0 Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8 Relocation Offset is: 07c6a000 Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
If we use pointers we get things like this:
Reserving 3670k for U-Boot at: 00007f1b58c8b008
I just don't want to deal with 64-bit addresses when debugging things, and there really is no point. The map_sysmem() function provides a nice and easy way to cast an address to a pointer, and it works on all architectures.
Regards, Simon
On 14.06.18 23:35, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 13:51, Alexander Graf agraf@suse.de wrote:
On 14.06.18 21:01, Simon Glass wrote:
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- Clarify address vs pointer
- include mapmem.h
lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I actually think that this patch tackles the problem the wrong way around. I've cooked up another one that converts fs_read() and fs_write() to instead take a pointer - which really is what most users of the API want in the first place:
https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814...
Actually this is a pretty good exposition of why we don't want to use pointers everywhere. U-Boot uses addresses all over the place. Even a search for something as specific as "ulong addr" gets over 200 matches. If we go down this path we will end up changing a pretty fundamental part of U-Boot, and I don't think it is a good idea to do that. Addresses are easy to understand, can be added/subtracted without pointer arithmetic, can be easily converted to pointers as needed, can be 32-bits long on sandbox, etc.
I tend to disagree with this. Most code in U-Boot actually consumes pointers rather than addresses.
So I think we should step back from the abyss here and stick with the way sandbox currently deals with addresses. It works well, is pretty easy to understand, allows debugging which is just as easy on sandbox as other archs (since they both uses addresses until basically the final access), the addresses are typically small values that start at 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
Here for example is the output from starting U-Boot with debugging in board_f.c (something I have turned on a lot when refactoring and debugging the init sequence):
U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
DRAM: Monitor len: 00395AB0 Ram size: 08000000 Ram top: 08000000 Reserving 3670k for U-Boot at: 07c6a000 Reserving 32776k for malloc() at: 05c68000 Reserving 120 Bytes for Board Info at: 05c67f88 Reserving 472 Bytes for Global Data at: 05c67db0 Reserving 4352 Bytes for FDT at: 05c66cb0 Reserving 0x3c8 Bytes for bootstage at: 05c668e8
RAM Configuration: Bank #0: 0 128 MiB
DRAM: 128 MiB New Stack Pointer is: 05c668d0 Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8 Relocation Offset is: 07c6a000 Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
If we use pointers we get things like this:
Reserving 3670k for U-Boot at: 00007f1b58c8b008
I just don't want to deal with 64-bit addresses when debugging things, and there really is no point. The map_sysmem() function provides a nice and easy way to cast an address to a pointer, and it works on all architectures.
Ok, circling back to square 1. The main issue we're facing here is that most code in U-Boot does not really understand the difference between virtual and physical addresses - it just simply assumes a 1:1 map.
With sandbox, we do not have a 1:1 map anymore - any address is somewhere else than its pointer.
We have a few options:
1) Deal with pointers at random addresses throughout the code
I can see why you don't want this. I find ASLR generated addresses quite cumbersome to read as well.
2) Explicitly map RAM at VA 0x10000000, then do 1:1 map
This would be the best of all worlds still IMHO. That way we will have easily readable addresses (that are identical in 32bit and 64bit), but can still do a 1:1 map. The only thing we need to do is to introduce a linker section at 0x10000000.
3) Keep converting addresses to pointers
I'm afraid if we follow this path, we will always have arguments on where the correct boundaries are. If you start to enable debugging in core dm code and print out pointers to your dm objects, you will get pointer values today as well. Sooner or later we will always end up with pointers.
Alex
Hi Alex,
On 14 June 2018 at 15:55, Alexander Graf agraf@suse.de wrote:
On 14.06.18 23:35, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 13:51, Alexander Graf agraf@suse.de wrote:
On 14.06.18 21:01, Simon Glass wrote:
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- Clarify address vs pointer
- include mapmem.h
lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I actually think that this patch tackles the problem the wrong way around. I've cooked up another one that converts fs_read() and fs_write() to instead take a pointer - which really is what most users of the API want in the first place:
https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814...
Actually this is a pretty good exposition of why we don't want to use pointers everywhere. U-Boot uses addresses all over the place. Even a search for something as specific as "ulong addr" gets over 200 matches. If we go down this path we will end up changing a pretty fundamental part of U-Boot, and I don't think it is a good idea to do that. Addresses are easy to understand, can be added/subtracted without pointer arithmetic, can be easily converted to pointers as needed, can be 32-bits long on sandbox, etc.
I tend to disagree with this. Most code in U-Boot actually consumes pointers rather than addresses.
That might be true within individual components, but addresses are the common currency in U-Boot. See for example bootm, all the image-handling stuff including FIT, commands, device tree address properties. Pointers are more often used at the lowest level of code.
So I think we should step back from the abyss here and stick with the way sandbox currently deals with addresses. It works well, is pretty easy to understand, allows debugging which is just as easy on sandbox as other archs (since they both uses addresses until basically the final access), the addresses are typically small values that start at 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
Here for example is the output from starting U-Boot with debugging in board_f.c (something I have turned on a lot when refactoring and debugging the init sequence):
U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
DRAM: Monitor len: 00395AB0 Ram size: 08000000 Ram top: 08000000 Reserving 3670k for U-Boot at: 07c6a000 Reserving 32776k for malloc() at: 05c68000 Reserving 120 Bytes for Board Info at: 05c67f88 Reserving 472 Bytes for Global Data at: 05c67db0 Reserving 4352 Bytes for FDT at: 05c66cb0 Reserving 0x3c8 Bytes for bootstage at: 05c668e8
RAM Configuration: Bank #0: 0 128 MiB
DRAM: 128 MiB New Stack Pointer is: 05c668d0 Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8 Relocation Offset is: 07c6a000 Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
If we use pointers we get things like this:
Reserving 3670k for U-Boot at: 00007f1b58c8b008
I just don't want to deal with 64-bit addresses when debugging things, and there really is no point. The map_sysmem() function provides a nice and easy way to cast an address to a pointer, and it works on all architectures.
Ok, circling back to square 1. The main issue we're facing here is that most code in U-Boot does not really understand the difference between virtual and physical addresses - it just simply assumes a 1:1 map.
Traditionally that was true, but over the past 5 years quite a bit of code has been converted to work correctly with sandbox. Also, almost all *tested* code supports sandbox.
With sandbox, we do not have a 1:1 map anymore - any address is somewhere else than its pointer.
We have a few options:
- Deal with pointers at random addresses throughout the code
I can see why you don't want this. I find ASLR generated addresses quite cumbersome to read as well.
I'm not sure what this means.
- Explicitly map RAM at VA 0x10000000, then do 1:1 map
This would be the best of all worlds still IMHO. That way we will have easily readable addresses (that are identical in 32bit and 64bit), but can still do a 1:1 map. The only thing we need to do is to introduce a linker section at 0x10000000.
Linker section or mmap() is essentially the same thing. The former will presumably just fail to run (bus error?) if the address is not available. The latter will select a similar, available address.
- Keep converting addresses to pointers
I'm afraid if we follow this path, we will always have arguments on where the correct boundaries are. If you start to enable debugging in core dm code and print out pointers to your dm objects, you will get pointer values today as well. Sooner or later we will always end up with pointers.
The addresses should generally be converted just before use. Most APIs should use addresses rather than pointers, particularly those related to images, loaded data in memory, hardware peripheral addresses and booting.
Regards, Simon
On 06/20/2018 12:03 AM, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 15:55, Alexander Graf agraf@suse.de wrote:
On 14.06.18 23:35, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 13:51, Alexander Graf agraf@suse.de wrote:
On 14.06.18 21:01, Simon Glass wrote:
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote:
The fs_read() function wants to get an address rather than the pointer to a buffer.
So let's convert the passed buffer from pointer back a the address to make efi_loader on sandbox happier.
Signed-off-by: Alexander Graf agraf@suse.de
v1 -> v2:
- Clarify address vs pointer
- include mapmem.h
lib/efi_loader/efi_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I actually think that this patch tackles the problem the wrong way around. I've cooked up another one that converts fs_read() and fs_write() to instead take a pointer - which really is what most users of the API want in the first place:
https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814...
Actually this is a pretty good exposition of why we don't want to use pointers everywhere. U-Boot uses addresses all over the place. Even a search for something as specific as "ulong addr" gets over 200 matches. If we go down this path we will end up changing a pretty fundamental part of U-Boot, and I don't think it is a good idea to do that. Addresses are easy to understand, can be added/subtracted without pointer arithmetic, can be easily converted to pointers as needed, can be 32-bits long on sandbox, etc.
I tend to disagree with this. Most code in U-Boot actually consumes pointers rather than addresses.
That might be true within individual components, but addresses are the common currency in U-Boot. See for example bootm, all the image-handling stuff including FIT, commands, device tree address properties. Pointers are more often used at the lowest level of code.
Yes, but all counterparts of fs_read and APIs on the same level use pointers. And please take a few minutes to look at its users. 90% of them are using pointers too.
As I mentioned elsewhere already, going from address to pointer is something reasonable. Going from address to pointer usually a sign of bad design ;). If most users of fs_read() are already using pointers, it means that either all of its users are wrong or fs_read() is wrong. I think the latter is the case.
So I think we should step back from the abyss here and stick with the way sandbox currently deals with addresses. It works well, is pretty easy to understand, allows debugging which is just as easy on sandbox as other archs (since they both uses addresses until basically the final access), the addresses are typically small values that start at 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
Here for example is the output from starting U-Boot with debugging in board_f.c (something I have turned on a lot when refactoring and debugging the init sequence):
U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
DRAM: Monitor len: 00395AB0 Ram size: 08000000 Ram top: 08000000 Reserving 3670k for U-Boot at: 07c6a000 Reserving 32776k for malloc() at: 05c68000 Reserving 120 Bytes for Board Info at: 05c67f88 Reserving 472 Bytes for Global Data at: 05c67db0 Reserving 4352 Bytes for FDT at: 05c66cb0 Reserving 0x3c8 Bytes for bootstage at: 05c668e8
RAM Configuration: Bank #0: 0 128 MiB
DRAM: 128 MiB New Stack Pointer is: 05c668d0 Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8 Relocation Offset is: 07c6a000 Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
If we use pointers we get things like this:
Reserving 3670k for U-Boot at: 00007f1b58c8b008
I just don't want to deal with 64-bit addresses when debugging things, and there really is no point. The map_sysmem() function provides a nice and easy way to cast an address to a pointer, and it works on all architectures.
Ok, circling back to square 1. The main issue we're facing here is that most code in U-Boot does not really understand the difference between virtual and physical addresses - it just simply assumes a 1:1 map.
Traditionally that was true, but over the past 5 years quite a bit of code has been converted to work correctly with sandbox. Also, almost all *tested* code supports sandbox.
I think the difference is between infrastructure and device code. Most device code is still assuming 1:1 maps, but just doesn't get executed in sandbox.
With sandbox, we do not have a 1:1 map anymore - any address is somewhere else than its pointer.
We have a few options:
- Deal with pointers at random addresses throughout the code
I can see why you don't want this. I find ASLR generated addresses quite cumbersome to read as well.
I'm not sure what this means.
Address Space Layout Randomization. It's what causes these weird looking addresses ;).
- Explicitly map RAM at VA 0x10000000, then do 1:1 map
This would be the best of all worlds still IMHO. That way we will have easily readable addresses (that are identical in 32bit and 64bit), but can still do a 1:1 map. The only thing we need to do is to introduce a linker section at 0x10000000.
Linker section or mmap() is essentially the same thing. The former will presumably just fail to run (bus error?) if the address is not available. The latter will select a similar, available address.
No, they're inherently different things. When a process starts, its virtual address space is basically empty (apart from the vdso and a tiny chunk of ld). The linker is then responsible to ensure that all application defined memory regions are mapped. Only after that happened, it will map linked libraries (such as glibc, SDL, whatever) at random places in address space.
So with a linker section, we're basically guaranteed to always have memory live at the same spot. With mmap() there is a good chance we're overlapping with something that happened to get mapped there in between.
- Keep converting addresses to pointers
I'm afraid if we follow this path, we will always have arguments on where the correct boundaries are. If you start to enable debugging in core dm code and print out pointers to your dm objects, you will get pointer values today as well. Sooner or later we will always end up with pointers.
The addresses should generally be converted just before use. Most APIs should use addresses rather than pointers, particularly those related to images, loaded data in memory, hardware peripheral addresses and booting.
I don't think that's a reasonable definition for the API requirements to be honest. I think addresses make a lot of sense as long as you don't want to touch any data inside. You basically use them like a token. Once you've gone past that point - so you've run map_sysmem() on that address - we should stay in pointer land IMHO.
Alex
Hi Alex,
On 20 June 2018 at 03:31, Alexander Graf agraf@suse.de wrote:
On 06/20/2018 12:03 AM, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 15:55, Alexander Graf agraf@suse.de wrote:
On 14.06.18 23:35, Simon Glass wrote:
Hi Alex,
On 14 June 2018 at 13:51, Alexander Graf agraf@suse.de wrote:
On 14.06.18 21:01, Simon Glass wrote:
On 14 June 2018 at 12:22, Alexander Graf agraf@suse.de wrote: > > The fs_read() function wants to get an address rather than the > pointer to a buffer. > > So let's convert the passed buffer from pointer back a the address > to make efi_loader on sandbox happier. > > Signed-off-by: Alexander Graf agraf@suse.de > > --- > > v1 -> v2: > > - Clarify address vs pointer > - include mapmem.h > --- > lib/efi_loader/efi_file.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I actually think that this patch tackles the problem the wrong way around. I've cooked up another one that converts fs_read() and fs_write() to instead take a pointer - which really is what most users of the API want in the first place:
https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814...
Actually this is a pretty good exposition of why we don't want to use pointers everywhere. U-Boot uses addresses all over the place. Even a search for something as specific as "ulong addr" gets over 200 matches. If we go down this path we will end up changing a pretty fundamental part of U-Boot, and I don't think it is a good idea to do that. Addresses are easy to understand, can be added/subtracted without pointer arithmetic, can be easily converted to pointers as needed, can be 32-bits long on sandbox, etc.
I tend to disagree with this. Most code in U-Boot actually consumes pointers rather than addresses.
That might be true within individual components, but addresses are the common currency in U-Boot. See for example bootm, all the image-handling stuff including FIT, commands, device tree address properties. Pointers are more often used at the lowest level of code.
Yes, but all counterparts of fs_read and APIs on the same level use pointers. And please take a few minutes to look at its users. 90% of them are using pointers too.
Do you mean the callers of fs_read()? That's not the point. Of course there will be conversion from addresses to pointers, but I am trying to keep those at the 'edge', away from the core U-Boot code. Your patch is basically designed to avoid a pointer conversion (which you can't support) in the filesystem layer. Next you will be doing this in networking, display, etc. etc. There is just no need for this change.
As I mentioned elsewhere already, going from address to pointer is something reasonable. Going from address to pointer usually a sign of bad design ;). If most users of fs_read() are already using pointers, it means that either all of its users are wrong or fs_read() is wrong. I think the latter is the case.
By that logic you would have U-Boot not use addresses at all, and have pointers everywhere. It just isn't the way the code base is today.
So I think we should step back from the abyss here and stick with the way sandbox currently deals with addresses. It works well, is pretty easy to understand, allows debugging which is just as easy on sandbox as other archs (since they both uses addresses until basically the final access), the addresses are typically small values that start at 0 which much is easier to read than (e.g.) 00007f1b58c8b008.
Here for example is the output from starting U-Boot with debugging in board_f.c (something I have turned on a lot when refactoring and debugging the init sequence):
U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)
DRAM: Monitor len: 00395AB0 Ram size: 08000000 Ram top: 08000000 Reserving 3670k for U-Boot at: 07c6a000 Reserving 32776k for malloc() at: 05c68000 Reserving 120 Bytes for Board Info at: 05c67f88 Reserving 472 Bytes for Global Data at: 05c67db0 Reserving 4352 Bytes for FDT at: 05c66cb0 Reserving 0x3c8 Bytes for bootstage at: 05c668e8
RAM Configuration: Bank #0: 0 128 MiB
DRAM: 128 MiB New Stack Pointer is: 05c668d0 Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8 Relocation Offset is: 07c6a000 Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0
If we use pointers we get things like this:
Reserving 3670k for U-Boot at: 00007f1b58c8b008
I just don't want to deal with 64-bit addresses when debugging things, and there really is no point. The map_sysmem() function provides a nice and easy way to cast an address to a pointer, and it works on all architectures.
Ok, circling back to square 1. The main issue we're facing here is that most code in U-Boot does not really understand the difference between virtual and physical addresses - it just simply assumes a 1:1 map.
Traditionally that was true, but over the past 5 years quite a bit of code has been converted to work correctly with sandbox. Also, almost all *tested* code supports sandbox.
I think the difference is between infrastructure and device code. Most device code is still assuming 1:1 maps, but just doesn't get executed in sandbox.
If you mean the drivers, then yes I agree. The drivers typically get an address and map it to a pointer which they use from then on.
I do have some ideas about executing driver code in sandbox. We already build a lot of drivers in sandbox, but execute only a very few. But sandbox could absolutely provide a way to test driver code, with suitable plumbing. Of course, we need address mapping...:-)
With sandbox, we do not have a 1:1 map anymore - any address is somewhere else than its pointer.
We have a few options:
- Deal with pointers at random addresses throughout the code
I can see why you don't want this. I find ASLR generated addresses quite cumbersome to read as well.
I'm not sure what this means.
Address Space Layout Randomization. It's what causes these weird looking addresses ;).
No, I mean, I am not sure what your comment means. I know what ASLR is, but I just don't understand your comment as a whole. What problem are you trying to solve?
- Explicitly map RAM at VA 0x10000000, then do 1:1 map
This would be the best of all worlds still IMHO. That way we will have easily readable addresses (that are identical in 32bit and 64bit), but can still do a 1:1 map. The only thing we need to do is to introduce a linker section at 0x10000000.
Linker section or mmap() is essentially the same thing. The former will presumably just fail to run (bus error?) if the address is not available. The latter will select a similar, available address.
No, they're inherently different things. When a process starts, its virtual address space is basically empty (apart from the vdso and a tiny chunk of ld). The linker is then responsible to ensure that all application defined memory regions are mapped. Only after that happened, it will map linked libraries (such as glibc, SDL, whatever) at random places in address space.
So with a linker section, we're basically guaranteed to always have memory live at the same spot. With mmap() there is a good chance we're overlapping with something that happened to get mapped there in between.
I don't think so. The only calls to mmap() in U-Boot are in os.c so we can control use of this <4GB address. From my limited testing with 64-bit machines I always get a huge address for things where I don't specify a hint address.
- Keep converting addresses to pointers
I'm afraid if we follow this path, we will always have arguments on where the correct boundaries are. If you start to enable debugging in core dm code and print out pointers to your dm objects, you will get pointer values today as well. Sooner or later we will always end up with pointers.
The addresses should generally be converted just before use. Most APIs should use addresses rather than pointers, particularly those related to images, loaded data in memory, hardware peripheral addresses and booting.
I don't think that's a reasonable definition for the API requirements to be honest. I think addresses make a lot of sense as long as you don't want to touch any data inside. You basically use them like a token. Once you've gone past that point - so you've run map_sysmem() on that address - we should stay in pointer land IMHO.
Again, by that logic we would never uses addresses in U-Boot. Everything would be a pointer. Addresses have a number of advantages over pointers:
- easy to do arithmetic - don't need casts - easy to printf() without getting massive 16-digit hex strings all the time - can be 32-bits wide in sandbox even on a 64-bit machine
Probably some others...you yourself said you cannot do pointer arithmetic without casting to a uintptr_t first. I don't think that is true, but it is indicative of the messiness of dealing with pointers.
Regards, Simon
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
participants (3)
-
Alexander Graf -
Heinrich Schuchardt -
Simon Glass