sandbox: efi_loader support
Subsystems (5)
v4 cover sandbox: efi_loader support
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
Signature · 1 line
2.12.3
PATCH v4 1/21 efi: sandbox: Add distroboot support
From: Simon Glass <[email protected]>
With sandbox these values depend on the host system. Let's assume that it is x86_64 for now.
--- include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
include/config_distro_bootcmd.h +13 -0
@@ -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
Signature · 1 line
2.12.3
PATCH v4 2/21 efi: sandbox: Add relocation constants
In reply to Alexander GrafFrom: Simon Glass <[email protected]>
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.
--- lib/efi_loader/efi_runtime.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
lib/efi_loader/efi_runtime.c +12 -0
@@ -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
Signature · 1 line
2.12.3
PATCH v4 3/21 efi_loader: Use compiler constants for image loader
In reply to Alexander GrafThe 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.
--- lib/efi_loader/efi_image_loader.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
lib/efi_loader/efi_image_loader.c +6 -6
@@ -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 };
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 3/21 efi_loader: Use compiler constants for image loader
Quoted 10 lines
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 <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 4/21 efi_loader: Use map_sysmem() in bootefi command
In reply to Alexander GrafThe 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.
--- cmd/bootefi.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
cmd/bootefi.c +8 -5
@@ -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 );
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 4/21 efi_loader: Use map_sysmem() in bootefi command
Quoted 6 lines
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 <[email protected]>Reviewed-by Simon Glass <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 5/21 efi.h: Do not use config options
In reply to Alexander GrafCurrently 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.
--- include/efi.h | 17 ++++------------- lib/efi/Makefile | 4 ++-- 2 files changed, 6 insertions(+), 15 deletions(-)
include/efi.h +4 -13
@@ -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))
lib/efi/Makefile +2 -2
@@ -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
Signature · 1 line
2.12.3
PATCH v4 6/21 efi_loader: Allow SMBIOS tables in highmem
In reply to Alexander GrafWe 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.
--- lib/efi_loader/efi_smbios.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
lib/efi_loader/efi_smbios.c +9 -2
@@ -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
Signature · 1 line
2.12.3
PATCH v4 6/21 efi_loader: Allow SMBIOS tables in highmem
Hi Alex,
Quoted 4 lines · 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.
Does this actually happen?
Quoted 29 lines
Signed-off-by Alexander Graf <[email protected]>--- lib/efi_loader/efi_smbios.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)lib/efi_loader/efi_smbios.c +9
-3@@ -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 --
Quoted 2 lines
2.12.3
Regards,
Simon
PATCH v4 6/21 efi_loader: Allow SMBIOS tables in highmem
Quoted 8 lines · Simon Glass
Hi Alex,
Quoted 4 lines · 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.
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
PATCH v4 6/21 efi_loader: Allow SMBIOS tables in highmem
Hi Alex,
Quoted 12 lines · Alexander Graf
Quoted 10 lines · Simon Glass
Hi Alex,
Quoted 5 lines · 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.
Quoted 6 lines
Quoted 1 line
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
Alexander Graf
AppliedPATCH v4 6/21 efi_loader: Allow SMBIOS tables in highmem
In reply to Alexander GrafQuoted 6 lines
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 <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 7/21 sandbox: Map host memory for efi_loader
In reply to Alexander GrafWith 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.
---
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(+)
arch/sandbox/cpu/cpu.c +20 -0
@@ -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
Signature · 1 line
2.12.3
PATCH v4 7/21 sandbox: Map host memory for efi_loader
Hi Alex,
Quoted 13 lines · Alexander Graf
With
efi_loaderwe 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 <[email protected]>---
v1 -> v2:
Quoted 8 lines
- only compile
efi_add_known_memoryifefi_loaderis enabledv3 -> 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
PATCH v4 8/21 efi_loader: efi_allocate_pages is too restrictive
In reply to Alexander GrafFrom: Heinrich Schuchardt <[email protected]>
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.
--- lib/efi_loader/efi_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/efi_loader/efi_memory.c +1 -1
@@ -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 ;
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 8/21 efi_loader: efi_allocate_pages is too restrictive
Quoted 5 lines
From: Heinrich Schuchardt <[email protected]>
When running on the sandbox the stack is not necessarily at a higher memory address than the highest free memory.
Quoted 7 lines
There is no reason why the checking of the highest memory address should be more restrictive for
EFI_ALLOCATE_ANY_PAGESthan forEFI_ALLOCATE_MAX_ADDRESS.Signed-off-by Heinrich Schuchardt <[email protected]>[agraf: use -1ULL instead]Signed-off-by Alexander Graf <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 9/21 efi_loader: Disable miniapps on sandbox
In reply to Alexander GrafIn 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.
--- lib/efi_selftest/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/efi_selftest/Makefile +1 -1
@@ -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),)
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 9/21 efi_loader: Disable miniapps on sandbox
Quoted 6 lines
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 <[email protected]>Reviewed-by Simon Glass <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 10/21 fs: Convert fs_read/write to take buffer instead of address
In reply to Alexander GrafThe 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.
--- 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(-)
board/BuR/common/common.c +1 -1
@@ -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;
board/gdsys/p1022/controlcenterd-id.c +5 -5
@@ -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; }
cmd/mvebu/bubt.c +2 -2
@@ -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 ;
common/splash_source.c +3 -1
@@ -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 )
drivers/bootcount/bootcount_ext.c +6 -6
@@ -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 ];
drivers/fpga/zynqpl.c +5 -3
@@ -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);
fs/fs.c +10 -10
@@ -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 ;
include/fs.h +6 -6
@@ -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);/*
lib/efi_loader/efi_file.c +2 -4
@@ -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; }
Signature · 1 line
2.12.3
PATCH v4 10/21 fs: Convert fs_read/write to take buffer instead of address
Quoted 10 lines · Alexander Graf
The
fs_read()andfs_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 <[email protected]>
Quoted 11 lines
--- 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
PATCH v4 10/21 fs: Convert fs_read/write to take buffer instead of address
Quoted 23 lines · Simon Glass
Quoted 10 lines · Alexander Graf
The
fs_read()andfs_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 <[email protected]>Quoted 11 lines
--- 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(-)
Quoted 3 lines
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!
Signature · 8 lines
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>
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
In reply to Alexander GrafVarargs 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.
--- include/efi.h | 8 ++++++++ lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------ 2 files changed, 26 insertions(+), 18 deletions(-)
include/efi.h +8 -0
@@ -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 ;
lib/efi_loader/efi_boottime.c +18 -18
@@ -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); }
Signature · 1 line
2.12.3
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Hi Alex,
Quoted 7 lines · Alexander Graf
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms abi though, so we also need to make sure we usex86_64varargs 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.
Quoted 8 lines
This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests on
x86_64for me.Signed-off-by Alexander Graf <[email protected]>--- 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
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Quoted 13 lines · Simon Glass
Hi Alex,
Quoted 10 lines · Alexander Graf
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms abi though, so we also need to make sure we usex86_64varargs 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_64for me.
Quoted 7 lines
Quoted 5 lines
Signed-off-by Alexander Graf <[email protected]>--- 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
Alexander Graf
AppliedPATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
In reply to Alexander GrafQuoted 11 lines
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms abi though, so we also need to make sure we usex86_64varargs 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_64for me.Signed-off-by Alexander Graf <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Hi Alex,
Quoted 7 lines · Alexander Graf
Quoted 7 lines
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms abi though, so we also need to make sure we usex86_64varargs 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.
Quoted 7 lines
Quoted 4 lines
This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests on
x86_64for me.Signed-off-by Alexander Graf <[email protected]>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
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Quoted 13 lines · Bin Meng
Hi Alex,
Quoted 10 lines · Alexander Graf
Quoted 10 lines
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms abi though, so we also need to make sure we usex86_64varargs 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_64for me.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Alexander Graf <[email protected]>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
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Hi Alex,
Quoted 22 lines · Alexander Graf
Quoted 21 lines · Bin Meng
Hi Alex,
Quoted 17 lines · Alexander Graf
Quoted 9 lines
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms
abi though, so we also need to make sure we usex86_64varargs 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.
Quoted 5 lines
This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
onx86_64for me.Signed-off-by Alexander Graf <[email protected]>Thanks, applied to efi-next
Quoted 7 lines
Quoted 3 lines
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
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Quoted 22 lines · Bin Meng
Hi Alex,
Quoted 19 lines · Alexander Graf
Quoted 18 lines · Bin Meng
Hi Alex,
Quoted 15 lines · Alexander Graf
Quoted 8 lines
Varargs differ between sysv and ms abi. On
x86_64we have to follow the ms
abi though, so we also need to make sure we usex86_64varargs 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.
Quoted 5 lines
This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
onx86_64for me.Signed-off-by Alexander Graf <[email protected]>Thanks, applied to efi-next
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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
PATCH v4 11/21 efi_loader: Introduce ms abi vararg helpers
Hi Alex,
Quoted 39 lines · Alexander Graf
Quoted 38 lines · Bin Meng
Hi Alex,
Quoted 33 lines · Alexander Graf
Quoted 19 lines · Bin Meng
Hi Alex,
Quoted 15 lines · Alexander Graf
Quoted 15 lines
Varargs differ between sysv and ms abi. On
x86_64we have to follow the
ms
abi though, so we also need to make sure we usex86_64varargs 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
onx86_64for me.Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Alexander Graf <[email protected]>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.
Quoted 7 lines
Quoted 2 lines
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_64target, 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.
Quoted 3 lines
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)
Quoted 6 lines
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.
Quoted 7 lines
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
PATCH v4 12/21 efi: sandbox: Enable EFI loader for sandbox
In reply to Alexander GrafFrom: Simon Glass <[email protected]>
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.
--- lib/efi_loader/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/efi_loader/Kconfig +1 -1
@@ -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
Signature · 1 line
2.12.3
PATCH v4 13/21 distro: Move to compiler based target architecture determination
In reply to Alexander GrafThanks 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.
---
v3 -> v4:
- Compile fix for dts
--- include/config_distro_bootcmd.h | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-)
include/config_distro_bootcmd.h +11 -19
@@ -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
Signature · 1 line
2.12.3
PATCH v4 14/21 efi_loader: Move to compiler based target architecture determination
In reply to Alexander GrafThanks 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.
--- lib/efi_loader/efi_runtime.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
lib/efi_loader/efi_runtime.c +8 -18
@@ -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
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 14/21 efi_loader: Move to compiler based target architecture determination
Quoted 7 lines
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 <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 15/21 sandbox: Fix setjmp/longjmp
In reply to Alexander GrafIn 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.
--- 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(-)
arch/sandbox/cpu/cpu.c +0 -5
@@ -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);
arch/sandbox/cpu/os.c +2 -18
@@ -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); }
arch/sandbox/include/asm/setjmp.h +3 -1
@@ -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_ */
Signature · 1 line
2.12.3
PATCH v4 15/21 sandbox: Fix setjmp/longjmp
Hi Alex,
Quoted 6 lines · 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.
Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- 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
PATCH v4 15/21 sandbox: Fix setjmp/longjmp
Quoted 16 lines · Simon Glass
Hi Alex,
Quoted 6 lines · 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.
Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- 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(-)
Quoted 3 lines
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
PATCH v4 15/21 sandbox: Fix setjmp/longjmp
Hi Alex,
Quoted 20 lines · Alexander Graf
Quoted 18 lines · Simon Glass
Hi Alex,
Quoted 14 lines · 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 <[email protected]>--- 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(-)
Quoted 8 lines
Quoted 3 lines
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
PATCH v4 15/21 sandbox: Fix setjmp/longjmp
Quoted 24 lines · Simon Glass
Hi Alex,
Quoted 21 lines · Alexander Graf
Quoted 19 lines · Simon Glass
Hi Alex,
Quoted 6 lines · 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.
Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- 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?
Quoted 7 lines
Quoted 3 lines
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 thatlongjmp()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
PATCH v4 16/21 elf: Move x86 reloc defines to common elf.h
In reply to Alexander GrafWe 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.
--- 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
arch/x86/include/asm/elf.h deleted +0 -45
@@ -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
arch/x86/lib/reloc_ia32_efi.c +0 -1
@@ -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)
arch/x86/lib/reloc_x86_64_efi.c +0 -1
@@ -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)
include/elf.h +35 -0
@@ -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.
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 16/21 elf: Move x86 reloc defines to common elf.h
Quoted 7 lines
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 <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 17/21 efi_loader: Use common elf.h reloc defines
In reply to Alexander GrafNow that elf.h contains relocation defines for all architectures we care about, let's just include it unconditionally and refer to the defines.
--- lib/efi_loader/efi_runtime.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
lib/efi_loader/efi_runtime.c +3 -4
@@ -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
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 17/21 efi_loader: Use common elf.h reloc defines
Quoted 5 lines
Now that
elf.hcontains relocation defines for all architectures we care about, let's just include it unconditionally and refer to the defines.Signed-off-by Alexander Graf <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 18/21 efi: sandbox: Adjust memory usage for sandbox
In reply to Alexander GrafFrom: Simon Glass <[email protected]>
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.
--- lib/efi_loader/efi_memory.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
lib/efi_loader/efi_memory.c +11 -3
@@ -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 */
Signature · 1 line
2.12.3
PATCH v4 18/21 efi: sandbox: Adjust memory usage for sandbox
Hi Alex,
Quoted 9 lines · Alexander Graf
From: Simon Glass <[email protected]>
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 <[email protected]>[agraf: Remove map_sysmem() call]Signed-off-by Alexander Graf <[email protected]>
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
Alexander Graf
AppliedPATCH v4 18/21 efi: sandbox: Adjust memory usage for sandbox
In reply to Alexander GrafQuoted 9 lines
From: Simon Glass <[email protected]>
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 <[email protected]>[agraf: Remove map_sysmem() call]Signed-off-by Alexander Graf <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 19/21 sandbox: Allow to execute from RAM
In reply to Alexander GrafWith 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.
--- arch/sandbox/cpu/cpu.c | 14 ++++++++++++++ arch/sandbox/cpu/os.c | 14 ++++++++++++++ include/os.h | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+)
arch/sandbox/cpu/cpu.c +14 -0
@@ -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
arch/sandbox/cpu/os.c +14 -0
@@ -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);
include/os.h +19 -0
@@ -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
Signature · 1 line
2.12.3
PATCH v4 19/21 sandbox: Allow to execute from RAM
Hi Alex,
Quoted 11 lines · 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_loadercase, 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.
Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- 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
PATCH v4 19/21 sandbox: Allow to execute from RAM
Quoted 21 lines · Simon Glass
Hi Alex,
Quoted 11 lines · 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_loadercase, 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.Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- arch/sandbox/cpu/cpu.c | 14 ++++++++++++++ arch/sandbox/cpu/os.c | 14 ++++++++++++++ include/os.h | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+)
Quoted 2 lines
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
PATCH v4 19/21 sandbox: Allow to execute from RAM
Hi Alex,
Quoted 30 lines · Alexander Graf
Quoted 25 lines · Simon Glass
Hi Alex,
Quoted 14 lines · 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_loadercase, 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.Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- arch/sandbox/cpu/cpu.c | 14 ++++++++++++++ arch/sandbox/cpu/os.c | 14 ++++++++++++++ include/os.h | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+)Quoted 2 lines
What is this patch actually for? Does it make something work that did not before? Where is it called?
Quoted 7 lines
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 withPROT_EXECis 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
PATCH v4 19/21 sandbox: Allow to execute from RAM
Quoted 35 lines · Simon Glass
Hi Alex,
Quoted 32 lines · Alexander Graf
Quoted 23 lines · Simon Glass
Hi Alex,
Quoted 13 lines · 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_loadercase, 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.Quoted 7 lines
Signed-off-by Alexander Graf <[email protected]>--- arch/sandbox/cpu/cpu.c | 14 ++++++++++++++ arch/sandbox/cpu/os.c | 14 ++++++++++++++ include/os.h | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+)Quoted 2 lines
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 withPROT_EXECis a good thing in the normal sandbox use case, but for EFI we need to run from RAM ;).
Quoted 6 lines
Quoted 2 lines
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.
Quoted 2 lines
just call your code on start-up in
cpu.c. We don't need to connect to anallow_unaligned()thing, which is a misnomer in this case.
In that case we can just tell mmap() to map RAM with PROC_EXEC.
Alex
PATCH v4 20/21 sandbox: Always allocate aligned buffers
In reply to Alexander GrafIn 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.
--- arch/sandbox/cpu/os.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/sandbox/cpu/os.c +3 -2
@@ -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)
Signature · 1 line
2.12.3
PATCH v4 21/21 efi_loader: Expose U-Boot addresses in memory map for sandbox
In reply to Alexander GrafWe 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.
--- arch/sandbox/cpu/cpu.c | 19 ------------------- lib/efi_loader/efi_memory.c | 12 ++++++------ 2 files changed, 6 insertions(+), 25 deletions(-)
arch/sandbox/cpu/cpu.c +0 -19
@@ -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;
lib/efi_loader/efi_memory.c +6 -6
@@ -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; }
Signature · 1 line
2.12.3
Alexander Graf
AppliedPATCH v4 21/21 efi_loader: Expose U-Boot addresses in memory map for sandbox
Quoted 12 lines
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.
Quoted 4 lines
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 <[email protected]>
Thanks, applied to efi-next
Alex
PATCH v4 21/21 efi_loader: Expose U-Boot addresses in memory map for sandbox
In reply to Alexander GrafHi Alex,
Quoted 12 lines · 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.
Quoted 8 lines
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 <[email protected]>--- 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
PATCH v4 21/21 efi_loader: Expose U-Boot addresses in memory map for sandbox
Hi Simon,
Quoted 26 lines
Am 23.06.2018 um 06:01 schrieb Simon Glass <[email protected]>:
Hi Alex,
Quoted 13 lines · 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.Quoted 8 lines
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 <[email protected]>---
arch/sandbox/cpu/cpu.c| 19 -------------------lib/efi_loader/efi_memory.c| 12 ++++++------2 files changed, 6 insertions(+), 25 deletions(-)
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
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
PATCH v4 21/21 efi_loader: Expose U-Boot addresses in memory map for sandbox
Hi Alex,
Quoted 61 lines · Alexander Graf
Hi Simon,
Am 23.06.2018 um 06:01 schrieb Simon Glass <[email protected]>:
Hi Alex,
On 18 June 2018 at 09:23, Alexander Graf <[email protected]> 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 tobe 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 <[email protected]>---
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_loaderis 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.
Quoted 3 lines
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.
Quoted 11 lines
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
PATCH v4 21/21 efi_loader: Expose U-Boot addresses in memory map for sandbox
In reply to Alexander GrafHi Alex,
Quoted 13 lines · 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.
Quoted 8 lines
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 <[email protected]>--- 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.
Quoted 36 lines
arch/sandbox/cpu/cpu.c +0
-19@@ -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;lib/efi_loader/efi_memory.c +0
-0
lib/efi_loader/efi_memory.c efi_status_t efi_allocate_pages(int type, int memory_type, · line 329
Quoted 5 lines
lib/efi_loader/efi_memory.c +1
-1@@ -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.
Quoted 42 lines
lib/efi_loader/efi_memory.c +5
-6}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; } --
Quoted 2 lines
2.12.3
Regards,
Simon
v4 cover sandbox: efi_loader support
In reply to Alexander GrafQuoted 11 lines · Alexander Graf
This patch set augments Simon's patch set for
efi_loadersupport in sandbox[1], but cuts off the memory allocation scheme at a different point.According to the UEFI spec,
efi_allocate_pages()takes auint64_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_64grub.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
grub-core/kern/i386/tsc.c +0 -1
@@ -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
include/grub/i386/io.h +3 -10
@@ -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 */
v4 cover sandbox: efi_loader support
Hi Alex,
Quoted 15 lines · Alexander Graf
Quoted 5 lines · Alexander Graf
This patch set augments Simon's patch set for
efi_loadersupport in sandbox[1], but cuts off the memory allocation scheme at a different point.Quoted 7 lines
According to the UEFI spec,
efi_allocate_pages()takes auint64_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_64grub.efi doesn't work because that one requires inl instructions to work.
Quoted 8 lines
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
v4 cover sandbox: efi_loader support
Quoted 21 lines · Simon Glass
Hi Alex,
Quoted 18 lines · Alexander Graf
Quoted 11 lines · Alexander Graf
This patch set augments Simon's patch set for
efi_loadersupport in sandbox[1], but cuts off the memory allocation scheme at a different point.According to the UEFI spec,
efi_allocate_pages()takes auint64_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_64grub.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
Quoted 8 lines
Quoted 3 lines
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 signatureI'm running on
x86_64.
Are you using my patch set or yours? In mine this should be fixed.
Quoted 3 lines
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
v4 cover sandbox: efi_loader support
Hi Alex,
Quoted 31 lines · Alexander Graf
Quoted 30 lines · Simon Glass
Hi Alex,
Quoted 25 lines · Alexander Graf
Quoted 5 lines · Alexander Graf
This patch set augments Simon's patch set for
efi_loadersupport in sandbox[1], but cuts off the memory allocation scheme at a different point.Quoted 7 lines
According to the UEFI spec,
efi_allocate_pages()takes auint64_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_64grub.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.efiWhen 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.
Quoted 8 lines
Quoted 5 lines
OK thanks for that. The binary says this for me:
efi_load_pe: Invalid DOS signatureI'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?
Quoted 8 lines
Quoted 4 lines
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
v4 cover sandbox: efi_loader support
Quoted 42 lines · Simon Glass
Hi Alex,
Quoted 26 lines · Alexander Graf
Quoted 25 lines · Simon Glass
Hi Alex,
Quoted 22 lines · Alexander Graf
Quoted 11 lines · Alexander Graf
This patch set augments Simon's patch set for
efi_loadersupport in sandbox[1], but cuts off the memory allocation scheme at a different point.According to the UEFI spec,
efi_allocate_pages()takes auint64_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_64grub.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.efiWhen 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.Quoted 7 lines
Quoted 5 lines
OK thanks for that. The binary says this for me:
efi_load_pe: Invalid DOS signatureI'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?
Quoted 4 lines
Quoted 3 lines
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
Quoted 5 lines
Quoted 2 lines
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