Re: [PATCH V6 6/8] Add EFI stub for ARM
On 10 January 2014 17:30, Roy Franz roy.fr...@linaro.org wrote: This patch adds EFI stub support for the ARM Linux kernel. The EFI stub operates similarly to the x86 stub: it is a shim between the EFI firmware and the normal zImage entry point, and sets up the environment that the zImage is expecting. This includes loading the initrd (optionaly) and device tree from the system partition based on the kernel command line. The stub updates the device tree as necessary, adding entries for EFI runtime services. The PE/COFF MZ header at offset 0 results in the first instruction being an add that corrupts r5, which is not used by the zImage interface. Signed-off-by: Roy Franz roy.fr...@linaro.org Acked-by: Grant Likely grant.lik...@linaro.org --- [...] diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S new file mode 100644 index 000..dbb7101 --- /dev/null +++ b/arch/arm/boot/compressed/efi-header.S @@ -0,0 +1,117 @@ +@ Copyright (C) 2013 Linaro Ltd; roy.fr...@linaro.org +@ +@ This file contains the PE/COFF header that is part of the +@ EFI stub. +@ + + .org0x3c + @ + @ The PE header can be anywhere in the file, but for + @ simplicity we keep it together with the MSDOS header + @ The offset to the PE/COFF header needs to be at offset + @ 0x3C in the MSDOS header. + @ The only 2 fields of the MSDOS header that are used are this + @ PE/COFF offset, and the MZ bytes at offset 0x0. + @ + .long pe_header @ Offset to the PE header. + + .align 3 +pe_header: + .ascii PE + .short 0 + +coff_header: + .short 0x01c2 @ ARM or Thumb Could you explain why you are using 0x1c2 (Thumb) here and not 0x1c0 (ARM) ? Cheers, Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] Add EFI stub for ARM
On 15 January 2014 03:16, Roy Franz roy.fr...@linaro.org wrote: On Tue, Jan 14, 2014 at 5:47 PM, Roy Franz roy.fr...@linaro.org wrote: On Tue, Jan 14, 2014 at 1:05 AM, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 10 January 2014 17:30, Roy Franz roy.fr...@linaro.org wrote: This patch adds EFI stub support for the ARM Linux kernel. The EFI stub operates similarly to the x86 stub: it is a shim between the EFI firmware and the normal zImage entry point, and sets up the environment that the zImage is expecting. This includes loading the initrd (optionaly) and device tree from the system partition based on the kernel command line. The stub updates the device tree as necessary, adding entries for EFI runtime services. The PE/COFF MZ header at offset 0 results in the first instruction being an add that corrupts r5, which is not used by the zImage interface. Signed-off-by: Roy Franz roy.fr...@linaro.org Acked-by: Grant Likely grant.lik...@linaro.org --- [...] diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S new file mode 100644 index 000..dbb7101 --- /dev/null +++ b/arch/arm/boot/compressed/efi-header.S @@ -0,0 +1,117 @@ +@ Copyright (C) 2013 Linaro Ltd; roy.fr...@linaro.org +@ +@ This file contains the PE/COFF header that is part of the +@ EFI stub. +@ + + .org0x3c + @ + @ The PE header can be anywhere in the file, but for + @ simplicity we keep it together with the MSDOS header + @ The offset to the PE/COFF header needs to be at offset + @ 0x3C in the MSDOS header. + @ The only 2 fields of the MSDOS header that are used are this + @ PE/COFF offset, and the MZ bytes at offset 0x0. + @ + .long pe_header @ Offset to the PE header. + + .align 3 Btw you also have a whitespace error here. +pe_header: + .ascii PE + .short 0 + +coff_header: + .short 0x01c2 @ ARM or Thumb Could you explain why you are using 0x1c2 (Thumb) here and not 0x1c0 (ARM) ? Cheers, Ard. Nope. It should be 0x1c0. Roy OK, now I resolved the nagging feeling that I had already fixed this... Right now, the EDK2 UEFI implementation requires the machine type for ARM to be 0x1c2. I don't think that this is correct, but correcting this in EDK2 slipped through the cracks, but is now back on my todo list. I think that for now we should leave this as 0x1c2 so that the unpatched EDK2 builds will boot it, and some time after EDK2 is updated this can be changed. I'll work on a patch for EDK2 and get the discussion going on that list to resolve this in EDK2. OK. I have updated sbsigntool (Linaro's version) so it supports either, but I agree that using the ARM constant is the correct way. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/22] arm: efistub: ignore dtb= when UEFI SecureBoot is enabled
On 5 February 2014 18:04, Leif Lindholm leif.lindh...@linaro.org wrote: From: Ard Biesheuvel ard.biesheu...@linaro.org Loading unauthenticated FDT blobs directly from storage is a security hazard, so this should only be allowed when running with UEFI Secure Boot disabled. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Signed-off-by: Leif Lindholm leif.lindh...@linaro.org --- drivers/firmware/efi/arm-stub.c|4 +++- drivers/firmware/efi/efi-stub-helper.c | 24 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/efi/arm-stub.c b/drivers/firmware/efi/arm-stub.c index b505fde..c651082 100644 --- a/drivers/firmware/efi/arm-stub.c +++ b/drivers/firmware/efi/arm-stub.c @@ -95,7 +95,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table, /* Load a device tree from the configuration table, if present. */ fdt_addr = (uintptr_t)get_fdt(sys_table); - if (!fdt_addr) { + if (efi_secureboot_enabled(sys_table)) + pr_efi(sys_table, UEFI Secure Boot is enabled, ignoring dtb= commandline option.\n); I am pretty sure my original patch had braces on both branches of the if () :-) Also, I think the precedence is backward here: dtb= should trump config table, not the other way around. -- Ard. + else if (!fdt_addr) { status = handle_cmdline_files(sys_table, image, cmdline_ptr, dtb=, ~0UL, (unsigned long *)fdt_addr, diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c index 2ee69ea..6221be7 100644 --- a/drivers/firmware/efi/efi-stub-helper.c +++ b/drivers/firmware/efi/efi-stub-helper.c @@ -721,3 +721,27 @@ static char *efi_convert_cmdline(efi_system_table_t *sys_table_arg, *cmd_line_len = options_bytes; return (char *)cmdline_addr; } + +static int __init efi_secureboot_enabled(efi_system_table_t *sys_table_arg) +{ + static efi_guid_t const var_guid __initconst = EFI_GLOBAL_VARIABLE_GUID; + static efi_char16_t const var_name[] __initconst = { + 'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 }; + + efi_get_variable_t *f_getvar = sys_table_arg-runtime-get_variable; + unsigned long size = sizeof(u8); + efi_status_t status; + u8 val; + + status = efi_call_phys5(f_getvar, (efi_char16_t *)var_name, + (efi_guid_t *)var_guid, NULL, size, val); + + switch (status) { + case EFI_SUCCESS: + return val; + case EFI_NOT_FOUND: + return 0; + default: + return 1; + } +} -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: EFI_STUB fails to boot non-EFI on arm64
On 23 May 2014 17:45, Leif Lindholm leif.lindh...@linaro.org wrote: On Fri, May 23, 2014 at 05:17:39PM +0200, Ard Biesheuvel wrote: Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init(). Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro. Considering that a) the raw Image loader and the stub enter the kernel through different entry points (i.e., offset #0 and whatever entry point is specified in the PE/COFF header, respectively), and b) there is no decompressor etc involved so we jump straight into the kernel startup code c) head.S already deals with a similar problem, i.e., storing the CPU boot mode I would assume it shouldn't be so difficult to set a bit somewhere indicating which case we are dealing with? That would certainly be possible, but what would be the benefit of having two separate mechanisms for determining whether we are booting via UEFI - which could potentially end up disagreeing? Yeah, you're right. Also, the ARM requirements are sufficiently different (as Roy points out) that the presence of the FDT nodes is the most reliable indicator of whether you can proceed booting in EFI mode. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 1/4] efi/x86: efistub: move shared dependencies to asm/efi.h
This moves definitions depended upon both by code under arch/x86/boot and under drivers/firmware/efi to asm/efi.h. This is in preparation of turning the stub code under drivers/firmware/efi into a static library. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/x86/boot/compressed/eboot.c | 5 + arch/x86/boot/compressed/eboot.h | 16 arch/x86/include/asm/efi.h | 25 + 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 0331d765c2bb..2fd5e2643623 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -19,10 +19,7 @@ static efi_system_table_t *sys_table; -static struct efi_config *efi_early; - -#define efi_call_early(f, ...) \ - efi_early-call(efi_early-f, __VA_ARGS__); +struct efi_config *efi_early; #define BOOT_SERVICES(bits)\ static void setup_boot_services##bits(struct efi_config *c)\ diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h index c88c31ecad12..d487e727f1ec 100644 --- a/arch/x86/boot/compressed/eboot.h +++ b/arch/x86/boot/compressed/eboot.h @@ -103,20 +103,4 @@ struct efi_uga_draw_protocol { void *blt; }; -struct efi_config { - u64 image_handle; - u64 table; - u64 allocate_pool; - u64 allocate_pages; - u64 get_memory_map; - u64 free_pool; - u64 free_pages; - u64 locate_handle; - u64 handle_protocol; - u64 exit_boot_services; - u64 text_output; - efi_status_t (*call)(unsigned long, ...); - bool is64; -} __packed; - #endif /* BOOT_COMPRESSED_EBOOT_H */ diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 1eb5f6433ad8..55059a50a01f 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -156,6 +156,31 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( return EFI_SUCCESS; } #endif /* CONFIG_EFI_MIXED */ + + +/* arch specific definitions used by the stub code */ + +struct efi_config { + u64 image_handle; + u64 table; + u64 allocate_pool; + u64 allocate_pages; + u64 get_memory_map; + u64 free_pool; + u64 free_pages; + u64 locate_handle; + u64 handle_protocol; + u64 exit_boot_services; + u64 text_output; + efi_status_t (*call)(unsigned long, ...); + bool is64; +} __packed; + +extern struct efi_config *efi_early; + +#define efi_call_early(f, ...) \ + efi_early-call(efi_early-f, __VA_ARGS__); + #else /* * IF EFI is not configured, have the EFI calls return -ENOSYS. -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 3/4] efi: efistub: refactor stub components
In order to move from the #include ../../../x.c anti-pattern used by both the x86 and arm64 versions of the stub to a static library linked into either the kernel proper (arm64) or a separate boot executable (x86), there is some prepatory work required. This patch does the following: - move forward declarations of functions shared between the arch specific and the generic parts of the stub to include/linux/efi.h - move forward declarations of functions shared between various .c files of the generic stub code to a new local header file called efistub.h - add #includes to all .c files which were formerly relying on the #includor to include the correct header files - remove all static modifiers from functions which will need to be externally visible once we move to a static library Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 29 - arch/x86/boot/compressed/eboot.c | 13 +++--- drivers/firmware/efi/arm-stub.c| 18 ++--- drivers/firmware/efi/efi-stub-helper.c | 74 +- drivers/firmware/efi/efistub.h | 42 +++ drivers/firmware/efi/fdt.c | 20 + include/linux/efi.h| 42 +++ 7 files changed, 157 insertions(+), 81 deletions(-) create mode 100644 drivers/firmware/efi/efistub.h diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 23cbde4324b1..e4999021b07d 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -11,36 +11,21 @@ */ #include linux/efi.h #include asm/efi.h -#include linux/libfdt.h #include asm/sections.h -static void efi_char16_printk(efi_system_table_t *sys_table_arg, - efi_char16_t *str); - -static efi_status_t efi_open_volume(efi_system_table_t *sys_table, - void *__image, void **__fh); -static efi_status_t efi_file_close(void *handle); - -static efi_status_t -efi_file_read(void *handle, unsigned long *size, void *addr); - -static efi_status_t -efi_file_size(efi_system_table_t *sys_table, void *__fh, - efi_char16_t *filename_16, void **handle, u64 *file_sz); - /* Include shared EFI stub code */ #include ../../../drivers/firmware/efi/efi-stub-helper.c #include ../../../drivers/firmware/efi/fdt.c #include ../../../drivers/firmware/efi/arm-stub.c -static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, - unsigned long *image_addr, - unsigned long *image_size, - unsigned long *reserve_addr, - unsigned long *reserve_size, - unsigned long dram_base, - efi_loaded_image_t *image) +efi_status_t handle_kernel_image(efi_system_table_t *sys_table, +unsigned long *image_addr, +unsigned long *image_size, +unsigned long *reserve_addr, +unsigned long *reserve_size, +unsigned long dram_base, +efi_loaded_image_t *image) { efi_status_t status; unsigned long kernel_size, kernel_memsize = 0; diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 2fd5e2643623..d338c134c659 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -45,8 +45,7 @@ static void setup_boot_services##bits(struct efi_config *c) \ BOOT_SERVICES(32); BOOT_SERVICES(64); -static void efi_printk(efi_system_table_t *, char *); -static void efi_char16_printk(efi_system_table_t *, efi_char16_t *); +void efi_char16_printk(efi_system_table_t *, efi_char16_t *); static efi_status_t __file_size32(void *__fh, efi_char16_t *filename_16, @@ -153,7 +152,7 @@ grow: return status; } -static efi_status_t +efi_status_t efi_file_size(efi_system_table_t *sys_table, void *__fh, efi_char16_t *filename_16, void **handle, u64 *file_sz) { @@ -163,7 +162,7 @@ efi_file_size(efi_system_table_t *sys_table, void *__fh, return __file_size32(__fh, filename_16, handle, file_sz); } -static inline efi_status_t +efi_status_t efi_file_read(void *handle, unsigned long *size, void *addr) { unsigned long func; @@ -181,7 +180,7 @@ efi_file_read(void *handle, unsigned long *size, void *addr) } } -static inline efi_status_t efi_file_close(void *handle) +efi_status_t efi_file_close(void *handle) { if (efi_early-is64) { efi_file_handle_64_t *fh = handle; @@ -246,7 +245,7 @@ static inline efi_status_t __open_volume64(void *__image, void **__fh) return status; } -static inline efi_status_t +efi_status_t efi_open_volume
Re: [RFC PATCH 4/4] efi: efistub: convert into static library
On 16 June 2014 17:14, Ard Biesheuvel ard.biesheu...@linaro.org wrote: This patch changes both x86 and arm64 efistub implementations from #including shared .c files under drivers/firmware/efi to building the shared code as a static library. The x86 code uses a stub built into the boot executable which uncompresses the kernel at boot time. In this case, the library is linked into the decompressor. In the arm64 case, the stub is part of the kernel proper so the library is linked into the kernel proper as well. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/Kconfig | 1 + arch/arm64/Makefile| 1 + arch/arm64/kernel/efi-stub.c | 5 - arch/x86/boot/compressed/Makefile | 3 +- arch/x86/boot/compressed/eboot.c | 2 - drivers/firmware/efi/Kconfig | 3 + drivers/firmware/efi/Makefile | 2 +- drivers/firmware/efi/arm-stub.c| 284 --- drivers/firmware/efi/efi-stub-helper.c | 632 - drivers/firmware/efi/efistub.h | 42 -- drivers/firmware/efi/fdt.c | 279 --- drivers/firmware/efi/libstub/Makefile | 5 + drivers/firmware/efi/libstub/arm-stub.c| 284 +++ drivers/firmware/efi/libstub/efi-stub-helper.c | 632 + drivers/firmware/efi/libstub/efistub.h | 42 ++ drivers/firmware/efi/libstub/fdt.c | 279 +++ 16 files changed, 1250 insertions(+), 1246 deletions(-) delete mode 100644 drivers/firmware/efi/arm-stub.c delete mode 100644 drivers/firmware/efi/efi-stub-helper.c delete mode 100644 drivers/firmware/efi/efistub.h delete mode 100644 drivers/firmware/efi/fdt.c create mode 100644 drivers/firmware/efi/libstub/Makefile create mode 100644 drivers/firmware/efi/libstub/arm-stub.c create mode 100644 drivers/firmware/efi/libstub/efi-stub-helper.c create mode 100644 drivers/firmware/efi/libstub/efistub.h create mode 100644 drivers/firmware/efi/libstub/fdt.c Oops, forgot to enable diff.renames ... diffstat is actually arch/arm64/Kconfig | 1 + arch/arm64/Makefile | 1 + arch/arm64/kernel/efi-stub.c | 5 - arch/x86/boot/compressed/Makefile| 3 ++- arch/x86/boot/compressed/eboot.c | 2 -- drivers/firmware/efi/Kconfig | 3 +++ drivers/firmware/efi/Makefile| 2 +- drivers/firmware/efi/libstub/Makefile| 5 + drivers/firmware/efi/{ = libstub}/arm-stub.c| 0 drivers/firmware/efi/{ = libstub}/efi-stub-helper.c | 0 drivers/firmware/efi/{ = libstub}/efistub.h | 0 drivers/firmware/efi/{ = libstub}/fdt.c | 0 12 files changed, 13 insertions(+), 9 deletions(-) create mode 100644 drivers/firmware/efi/libstub/Makefile rename drivers/firmware/efi/{ = libstub}/arm-stub.c (100%) rename drivers/firmware/efi/{ = libstub}/efi-stub-helper.c (100%) rename drivers/firmware/efi/{ = libstub}/efistub.h (100%) rename drivers/firmware/efi/{ = libstub}/fdt.c (100%) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 7295419165e1..1ba4b5eae886 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -298,6 +298,7 @@ config EFI select LIBFDT select UCS2_STRING select EFI_PARAMS_FROM_FDT + select EFI_ARMSTUB default y help This option provides support for runtime services provided diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 8185a913c5ed..bb8f21a626c0 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -48,6 +48,7 @@ core-$(CONFIG_XEN) += arch/arm64/xen/ core-$(CONFIG_CRYPTO) += arch/arm64/crypto/ libs-y := arch/arm64/lib/ $(libs-y) libs-y += $(LIBGCC) +libs-$(CONFIG_EFI) += drivers/firmware/efi/libstub/ # Default target when executing plain make KBUILD_IMAGE := Image.gz diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index e4999021b07d..12456a7d3fa2 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -13,11 +13,6 @@ #include asm/efi.h #include asm/sections.h -/* Include shared EFI stub code */ -#include ../../../drivers/firmware/efi/efi-stub-helper.c -#include ../../../drivers/firmware/efi/fdt.c -#include ../../../drivers/firmware/efi/arm-stub.c - efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 0fcd9133790c..7a801a310e37 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -33,7 +33,8 @@ VMLINUX_OBJS = $(obj
Re: [PATCH] efi: fix pointer type errors in fdt_uefi_find_params()
On 18 June 2014 11:02, Matt Fleming m...@console-pimps.org wrote: On Fri, 13 Jun, at 04:54:59PM, Ard Biesheuvel wrote: Fix two instances of pointer type errors, a harmless one where a const void* value is assigned to a non-const void* variable, and a not-so-harmless one where we pass a pointer to unsigned long where a pointer to int is expected. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- drivers/firmware/efi/efi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index cd36deb619fa..fe737832a882 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -353,8 +353,8 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname, int depth, void *data) { struct param_info *info = data; - void *prop, *dest; - unsigned long len; + void const *prop, *dest; + int len; u64 val; int i; const void *? You say potato, I say po-tah-to? But seriously, whichever you prefer ... Regards, Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efi/arm64: efistub: remove local copy of linux_banner
On 18 June 2014 10:50, Matt Fleming m...@console-pimps.org wrote: On Fri, 13 Jun, at 01:11:51PM, Ard Biesheuvel wrote: The shared efistub code for ARM and arm64 contains a local copy of linux_banner, allowing it to be referenced from separate executables such as the ARM decompressor. However, this introduces a dependency on generated header files, causing unnecessary rebuilds of the stub itself and, in case of arm64, vmlinux which contains it. On arm64, the copy is not actually needed since we can reference the original symbol directly, and as it turns out, there may be better ways to deal with this for ARM as well, so let's remove it from the shared code. If it still needs to be reintroduced for ARM later, it should live under arch/arm anyway and not in shared code. I remember making some similar arguments when this patch was first introduced. From looking at my notes, the patch rationale was based on some DT binding discussion where people wanted an explicit way to identify the kernel they were booting and everyone agreed upon this string - I don't know which discussion, I don't have that data. I'm more than happy to delete this from the shared code, I never wanted it to go in in the first place. But could someone please give me some details as to why this change is safe and isn't going to break ARM/ARM64? How does this affect DT bindings? This just removes the duplicate definition of linux_banner, *not* the reference to it a couple of lines down. IOW, the DT bindings and everything behind it are not affected at all by this patch. The reason it works fine for arm64 is that its stub is part of the kernel proper, so the reference will bind to the global definition instead. What will be affected is 32-bit ARM, as its stub is not part of the kernel proper. In this case, we will propose an alternative [and better] way of allowing the stub to reference linux_banner, i.e., without having to redefine it at the C level. We will take this into account when we propose the next round of efistub patches for ARM. Regards, Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efi: fix pointer type errors in fdt_uefi_find_params()
On 18 June 2014 11:28, Matt Fleming m...@console-pimps.org wrote: On Wed, 18 Jun, at 11:06:09AM, Ard Biesheuvel wrote: You say potato, I say po-tah-to? But seriously, whichever you prefer ... $ git grep const void | wc -l 4441 $ git grep void const | wc -l 50 I say potato, you say tasty carbohydrate ball But yeah, it's not a big deal and I can fix this up when applying. Actually, it appears I messed up the subject line too: s/fdt_uefi_find_params/fdt_find_uefi_params/ Cheers, Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code
In order for other archs (such as arm64) to be able to reuse the virtual mode function call wrappers, move them to drivers/firmware/efi/runtime.c. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/x86/platform/efi/efi.c| 144 +-- drivers/firmware/efi/Makefile | 2 +- drivers/firmware/efi/runtime.c | 167 + include/linux/efi.h| 2 + 4 files changed, 172 insertions(+), 143 deletions(-) create mode 100644 drivers/firmware/efi/runtime.c diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 87fc96bcc13c..36d0835210c3 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -104,130 +104,6 @@ static int __init setup_storage_paranoia(char *arg) } early_param(efi_no_storage_paranoia, setup_storage_paranoia); -static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(get_time, tm, tc); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_set_time(efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(set_time, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, -efi_bool_t *pending, -efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(get_wakeup_time, enabled, pending, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(set_wakeup_time, enabled, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_get_variable(efi_char16_t *name, - efi_guid_t *vendor, - u32 *attr, - unsigned long *data_size, - void *data) -{ - return efi_call_virt(get_variable, -name, vendor, attr, -data_size, data); -} - -static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, - efi_char16_t *name, - efi_guid_t *vendor) -{ - return efi_call_virt(get_next_variable, -name_size, name, vendor); -} - -static efi_status_t virt_efi_set_variable(efi_char16_t *name, - efi_guid_t *vendor, - u32 attr, - unsigned long data_size, - void *data) -{ - return efi_call_virt(set_variable, -name, vendor, attr, -data_size, data); -} - -static efi_status_t virt_efi_query_variable_info(u32 attr, -u64 *storage_space, -u64 *remaining_space, -u64 *max_variable_size) -{ - if (efi.runtime_version EFI_2_00_SYSTEM_TABLE_REVISION) - return EFI_UNSUPPORTED; - - return efi_call_virt(query_variable_info, attr, storage_space, -remaining_space, max_variable_size); -} - -static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) -{ - return efi_call_virt(get_next_high_mono_count, count); -} - -static void virt_efi_reset_system(int reset_type, - efi_status_t status, - unsigned long data_size, - efi_char16_t *data) -{ - __efi_call_virt(reset_system, reset_type, status, - data_size, data); -} - -static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, - unsigned long count, - unsigned long sg_list) -{ - if (efi.runtime_version EFI_2_00_SYSTEM_TABLE_REVISION) - return EFI_UNSUPPORTED; - - return efi_call_virt(update_capsule, capsules, count, sg_list); -} - -static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, - unsigned long count
[PATCH 2/2] efi/arm64: preserve NEON registers on UEFI runtime services calls
UEFI Runtime Services function calls may clobber the contents of the NEON register file, so we need to make sure that we preserve/restore them when performing such a function call. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/efi.h | 21 + arch/arm64/kernel/efi.c | 14 +- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index 5a46c4e7f539..375ba342dca6 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -2,6 +2,7 @@ #define _ASM_EFI_H #include asm/io.h +#include asm/neon.h #ifdef CONFIG_EFI extern void efi_init(void); @@ -11,4 +12,24 @@ extern void efi_idmap_init(void); #define efi_idmap_init() #endif +#define efi_call_virt(f, ...) \ +({ \ + efi_##f##_t *__f = efi.systab-runtime-f; \ + efi_status_t __s; \ + \ + kernel_neon_begin();\ + __s = __f(__VA_ARGS__); \ + kernel_neon_end(); \ + __s;\ +}) + +#define __efi_call_virt(f, ...) \ +({ \ + efi_##f##_t *__f = efi.systab-runtime-f; \ + \ + kernel_neon_begin();\ + __f(__VA_ARGS__); \ + kernel_neon_end(); \ +}) + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 14db1f6e8d7f..56c3327bbf79 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -449,19 +449,7 @@ static int __init arm64_enter_virtual_mode(void) /* Set up runtime services function pointers */ runtime = efi.systab-runtime; - efi.get_time = runtime-get_time; - efi.set_time = runtime-set_time; - efi.get_wakeup_time = runtime-get_wakeup_time; - efi.set_wakeup_time = runtime-set_wakeup_time; - efi.get_variable = runtime-get_variable; - efi.get_next_variable = runtime-get_next_variable; - efi.set_variable = runtime-set_variable; - efi.query_variable_info = runtime-query_variable_info; - efi.update_capsule = runtime-update_capsule; - efi.query_capsule_caps = runtime-query_capsule_caps; - efi.get_next_high_mono_count = runtime-get_next_high_mono_count; - efi.reset_system = runtime-reset_system; - + efi_native_runtime_setup(); set_bit(EFI_RUNTIME_SERVICES, efi.flags); return 0; -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] efi: preserve NEON registers on UEFI services calls
The current UEFI implementation for arm64 fails to preserve/restore the contents of the NEON register file, which may result in data corruption, especially now that those contents are lazily restored for user processes. This series proposes to fix this by wrapping all runtime services calls, and adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers. The first patch moves the existing x86 versions of those wrappers to generic code, so that the second patch can easily enable them by supplying a definition for efi_call_virt and adding a call to efi_native_runtime_setup(). Ard Biesheuvel (2): efi/x86: move UEFI Runtime Services wrappers to generic code efi/arm64: preserve NEON registers on UEFI runtime services calls arch/arm64/include/asm/efi.h | 21 ++ arch/arm64/kernel/efi.c| 14 +--- arch/x86/platform/efi/efi.c| 144 +-- drivers/firmware/efi/Makefile | 2 +- drivers/firmware/efi/runtime.c | 167 + include/linux/efi.h| 2 + 6 files changed, 194 insertions(+), 156 deletions(-) create mode 100644 drivers/firmware/efi/runtime.c -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] efi: preserve NEON registers on UEFI services calls
On 23 June 2014 16:18, Ard Biesheuvel ard.biesheu...@linaro.org wrote: The current UEFI implementation for arm64 fails to preserve/restore the contents of the NEON register file, which may result in data corruption, especially now that those contents are lazily restored for user processes. This series proposes to fix this by wrapping all runtime services calls, and adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers. The first patch moves the existing x86 versions of those wrappers to generic code, so that the second patch can easily enable them by supplying a definition for efi_call_virt and adding a call to efi_native_runtime_setup(). CC'ing Olivier and Mark (with correct email address this time). Also, as an additional note, the UEFI spec section 2.3.6.4 mandates that 'any additional execution state context' should be saved and restored by the callee, which would imply that doing it in the kernel is redundant. But current implementations of Tianocore/EDK2 don't seem to honor that requirement, and considering GCC's tendency to spill state to FPSIMD registers, we may choose to do so anyway to be on the safe side. -- Ard. Ard Biesheuvel (2): efi/x86: move UEFI Runtime Services wrappers to generic code efi/arm64: preserve NEON registers on UEFI runtime services calls arch/arm64/include/asm/efi.h | 21 ++ arch/arm64/kernel/efi.c| 14 +--- arch/x86/platform/efi/efi.c| 144 +-- drivers/firmware/efi/Makefile | 2 +- drivers/firmware/efi/runtime.c | 167 + include/linux/efi.h| 2 + 6 files changed, 194 insertions(+), 156 deletions(-) create mode 100644 drivers/firmware/efi/runtime.c -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/4] efi: efistub: convert into static library
Hey Matt, On 25 jun. 2014, at 22:34, Matt Fleming m...@console-pimps.org wrote: On Mon, 16 Jun, at 05:14:48PM, Ard Biesheuvel wrote: This patch changes both x86 and arm64 efistub implementations from #including shared .c files under drivers/firmware/efi to building the shared code as a static library. The x86 code uses a stub built into the boot executable which uncompresses the kernel at boot time. In this case, the library is linked into the decompressor. In the arm64 case, the stub is part of the kernel proper so the library is linked into the kernel proper as well. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Sorry for the delay in reviewing this. Conceptually, this is a nice cleanup. Unfortunately, I'm seeing the following build error, drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o): In function `efi_printk': efi-stub-helper.c:(.text+0x1): undefined reference to `__fentry__' drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o): In function `efi_get_memory_map': efi-stub-helper.c:(.text+0x7f): undefined reference to `__fentry__' drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o): In function `efi_high_alloc': efi-stub-helper.c:(.text+0x1ea): undefined reference to `__fentry__' drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o): In function `efi_low_alloc': efi-stub-helper.c:(.text+0x3dd): undefined reference to `__fentry__' drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o): In function `efi_free': efi-stub-helper.c:(.text+0x59f): undefined reference to `__fentry__' drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o):efi-stub-helper.c:(.text+0x5ec): more undefined references to `__fentry__' follow drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o):(.data+0x260): undefined reference to `__gcov_merge_add' drivers/firmware/efi/libstub/lib.a(efi-stub-helper.o): In function `_GLOBAL__sub_I_65535_0_efi_printk': efi-stub-helper.c:(.text.startup+0xc): undefined reference to `__gcov_init' make[3]: *** [arch/x86/boot/compressed/vmlinux] Error 1 make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 2 make[1]: *** [bzImage] Error 2 This is because we use different CFLAGS for the EFI boot stub on x86. Take a look at arch/x86/boot/compressed/Makefile to see what I mean. Meh, I did try building it for x86, but apparently my .config does not have ftrace etc enabled. What I'd suggest is that you simply update the efi-stub-helper.c #include path in the x86 EFI boot stub in this series, and only roll out the awesome new libstub for arm64. I'll take care of doing the necessary CFLAG munging for x86 when I have some spare time. Could you do a v2? That is, unless you really want to make it work for x86 ;-) If this is primarily about disabling instrumentation stuff, then we will also be needing it for ARM anyway, and I don't think it will hurt to just disable it for all archs (including arm64). So I am happy to do a proper v2 that covers x86 as well. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [efi:next 6/7] drivers/firmware/efi/runtime.c:19:21: fatal error: asm/efi.h: No such file or directory
On 25 June 2014 23:53, kbuild test robot fengguang...@intel.com wrote: tree: git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next head: 8922cc07b558b167bc8f9e176c1a0b49acc5694f commit: 012e96bf2f05b3652ef02cf3df80b7a63cced0af [6/7] efi/x86: move UEFI Runtime Services wrappers to generic code config: make ARCH=ia64 allmodconfig All error/warnings: drivers/firmware/efi/runtime.c:19:21: fatal error: asm/efi.h: No such file or directory #include asm/efi.h ^ compilation terminated. Ahh, I wasn't prepared for that one ... So runtime.o needs to be included conditionally: any suggestions for a Kconfig symbol to recycle? Or invent a new one? -- Ard. vim +19 drivers/firmware/efi/runtime.c 13 * 14 * This file is released under the GPLv2. 15 */ 16 17 #include linux/efi.h 18 #include linux/spinlock.h /* spinlock_t */ 19 #include asm/efi.h 20 21 #ifndef efi_call_virt 22 #define efi_call_virt __efi_call_virt --- 0-DAY kernel build testing backend Open Source Technology Center http://lists.01.org/mailman/listinfo/kbuild Intel Corporation -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls
According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is allowed, and should adhere to the AAPCS64 calling convention, which states that 'only the bottom 64 bits of each value stored in registers v8-v15 need to be preserved' (section 5.1.2). This applies equally to UEFI Runtime Services called by the kernel, so make sure the FP/SIMD register file is preserved in this case. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/efi.h | 21 + arch/arm64/kernel/efi.c | 14 +- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a474de346be6..93e11f4d9513 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -299,6 +299,7 @@ config EFI select LIBFDT select UCS2_STRING select EFI_PARAMS_FROM_FDT + select EFI_RUNTIME_WRAPPERS default y help This option provides support for runtime services provided diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index 5a46c4e7f539..375ba342dca6 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -2,6 +2,7 @@ #define _ASM_EFI_H #include asm/io.h +#include asm/neon.h #ifdef CONFIG_EFI extern void efi_init(void); @@ -11,4 +12,24 @@ extern void efi_idmap_init(void); #define efi_idmap_init() #endif +#define efi_call_virt(f, ...) \ +({ \ + efi_##f##_t *__f = efi.systab-runtime-f; \ + efi_status_t __s; \ + \ + kernel_neon_begin();\ + __s = __f(__VA_ARGS__); \ + kernel_neon_end(); \ + __s;\ +}) + +#define __efi_call_virt(f, ...) \ +({ \ + efi_##f##_t *__f = efi.systab-runtime-f; \ + \ + kernel_neon_begin();\ + __f(__VA_ARGS__); \ + kernel_neon_end(); \ +}) + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 14db1f6e8d7f..56c3327bbf79 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -449,19 +449,7 @@ static int __init arm64_enter_virtual_mode(void) /* Set up runtime services function pointers */ runtime = efi.systab-runtime; - efi.get_time = runtime-get_time; - efi.set_time = runtime-set_time; - efi.get_wakeup_time = runtime-get_wakeup_time; - efi.set_wakeup_time = runtime-set_wakeup_time; - efi.get_variable = runtime-get_variable; - efi.get_next_variable = runtime-get_next_variable; - efi.set_variable = runtime-set_variable; - efi.query_variable_info = runtime-query_variable_info; - efi.update_capsule = runtime-update_capsule; - efi.query_capsule_caps = runtime-query_capsule_caps; - efi.get_next_high_mono_count = runtime-get_next_high_mono_count; - efi.reset_system = runtime-reset_system; - + efi_native_runtime_setup(); set_bit(EFI_RUNTIME_SERVICES, efi.flags); return 0; -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code
In order for other archs (such as arm64) to be able to reuse the virtual mode function call wrappers, move them to drivers/firmware/efi/runtime-wrappers.c. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/x86/Kconfig| 1 + arch/x86/platform/efi/efi.c | 144 +--- drivers/firmware/efi/Kconfig| 7 ++ drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/runtime-wrappers.c | 161 include/linux/efi.h | 2 + 6 files changed, 174 insertions(+), 142 deletions(-) create mode 100644 drivers/firmware/efi/runtime-wrappers.c diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index a8f749ef0fdc..4c3f026aa5ce 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1522,6 +1522,7 @@ config EFI bool EFI runtime service support depends on ACPI select UCS2_STRING + select EFI_RUNTIME_WRAPPERS ---help--- This enables the kernel to use EFI runtime services that are available (such as the EFI variable services). diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 87fc96bcc13c..36d0835210c3 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -104,130 +104,6 @@ static int __init setup_storage_paranoia(char *arg) } early_param(efi_no_storage_paranoia, setup_storage_paranoia); -static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(get_time, tm, tc); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_set_time(efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(set_time, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, -efi_bool_t *pending, -efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(get_wakeup_time, enabled, pending, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(set_wakeup_time, enabled, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_get_variable(efi_char16_t *name, - efi_guid_t *vendor, - u32 *attr, - unsigned long *data_size, - void *data) -{ - return efi_call_virt(get_variable, -name, vendor, attr, -data_size, data); -} - -static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, - efi_char16_t *name, - efi_guid_t *vendor) -{ - return efi_call_virt(get_next_variable, -name_size, name, vendor); -} - -static efi_status_t virt_efi_set_variable(efi_char16_t *name, - efi_guid_t *vendor, - u32 attr, - unsigned long data_size, - void *data) -{ - return efi_call_virt(set_variable, -name, vendor, attr, -data_size, data); -} - -static efi_status_t virt_efi_query_variable_info(u32 attr, -u64 *storage_space, -u64 *remaining_space, -u64 *max_variable_size) -{ - if (efi.runtime_version EFI_2_00_SYSTEM_TABLE_REVISION) - return EFI_UNSUPPORTED; - - return efi_call_virt(query_variable_info, attr, storage_space, -remaining_space, max_variable_size); -} - -static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) -{ - return efi_call_virt(get_next_high_mono_count, count); -} - -static void virt_efi_reset_system(int reset_type, - efi_status_t status, - unsigned long data_size, - efi_char16_t *data) -{ - __efi_call_virt(reset_system, reset_type, status
[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls
The current UEFI implementation for arm64 fails to preserve/restore the contents of the NEON register file, which may result in data corruption, especially now that those contents are lazily restored for user processes. This series proposes to fix this by wrapping all runtime services calls, and adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers. The first patch moves the existing x86 versions of those wrappers to generic code, so that the second patch can easily enable them by supplying a definition for efi_call_virt and adding a call to efi_native_runtime_setup(). Changes since v1: - rename runtime.c - runtime-wrappers.c - make build depend on new Kconfig symbol EFI_RUNTIME_WRAPPERS to fix ia64 breakage - remove default #defines for efi_call_virt()/__efi_call_virt(), they are not needed anymore now that it is built conditionally - add references to applicable UEFI/AAPCS spec sections Ard Biesheuvel (2): efi/x86: move UEFI Runtime Services wrappers to generic code efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls arch/arm64/Kconfig | 1 + arch/arm64/include/asm/efi.h| 21 + arch/arm64/kernel/efi.c | 14 +-- arch/x86/Kconfig| 1 + arch/x86/platform/efi/efi.c | 144 +--- drivers/firmware/efi/Kconfig| 7 ++ drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/runtime-wrappers.c | 161 include/linux/efi.h | 2 + 9 files changed, 197 insertions(+), 155 deletions(-) create mode 100644 drivers/firmware/efi/runtime-wrappers.c -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls
On 26 June 2014 15:58, Mark Salter msal...@redhat.com wrote: On Thu, 2014-06-26 at 12:09 +0200, Ard Biesheuvel wrote: The current UEFI implementation for arm64 fails to preserve/restore the contents of the NEON register file, Does the current implementation actually use NEON registers? I know there are some at least, which build with -mgeneral-regs-only to keep gcc from using NEON registers. Tianocore/EDK2 does not use -mgeneral-regs-only when building for AArch64, and my current build shows that, for instance, GCC starts spilling to FP registers when compiling the LZMA decoder, and it is likely to do the same for the crypto bits once I start enabling them (although, interestingly enough, the OpenSSL SHA-1 C implementation runs 60% faster with -mgeneral-regs-only set) Not that that means we don't need this. Just curious. The spec does not forbid it, so we will need it anyhow ... -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] efi/x86: efistub: move shared dependencies to asm/efi.h
This moves definitions depended upon both by code under arch/x86/boot and under drivers/firmware/efi to asm/efi.h. This is in preparation of turning the stub code under drivers/firmware/efi into a static library. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/x86/boot/compressed/eboot.c | 5 + arch/x86/boot/compressed/eboot.h | 16 arch/x86/include/asm/efi.h | 25 + 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 0331d765c2bb..2fd5e2643623 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -19,10 +19,7 @@ static efi_system_table_t *sys_table; -static struct efi_config *efi_early; - -#define efi_call_early(f, ...) \ - efi_early-call(efi_early-f, __VA_ARGS__); +struct efi_config *efi_early; #define BOOT_SERVICES(bits)\ static void setup_boot_services##bits(struct efi_config *c)\ diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h index c88c31ecad12..d487e727f1ec 100644 --- a/arch/x86/boot/compressed/eboot.h +++ b/arch/x86/boot/compressed/eboot.h @@ -103,20 +103,4 @@ struct efi_uga_draw_protocol { void *blt; }; -struct efi_config { - u64 image_handle; - u64 table; - u64 allocate_pool; - u64 allocate_pages; - u64 get_memory_map; - u64 free_pool; - u64 free_pages; - u64 locate_handle; - u64 handle_protocol; - u64 exit_boot_services; - u64 text_output; - efi_status_t (*call)(unsigned long, ...); - bool is64; -} __packed; - #endif /* BOOT_COMPRESSED_EBOOT_H */ diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 1eb5f6433ad8..55059a50a01f 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -156,6 +156,31 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( return EFI_SUCCESS; } #endif /* CONFIG_EFI_MIXED */ + + +/* arch specific definitions used by the stub code */ + +struct efi_config { + u64 image_handle; + u64 table; + u64 allocate_pool; + u64 allocate_pages; + u64 get_memory_map; + u64 free_pool; + u64 free_pages; + u64 locate_handle; + u64 handle_protocol; + u64 exit_boot_services; + u64 text_output; + efi_status_t (*call)(unsigned long, ...); + bool is64; +} __packed; + +extern struct efi_config *efi_early; + +#define efi_call_early(f, ...) \ + efi_early-call(efi_early-f, __VA_ARGS__); + #else /* * IF EFI is not configured, have the EFI calls return -ENOSYS. -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/5] efi/arm64: avoid EFI_ERROR as a generic return code
As EFI_ERROR is not a UEFI result code but a local invention only intended to allow get_dram_base() to signal failure, we should not use it elsewhere. Replace with EFI_LOAD_ERROR. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index e786e6cdc400..7aa7155a9740 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -69,7 +69,7 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, if (*image_addr != (dram_base + TEXT_OFFSET)) { pr_efi_err(sys_table, Failed to alloc kernel memory\n); efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; + return EFI_LOAD_ERROR; } *image_size = kernel_memsize; } -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] efi/arm64: efistub: move shared dependencies to asm/efi.h
This moves definitions depended upon both by code under arch/arm64/boot and under drivers/firmware/efi to asm/efi.h. This is in preparation of turning the stub code under drivers/firmware/efi into a static library. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/efi.h | 12 arch/arm64/kernel/efi-stub.c | 11 +-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index 375ba342dca6..a34fd3b12e2b 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -32,4 +32,16 @@ extern void efi_idmap_init(void); kernel_neon_end(); \ }) +/* arch specific definitions used by the stub code */ + +/* + * AArch64 requires the DTB to be 8-byte aligned in the first 512MiB from + * start of kernel and may not cross a 2MiB boundary. We set alignment to + * 2MiB so we know it won't cross a 2MiB boundary. + */ +#define EFI_FDT_ALIGN SZ_2M /* used by allocate_new_fdt_and_exit_boot() */ +#define MAX_FDT_OFFSET SZ_512M + +#define efi_call_early(f, ...) sys_table_arg-boottime-f(__VA_ARGS__) + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 7aa7155a9740..23cbde4324b1 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -10,19 +10,10 @@ * */ #include linux/efi.h +#include asm/efi.h #include linux/libfdt.h #include asm/sections.h -/* - * AArch64 requires the DTB to be 8-byte aligned in the first 512MiB from - * start of kernel and may not cross a 2MiB boundary. We set alignment to - * 2MiB so we know it won't cross a 2MiB boundary. - */ -#define EFI_FDT_ALIGN SZ_2M /* used by allocate_new_fdt_and_exit_boot() */ -#define MAX_FDT_OFFSET SZ_512M - -#define efi_call_early(f, ...) sys_table_arg-boottime-f(__VA_ARGS__) - static void efi_char16_printk(efi_system_table_t *sys_table_arg, efi_char16_t *str); -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/5] efi: efistub: refactor stub components
In order to move from the #include ../../../x.c anti-pattern used by both the x86 and arm64 versions of the stub to a static library linked into either the kernel proper (arm64) or a separate boot executable (x86), there is some prepatory work required. This patch does the following: - move forward declarations of functions shared between the arch specific and the generic parts of the stub to include/linux/efi.h - move forward declarations of functions shared between various .c files of the generic stub code to a new local header file called efistub.h - add #includes to all .c files which were formerly relying on the #includor to include the correct header files - remove all static modifiers from functions which will need to be externally visible once we move to a static library Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 29 - arch/x86/boot/compressed/eboot.c | 13 +++--- drivers/firmware/efi/arm-stub.c| 18 ++--- drivers/firmware/efi/efi-stub-helper.c | 74 +- drivers/firmware/efi/efistub.h | 42 +++ drivers/firmware/efi/fdt.c | 20 + include/linux/efi.h| 42 +++ 7 files changed, 157 insertions(+), 81 deletions(-) create mode 100644 drivers/firmware/efi/efistub.h diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 23cbde4324b1..e4999021b07d 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -11,36 +11,21 @@ */ #include linux/efi.h #include asm/efi.h -#include linux/libfdt.h #include asm/sections.h -static void efi_char16_printk(efi_system_table_t *sys_table_arg, - efi_char16_t *str); - -static efi_status_t efi_open_volume(efi_system_table_t *sys_table, - void *__image, void **__fh); -static efi_status_t efi_file_close(void *handle); - -static efi_status_t -efi_file_read(void *handle, unsigned long *size, void *addr); - -static efi_status_t -efi_file_size(efi_system_table_t *sys_table, void *__fh, - efi_char16_t *filename_16, void **handle, u64 *file_sz); - /* Include shared EFI stub code */ #include ../../../drivers/firmware/efi/efi-stub-helper.c #include ../../../drivers/firmware/efi/fdt.c #include ../../../drivers/firmware/efi/arm-stub.c -static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, - unsigned long *image_addr, - unsigned long *image_size, - unsigned long *reserve_addr, - unsigned long *reserve_size, - unsigned long dram_base, - efi_loaded_image_t *image) +efi_status_t handle_kernel_image(efi_system_table_t *sys_table, +unsigned long *image_addr, +unsigned long *image_size, +unsigned long *reserve_addr, +unsigned long *reserve_size, +unsigned long dram_base, +efi_loaded_image_t *image) { efi_status_t status; unsigned long kernel_size, kernel_memsize = 0; diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 2fd5e2643623..d338c134c659 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -45,8 +45,7 @@ static void setup_boot_services##bits(struct efi_config *c) \ BOOT_SERVICES(32); BOOT_SERVICES(64); -static void efi_printk(efi_system_table_t *, char *); -static void efi_char16_printk(efi_system_table_t *, efi_char16_t *); +void efi_char16_printk(efi_system_table_t *, efi_char16_t *); static efi_status_t __file_size32(void *__fh, efi_char16_t *filename_16, @@ -153,7 +152,7 @@ grow: return status; } -static efi_status_t +efi_status_t efi_file_size(efi_system_table_t *sys_table, void *__fh, efi_char16_t *filename_16, void **handle, u64 *file_sz) { @@ -163,7 +162,7 @@ efi_file_size(efi_system_table_t *sys_table, void *__fh, return __file_size32(__fh, filename_16, handle, file_sz); } -static inline efi_status_t +efi_status_t efi_file_read(void *handle, unsigned long *size, void *addr) { unsigned long func; @@ -181,7 +180,7 @@ efi_file_read(void *handle, unsigned long *size, void *addr) } } -static inline efi_status_t efi_file_close(void *handle) +efi_status_t efi_file_close(void *handle) { if (efi_early-is64) { efi_file_handle_64_t *fh = handle; @@ -246,7 +245,7 @@ static inline efi_status_t __open_volume64(void *__image, void **__fh) return status; } -static inline efi_status_t +efi_status_t efi_open_volume
[PATCH v2 0/5] efistub: convert into static library
This is v2 of the series to change the #include ../../../../xxx.c pattern into a static library linked into either the kernel (arm64) or a separate boot decompressor (x86, ARM). Changes since v1: - added patch #1 to change EFI_ERROR, it is not a result code defined by UEFI so it should only be returned by get_dram_base() and efi_entry() - added a section to libstub Makefile to clean CFLAGS of stack protecter and other options that are inappropriate for the stub - rebased onto the UEFI Runtime Services NEON patches (re)posted earlier today Ard Biesheuvel (5): efi/arm64: avoid EFI_ERROR as a generic return code efi/x86: efistub: move shared dependencies to asm/efi.h efi/arm64: efistub: move shared dependencies to asm/efi.h efi: efistub: refactor stub components efi: efistub: convert into static library arch/arm64/Kconfig | 1 + arch/arm64/Makefile| 1 + arch/arm64/include/asm/efi.h | 12 arch/arm64/kernel/efi-stub.c | 47 +++--- arch/x86/boot/compressed/Makefile | 3 +- arch/x86/boot/compressed/eboot.c | 20 ++ arch/x86/boot/compressed/eboot.h | 16 - arch/x86/include/asm/efi.h | 25 drivers/firmware/efi/Kconfig | 3 + drivers/firmware/efi/Makefile | 2 +- drivers/firmware/efi/libstub/Makefile | 26 drivers/firmware/efi/{ = libstub}/arm-stub.c | 32 ++ .../firmware/efi/{ = libstub}/efi-stub-helper.c | 74 +++--- drivers/firmware/efi/libstub/efistub.h | 42 drivers/firmware/efi/{ = libstub}/fdt.c | 20 +++--- include/linux/efi.h| 42 16 files changed, 238 insertions(+), 128 deletions(-) create mode 100644 drivers/firmware/efi/libstub/Makefile rename drivers/firmware/efi/{ = libstub}/arm-stub.c (93%) rename drivers/firmware/efi/{ = libstub}/efi-stub-helper.c (88%) create mode 100644 drivers/firmware/efi/libstub/efistub.h rename drivers/firmware/efi/{ = libstub}/fdt.c (94%) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] efi: efistub: convert into static library
This patch changes both x86 and arm64 efistub implementations from #including shared .c files under drivers/firmware/efi to building the shared code as a static library. The x86 code uses a stub built into the boot executable which uncompresses the kernel at boot time. In this case, the library is linked into the decompressor. In the arm64 case, the stub is part of the kernel proper so the library is linked into the kernel proper as well. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/Kconfig | 1 + arch/arm64/Makefile| 1 + arch/arm64/kernel/efi-stub.c | 5 - arch/x86/boot/compressed/Makefile | 3 ++- arch/x86/boot/compressed/eboot.c | 2 -- drivers/firmware/efi/Kconfig | 3 +++ drivers/firmware/efi/Makefile | 2 +- drivers/firmware/efi/libstub/Makefile | 26 ++ drivers/firmware/efi/{ = libstub}/arm-stub.c | 14 ++-- .../firmware/efi/{ = libstub}/efi-stub-helper.c | 0 drivers/firmware/efi/{ = libstub}/efistub.h | 0 drivers/firmware/efi/{ = libstub}/fdt.c | 0 12 files changed, 41 insertions(+), 16 deletions(-) create mode 100644 drivers/firmware/efi/libstub/Makefile rename drivers/firmware/efi/{ = libstub}/arm-stub.c (96%) rename drivers/firmware/efi/{ = libstub}/efi-stub-helper.c (100%) rename drivers/firmware/efi/{ = libstub}/efistub.h (100%) rename drivers/firmware/efi/{ = libstub}/fdt.c (100%) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 93e11f4d9513..f766f346022d 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -300,6 +300,7 @@ config EFI select UCS2_STRING select EFI_PARAMS_FROM_FDT select EFI_RUNTIME_WRAPPERS + select EFI_ARMSTUB default y help This option provides support for runtime services provided diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 8185a913c5ed..bb8f21a626c0 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -48,6 +48,7 @@ core-$(CONFIG_XEN) += arch/arm64/xen/ core-$(CONFIG_CRYPTO) += arch/arm64/crypto/ libs-y := arch/arm64/lib/ $(libs-y) libs-y += $(LIBGCC) +libs-$(CONFIG_EFI) += drivers/firmware/efi/libstub/ # Default target when executing plain make KBUILD_IMAGE := Image.gz diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index e4999021b07d..12456a7d3fa2 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -13,11 +13,6 @@ #include asm/efi.h #include asm/sections.h -/* Include shared EFI stub code */ -#include ../../../drivers/firmware/efi/efi-stub-helper.c -#include ../../../drivers/firmware/efi/fdt.c -#include ../../../drivers/firmware/efi/arm-stub.c - efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 0fcd9133790c..7a801a310e37 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -33,7 +33,8 @@ VMLINUX_OBJS = $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \ $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone ifeq ($(CONFIG_EFI_STUB), y) - VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o + VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \ + $(objtree)/drivers/firmware/efi/libstub/lib.a endif $(obj)/vmlinux: $(VMLINUX_OBJS) FORCE diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index d338c134c659..d4d865438a0c 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -280,8 +280,6 @@ void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str) } } -#include ../../../../drivers/firmware/efi/efi-stub-helper.c - static void find_bits(unsigned long mask, u8 *pos, u8 *size) { u8 first, len; diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index 04a7af46736a..395e76d9a1b5 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -61,6 +61,9 @@ config EFI_RUNTIME_WRAPPERS in which case it needs to provide #definitions of efi_call_virt and __efi_call_virt in asm/efi.h +config EFI_ARMSTUB + bool + endmenu config UEFI_CPER diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index e1096539eedb..d9abdbc962f1 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers/firmware/efi/Makefile @@ -1,7 +1,7 @@ # # Makefile for linux kernel # -obj-$(CONFIG_EFI) += efi.o vars.o +obj-$(CONFIG_EFI) += efi.o vars.o libstub/ obj-$(CONFIG_EFI_VARS) += efivars.o obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
Re: [PATCH v2 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code
On 1 July 2014 15:30, Matt Fleming m...@console-pimps.org wrote: On Thu, 26 Jun, at 12:09:05PM, Ard Biesheuvel wrote: In order for other archs (such as arm64) to be able to reuse the virtual mode function call wrappers, move them to drivers/firmware/efi/runtime-wrappers.c. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org [...] @@ -54,6 +54,13 @@ config EFI_PARAMS_FROM_FDT the EFI runtime support gets system table address, memory map address, and other parameters from the device tree. +config EFI_RUNTIME_WRAPPERS + bool + help + Selected by the arch if it needs to wrap UEFI Runtime Services calls, + in which case it needs to provide #definitions of efi_call_virt and + __efi_call_virt in asm/efi.h + endmenu Actually, could we drop this help text? That may seem like a backwards step, but I have concerns that we'll fail to keep this help text in sync with the code. Furthermore, by providing help text that kinda says, casual users need the help text to understand when to enable this feature. Clearly that's not what this Kconfig symbol is for. Most of the other guard Kconfig symbols don't provide this kind of text, and I think there's good reason to follow suit. What do you think? I agree, but I kind of followed the example of the Kconfig symbols in the vicinity. So sure, rip it out. Or would you like me to do a v(n+1)? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls
On 1 July 2014 15:26, Matt Fleming m...@console-pimps.org wrote: On Thu, 26 Jun, at 12:09:04PM, Ard Biesheuvel wrote: The current UEFI implementation for arm64 fails to preserve/restore the contents of the NEON register file, which may result in data corruption, especially now that those contents are lazily restored for user processes. This series proposes to fix this by wrapping all runtime services calls, and adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers. The first patch moves the existing x86 versions of those wrappers to generic code, so that the second patch can easily enable them by supplying a definition for efi_call_virt and adding a call to efi_native_runtime_setup(). Changes since v1: - rename runtime.c - runtime-wrappers.c - make build depend on new Kconfig symbol EFI_RUNTIME_WRAPPERS to fix ia64 breakage - remove default #defines for efi_call_virt()/__efi_call_virt(), they are not needed anymore now that it is built conditionally - add references to applicable UEFI/AAPCS spec sections Ard Biesheuvel (2): efi/x86: move UEFI Runtime Services wrappers to generic code efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls Thanks Ard. These patches look OK to me, and I've now pulled them into my 'next' branch. It'd be nice if we could get some ACKs from other people since this is an ABI issue. Yes, they have been awfully quiet, haven't they? At least Roy has volunteered to get involved in the USWG to get the spec clarified. @Leif, Catalin, Olivier: care to chime in? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/5] efi: efistub: refactor stub components
On 1 July 2014 17:11, Matt Fleming m...@console-pimps.org wrote: On Thu, 26 Jun, at 04:23:36PM, Ard Biesheuvel wrote: In order to move from the #include ../../../x.c anti-pattern used by both the x86 and arm64 versions of the stub to a static library linked into either the kernel proper (arm64) or a separate boot executable (x86), there is some prepatory work required. This patch does the following: - move forward declarations of functions shared between the arch specific and the generic parts of the stub to include/linux/efi.h - move forward declarations of functions shared between various .c files of the generic stub code to a new local header file called efistub.h - add #includes to all .c files which were formerly relying on the #includor to include the correct header files - remove all static modifiers from functions which will need to be externally visible once we move to a static library Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org [...] diff --git a/include/linux/efi.h b/include/linux/efi.h index 0ceb816bdfc2..3a64f2f85821 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1163,4 +1163,46 @@ static inline void efi_runtime_map_setup(void *map, int nr_entries, u32 desc_size) {} #endif +/* prototypes shared between arch specific and generic stub code */ + +#define pr_efi(sys_table, msg) efi_printk(sys_table, EFI stub: msg) +#define pr_efi_err(sys_table, msg) efi_printk(sys_table, EFI stub: ERROR: msg) + +void efi_printk(efi_system_table_t *sys_table_arg, char *str); + +void efi_free(efi_system_table_t *sys_table_arg, unsigned long size, + unsigned long addr); + +char *efi_convert_cmdline(efi_system_table_t *sys_table_arg, + efi_loaded_image_t *image, int *cmd_line_len); + We've been very good so far at not splattering include/linux/efi.h with any of the EFI boot stub prototypes, and it would be awesome if we didn't have to start now. Is there any way we could avoid doing this? Arguably everything should be in the new efistub.h, no? There are bits that are shared between code under arch/xxx and drivers/firmware/efi, and what those bits are is different between the archs. I used asm/efi.h just for convenience, but perhaps we could add asm/efistub.h for each arch, and #include it in both efistub.h under drivers/firmware/efi and the stub bits that live under arch/xxx? Then any other users of asm/efi.h won't have to see it. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services
According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- drivers/firmware/efi/runtime-wrappers.c | 109 +--- 1 file changed, 99 insertions(+), 10 deletions(-) diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 10daa4bbb258..6588d054af99 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -15,10 +15,50 @@ */ #include linux/efi.h -#include linux/spinlock.h /* spinlock_t */ +#include linux/mutex.h +#include linux/spinlock.h #include asm/efi.h /* + * According to section 7.1 of the UEFI spec, Runtime Services are not fully + * reentrant, and there are particular combinations of calls that need to be + * serialized. + * + * Table 31. Rules for Reentry Into Runtime Services + * ++---+ + * | If previous call is busy in | Forbidden to call | + * ++---+ + * | Any | SetVirtualAddressMap()| + * ++---+ + * | ConvertPointer() | ConvertPointer() | + * ++---+ + * | SetVariable() | ResetSystem() | + * | UpdateCapsule() | | + * | SetTime() | | + * | SetWakeupTime() | | + * | GetNextHighMonotonicCount() | | + * ++---+ + * | GetVariable() | GetVariable() | + * | GetNextVariableName() | GetNextVariableName() | + * | SetVariable() | SetVariable() | + * | QueryVariableInfo() | QueryVariableInfo() | + * | UpdateCapsule() | UpdateCapsule() | + * | QueryCapsuleCapabilities()| QueryCapsuleCapabilities() | + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | + * ++---+ + * | GetTime() | GetTime() | + * | SetTime() | SetTime() | + * | GetWakeupTime() | GetWakeupTime() | + * | SetWakeupTime() | SetWakeupTime() | + * ++---+ + * + * The first two are disregarded here, as SetVirtualAddressMap() is called + * only once, and very early, and ConvertPointer() is not exposed at all. + */ +static DEFINE_MUTEX(var_ro_mutex); +static DEFINE_MUTEX(var_rw_mutex); + +/* * As per commit ef68c8f87ed1 (x86: Serialize EFI time accesses on rtc_lock), * the EFI specification requires that callers of the time related runtime * functions serialize with other CMOS accesses in the kernel, as the EFI time @@ -78,14 +118,25 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, unsigned long *data_size, void *data) { - return efi_call_virt(get_variable, name, vendor, attr, data_size, data); + efi_status_t status; + + mutex_lock(var_ro_mutex); + status = efi_call_virt(get_variable, name, vendor, attr, data_size, + data); + mutex_unlock(var_ro_mutex); + return status; } static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { - return efi_call_virt(get_next_variable, name_size, name, vendor); + efi_status_t status; + + mutex_lock(var_ro_mutex); + status = efi_call_virt(get_next_variable, name_size, name, vendor); + mutex_unlock(var_ro_mutex); + return status; } static efi_status_t virt_efi_set_variable(efi_char16_t *name, @@ -94,7 +145,15 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, unsigned long data_size, void *data) { - return efi_call_virt(set_variable, name, vendor, attr, data_size, data); + efi_status_t status; + + mutex_lock(var_ro_mutex); + mutex_lock(var_rw_mutex); + status = efi_call_virt(set_variable, name, vendor, attr, data_size, + data
[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab
We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but then go on an dereference it anyway. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 56c3327bbf79..e785f93f17cb 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void) } efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); - if (efi.systab) - set_bit(EFI_SYSTEM_TABLES, efi.flags); + if (!efi.systab) { + pr_err(Failed to remap EFI System Table!\n); + return -1; + } + set_bit(EFI_SYSTEM_TABLES, efi.flags); local_irq_save(flags); cpu_switch_mm(idmap_pg_dir, init_mm); -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] efi: efistub: convert into static library
On 2 July 2014 13:15, Matt Fleming m...@console-pimps.org wrote: On Thu, 26 Jun, at 04:23:37PM, Ard Biesheuvel wrote: This patch changes both x86 and arm64 efistub implementations from #including shared .c files under drivers/firmware/efi to building the shared code as a static library. The x86 code uses a stub built into the boot executable which uncompresses the kernel at boot time. In this case, the library is linked into the decompressor. In the arm64 case, the stub is part of the kernel proper so the library is linked into the kernel proper as well. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org OK, this breaks the ia64 build because of the following... /drivers/firmware/efi/libstub/efi-stub-helper.c:14:21: fatal error: asm/efi.h: No such file or directory diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index e1096539eedb..d9abdbc962f1 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers/firmware/efi/Makefile @@ -1,7 +1,7 @@ # # Makefile for linux kernel # -obj-$(CONFIG_EFI)+= efi.o vars.o +obj-$(CONFIG_EFI)+= efi.o vars.o libstub/ obj-$(CONFIG_EFI_VARS) += efivars.o obj-$(CONFIG_EFI_VARS_PSTORE)+= efi-pstore.o obj-$(CONFIG_UEFI_CPER) += cper.o I guess what we need is CONFIG_EFI_LIBSTUB selected by both CONFIG_EFI_STUB (for x86) and CONFIG_EFI_ARMSTUB (for arm64)? e.g. obj-$(CONFIG_EFI_LIBSTUB) libstub/ Yes, that seems the appropriate way to deal with this. Let me respin so I can fix the other thing I mentioned yesterday as well. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] efi: efistub: convert into static library
Would something like this ifneq ($(CONFIG_EFI_STUB)$(CONFIG_EFI_ARMSTUB),nn) obj-y += libstub/ endif be too hideous? On 2 July 2014 13:23, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 2 July 2014 13:15, Matt Fleming m...@console-pimps.org wrote: On Thu, 26 Jun, at 04:23:37PM, Ard Biesheuvel wrote: This patch changes both x86 and arm64 efistub implementations from #including shared .c files under drivers/firmware/efi to building the shared code as a static library. The x86 code uses a stub built into the boot executable which uncompresses the kernel at boot time. In this case, the library is linked into the decompressor. In the arm64 case, the stub is part of the kernel proper so the library is linked into the kernel proper as well. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org OK, this breaks the ia64 build because of the following... /drivers/firmware/efi/libstub/efi-stub-helper.c:14:21: fatal error: asm/efi.h: No such file or directory diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index e1096539eedb..d9abdbc962f1 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers/firmware/efi/Makefile @@ -1,7 +1,7 @@ # # Makefile for linux kernel # -obj-$(CONFIG_EFI)+= efi.o vars.o +obj-$(CONFIG_EFI)+= efi.o vars.o libstub/ obj-$(CONFIG_EFI_VARS) += efivars.o obj-$(CONFIG_EFI_VARS_PSTORE)+= efi-pstore.o obj-$(CONFIG_UEFI_CPER) += cper.o I guess what we need is CONFIG_EFI_LIBSTUB selected by both CONFIG_EFI_STUB (for x86) and CONFIG_EFI_ARMSTUB (for arm64)? e.g. obj-$(CONFIG_EFI_LIBSTUB) libstub/ Yes, that seems the appropriate way to deal with this. Let me respin so I can fix the other thing I mentioned yesterday as well. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] efi: efistub: refactor stub components
In order to move from the #include ../../../x.c anti-pattern used by both the x86 and arm64 versions of the stub to a static library linked into either the kernel proper (arm64) or a separate boot executable (x86), there is some prepatory work required. This patch does the following: - move forward declarations of functions shared between the arch specific and the generic parts of the stub to include/linux/efi.h - move forward declarations of functions shared between various .c files of the generic stub code to a new local header file called efistub.h - add #includes to all .c files which were formerly relying on the #includor to include the correct header files - remove all static modifiers from functions which will need to be externally visible once we move to a static library Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 29 - arch/x86/boot/compressed/eboot.c | 13 +++--- drivers/firmware/efi/arm-stub.c| 32 +-- drivers/firmware/efi/efi-stub-helper.c | 74 +- drivers/firmware/efi/efistub.h | 42 +++ drivers/firmware/efi/fdt.c | 20 + include/linux/efi.h| 42 +++ 7 files changed, 164 insertions(+), 88 deletions(-) create mode 100644 drivers/firmware/efi/efistub.h diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 23cbde4324b1..e4999021b07d 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -11,36 +11,21 @@ */ #include linux/efi.h #include asm/efi.h -#include linux/libfdt.h #include asm/sections.h -static void efi_char16_printk(efi_system_table_t *sys_table_arg, - efi_char16_t *str); - -static efi_status_t efi_open_volume(efi_system_table_t *sys_table, - void *__image, void **__fh); -static efi_status_t efi_file_close(void *handle); - -static efi_status_t -efi_file_read(void *handle, unsigned long *size, void *addr); - -static efi_status_t -efi_file_size(efi_system_table_t *sys_table, void *__fh, - efi_char16_t *filename_16, void **handle, u64 *file_sz); - /* Include shared EFI stub code */ #include ../../../drivers/firmware/efi/efi-stub-helper.c #include ../../../drivers/firmware/efi/fdt.c #include ../../../drivers/firmware/efi/arm-stub.c -static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, - unsigned long *image_addr, - unsigned long *image_size, - unsigned long *reserve_addr, - unsigned long *reserve_size, - unsigned long dram_base, - efi_loaded_image_t *image) +efi_status_t handle_kernel_image(efi_system_table_t *sys_table, +unsigned long *image_addr, +unsigned long *image_size, +unsigned long *reserve_addr, +unsigned long *reserve_size, +unsigned long dram_base, +efi_loaded_image_t *image) { efi_status_t status; unsigned long kernel_size, kernel_memsize = 0; diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index c066bc4e3051..916bbdd7dd28 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -45,8 +45,7 @@ static void setup_boot_services##bits(struct efi_config *c) \ BOOT_SERVICES(32); BOOT_SERVICES(64); -static void efi_printk(efi_system_table_t *, char *); -static void efi_char16_printk(efi_system_table_t *, efi_char16_t *); +void efi_char16_printk(efi_system_table_t *, efi_char16_t *); static efi_status_t __file_size32(void *__fh, efi_char16_t *filename_16, @@ -153,7 +152,7 @@ grow: return status; } -static efi_status_t +efi_status_t efi_file_size(efi_system_table_t *sys_table, void *__fh, efi_char16_t *filename_16, void **handle, u64 *file_sz) { @@ -163,7 +162,7 @@ efi_file_size(efi_system_table_t *sys_table, void *__fh, return __file_size32(__fh, filename_16, handle, file_sz); } -static inline efi_status_t +efi_status_t efi_file_read(void *handle, unsigned long *size, void *addr) { unsigned long func; @@ -181,7 +180,7 @@ efi_file_read(void *handle, unsigned long *size, void *addr) } } -static inline efi_status_t efi_file_close(void *handle) +efi_status_t efi_file_close(void *handle) { if (efi_early-is64) { efi_file_handle_64_t *fh = handle; @@ -246,7 +245,7 @@ static inline efi_status_t __open_volume64(void *__image, void **__fh) return status; } -static inline efi_status_t +efi_status_t efi_open_volume
[PATCH v3 0/5] efistub: convert into static library
This is v3 of the series to change the #include ../../../../xxx.c pattern into a static library linked into either the kernel (arm64) or a separate boot decompressor (x86, ARM). Changes since v2: - make sure that removals of 'static' modifiers occur in a way that doesn't break bisect (i.e., definition + all declarations in the same patch) - avoid ia64 breakage (which does not use the stub) by building it conditionally on CONFIG_EFI_STUB not CONFIG_EFI Changes since v1: - added patch #1 to change EFI_ERROR, it is not a result code defined by UEFI so it should only be returned by get_dram_base() and efi_entry() - added a section to libstub Makefile to clean CFLAGS of stack protecter and other options that are inappropriate for the stub - rebased onto the UEFI Runtime Services NEON patches (re)posted earlier today Ard Biesheuvel (5): efi/arm64: Avoid EFI_ERROR as a generic return code efi/x86: efistub: Move shared dependencies to asm/efi.h efi/arm64: efistub: Move shared dependencies to asm/efi.h efi: efistub: refactor stub components efi: efistub: convert into static library arch/arm64/Kconfig | 5 ++ arch/arm64/Makefile| 1 + arch/arm64/include/asm/efi.h | 12 arch/arm64/kernel/efi-stub.c | 48 +++--- arch/x86/boot/compressed/Makefile | 3 +- arch/x86/boot/compressed/eboot.c | 20 ++ arch/x86/boot/compressed/eboot.h | 16 - arch/x86/include/asm/efi.h | 25 drivers/firmware/efi/Kconfig | 3 + drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/libstub/Makefile | 26 drivers/firmware/efi/{ = libstub}/arm-stub.c | 32 ++ .../firmware/efi/{ = libstub}/efi-stub-helper.c | 74 +++--- drivers/firmware/efi/libstub/efistub.h | 42 drivers/firmware/efi/{ = libstub}/fdt.c | 20 +++--- include/linux/efi.h| 42 16 files changed, 242 insertions(+), 128 deletions(-) create mode 100644 drivers/firmware/efi/libstub/Makefile rename drivers/firmware/efi/{ = libstub}/arm-stub.c (93%) rename drivers/firmware/efi/{ = libstub}/efi-stub-helper.c (88%) create mode 100644 drivers/firmware/efi/libstub/efistub.h rename drivers/firmware/efi/{ = libstub}/fdt.c (94%) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/5] efi/x86: efistub: Move shared dependencies to asm/efi.h
This moves definitions depended upon both by code under arch/x86/boot and under drivers/firmware/efi to asm/efi.h. This is in preparation of turning the stub code under drivers/firmware/efi into a static library. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Signed-off-by: Matt Fleming matt.flem...@intel.com --- arch/x86/boot/compressed/eboot.c | 5 + arch/x86/boot/compressed/eboot.h | 16 arch/x86/include/asm/efi.h | 25 + 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 385f42c200bc..c066bc4e3051 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -19,10 +19,7 @@ static efi_system_table_t *sys_table; -static struct efi_config *efi_early; - -#define efi_call_early(f, ...) \ - efi_early-call(efi_early-f, __VA_ARGS__); +struct efi_config *efi_early; #define BOOT_SERVICES(bits)\ static void setup_boot_services##bits(struct efi_config *c)\ diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h index c88c31ecad12..d487e727f1ec 100644 --- a/arch/x86/boot/compressed/eboot.h +++ b/arch/x86/boot/compressed/eboot.h @@ -103,20 +103,4 @@ struct efi_uga_draw_protocol { void *blt; }; -struct efi_config { - u64 image_handle; - u64 table; - u64 allocate_pool; - u64 allocate_pages; - u64 get_memory_map; - u64 free_pool; - u64 free_pages; - u64 locate_handle; - u64 handle_protocol; - u64 exit_boot_services; - u64 text_output; - efi_status_t (*call)(unsigned long, ...); - bool is64; -} __packed; - #endif /* BOOT_COMPRESSED_EBOOT_H */ diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 3dbf56eb540d..9043f365ebf5 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -158,6 +158,31 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( return EFI_SUCCESS; } #endif /* CONFIG_EFI_MIXED */ + + +/* arch specific definitions used by the stub code */ + +struct efi_config { + u64 image_handle; + u64 table; + u64 allocate_pool; + u64 allocate_pages; + u64 get_memory_map; + u64 free_pool; + u64 free_pages; + u64 locate_handle; + u64 handle_protocol; + u64 exit_boot_services; + u64 text_output; + efi_status_t (*call)(unsigned long, ...); + bool is64; +} __packed; + +extern struct efi_config *efi_early; + +#define efi_call_early(f, ...) \ + efi_early-call(efi_early-f, __VA_ARGS__); + #else /* * IF EFI is not configured, have the EFI calls return -ENOSYS. -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] efi: efistub: convert into static library
This patch changes both x86 and arm64 efistub implementations from #including shared .c files under drivers/firmware/efi to building the shared code as a static library. The x86 code uses a stub built into the boot executable which uncompresses the kernel at boot time. In this case, the library is linked into the decompressor. In the arm64 case, the stub is part of the kernel proper so the library is linked into the kernel proper as well. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/Kconfig | 5 + arch/arm64/Makefile| 1 + arch/arm64/kernel/efi-stub.c | 6 - arch/x86/boot/compressed/Makefile | 3 ++- arch/x86/boot/compressed/eboot.c | 2 -- drivers/firmware/efi/Kconfig | 3 +++ drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/libstub/Makefile | 26 ++ drivers/firmware/efi/{ = libstub}/arm-stub.c | 0 .../firmware/efi/{ = libstub}/efi-stub-helper.c | 0 drivers/firmware/efi/{ = libstub}/efistub.h | 0 drivers/firmware/efi/{ = libstub}/fdt.c | 0 12 files changed, 38 insertions(+), 9 deletions(-) create mode 100644 drivers/firmware/efi/libstub/Makefile rename drivers/firmware/efi/{ = libstub}/arm-stub.c (100%) rename drivers/firmware/efi/{ = libstub}/efi-stub-helper.c (100%) rename drivers/firmware/efi/{ = libstub}/efistub.h (100%) rename drivers/firmware/efi/{ = libstub}/fdt.c (100%) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 2cc14cef01bd..3a0a4ce4c751 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -292,6 +292,9 @@ config CMDLINE_FORCE This is useful if you cannot or don't want to change the command-line options your boot loader passes to the kernel. +config EFI_STUB + bool + config EFI bool UEFI runtime support depends on OF !CPU_BIG_ENDIAN @@ -299,6 +302,8 @@ config EFI select UCS2_STRING select EFI_PARAMS_FROM_FDT select EFI_RUNTIME_WRAPPERS + select EFI_STUB + select EFI_ARMSTUB default y help This option provides support for runtime services provided diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 8185a913c5ed..5836717d2f66 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -48,6 +48,7 @@ core-$(CONFIG_XEN) += arch/arm64/xen/ core-$(CONFIG_CRYPTO) += arch/arm64/crypto/ libs-y := arch/arm64/lib/ $(libs-y) libs-y += $(LIBGCC) +libs-$(CONFIG_EFI_STUB) += drivers/firmware/efi/libstub/ # Default target when executing plain make KBUILD_IMAGE := Image.gz diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index e4999021b07d..1317fef8dde9 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -13,12 +13,6 @@ #include asm/efi.h #include asm/sections.h -/* Include shared EFI stub code */ -#include ../../../drivers/firmware/efi/efi-stub-helper.c -#include ../../../drivers/firmware/efi/fdt.c -#include ../../../drivers/firmware/efi/arm-stub.c - - efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, unsigned long *image_size, diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 0fcd9133790c..7a801a310e37 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -33,7 +33,8 @@ VMLINUX_OBJS = $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \ $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone ifeq ($(CONFIG_EFI_STUB), y) - VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o + VMLINUX_OBJS += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \ + $(objtree)/drivers/firmware/efi/libstub/lib.a endif $(obj)/vmlinux: $(VMLINUX_OBJS) FORCE diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 916bbdd7dd28..3b5c66c8f749 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -280,8 +280,6 @@ void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str) } } -#include ../../../../drivers/firmware/efi/efi-stub-helper.c - static void find_bits(unsigned long mask, u8 *pos, u8 *size) { u8 first, len; diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index 588dc47e7075..f712d47f30d8 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -57,6 +57,9 @@ config EFI_PARAMS_FROM_FDT config EFI_RUNTIME_WRAPPERS bool +config EFI_ARMSTUB + bool + endmenu config UEFI_CPER diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index e1096539eedb..a204d1474cec 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers
[PATCH v3 1/5] efi/arm64: Avoid EFI_ERROR as a generic return code
As EFI_ERROR is not a UEFI result code but a local invention only intended to allow get_dram_base() to signal failure, we should not use it elsewhere. Replace with EFI_LOAD_ERROR. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Signed-off-by: Matt Fleming matt.flem...@intel.com --- arch/arm64/kernel/efi-stub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index e786e6cdc400..7aa7155a9740 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -69,7 +69,7 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, if (*image_addr != (dram_base + TEXT_OFFSET)) { pr_efi_err(sys_table, Failed to alloc kernel memory\n); efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; + return EFI_LOAD_ERROR; } *image_size = kernel_memsize; } -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] efi/x86: efistub: move shared dependencies to asm/efi.h
On 2 July 2014 14:59, Mark Salter msal...@redhat.com wrote: On Thu, 2014-06-26 at 16:23 +0200, Ard Biesheuvel wrote: This moves definitions depended upon both by code under arch/x86/boot and under drivers/firmware/efi to asm/efi.h. This is in preparation of turning the stub code under drivers/firmware/efi into a static library. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/x86/boot/compressed/eboot.c | 5 + arch/x86/boot/compressed/eboot.h | 16 arch/x86/include/asm/efi.h | 25 + 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 0331d765c2bb..2fd5e2643623 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -19,10 +19,7 @@ static efi_system_table_t *sys_table; -static struct efi_config *efi_early; - -#define efi_call_early(f, ...) \ - efi_early-call(efi_early-f, __VA_ARGS__); +struct efi_config *efi_early; #define BOOT_SERVICES(bits) \ static void setup_boot_services##bits(struct efi_config *c) \ diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h index c88c31ecad12..d487e727f1ec 100644 --- a/arch/x86/boot/compressed/eboot.h +++ b/arch/x86/boot/compressed/eboot.h @@ -103,20 +103,4 @@ struct efi_uga_draw_protocol { void *blt; }; -struct efi_config { - u64 image_handle; - u64 table; - u64 allocate_pool; - u64 allocate_pages; - u64 get_memory_map; - u64 free_pool; - u64 free_pages; - u64 locate_handle; - u64 handle_protocol; - u64 exit_boot_services; - u64 text_output; - efi_status_t (*call)(unsigned long, ...); - bool is64; -} __packed; - #endif /* BOOT_COMPRESSED_EBOOT_H */ diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 1eb5f6433ad8..55059a50a01f 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -156,6 +156,31 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( return EFI_SUCCESS; } #endif /* CONFIG_EFI_MIXED */ + + +/* arch specific definitions used by the stub code */ + +struct efi_config { + u64 image_handle; + u64 table; + u64 allocate_pool; + u64 allocate_pages; + u64 get_memory_map; + u64 free_pool; + u64 free_pages; + u64 locate_handle; + u64 handle_protocol; + u64 exit_boot_services; + u64 text_output; + efi_status_t (*call)(unsigned long, ...); + bool is64; +} __packed; + +extern struct efi_config *efi_early; + +#define efi_call_early(f, ...) \ + efi_early-call(efi_early-f, __VA_ARGS__); That shouldn't have the trailing ; Hmm, perhaps not. This patch just moves stuff around, so fixing this should probably be done in a separate patch. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab
On 2 July 2014 16:29, Mark Salter msal...@redhat.com wrote: On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote: On 2 July 2014 12:10, Ard Biesheuvel ard.biesheu...@linaro.org wrote: We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but then go on an dereference it anyway. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 56c3327bbf79..e785f93f17cb 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void) } efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); - if (efi.systab) - set_bit(EFI_SYSTEM_TABLES, efi.flags); + if (!efi.systab) { + pr_err(Failed to remap EFI System Table!\n); ... this needs a kfree(virtmap) as well. And probably should unmap all the remapped regions in virtmap first. Yes. Presumably the systab will be in a runtime memory region which gets virtual mapping from remap_region(). If that succeeds, then the efi_lookup_mapped_addr should always succeed. But to be careful, we should probably bail out earlier if remap_region() ever returns zero. If all remaps work and efi_lookup_mapped_addr() fails, then we should try ioremap_cache() directly. Or do what x86 does and make a local copy of the table earlier when we early_memremap() it in uefi_init(). Bailing early (and cleaning up) if any remap_region() call fails seems like a wise thing to do. But if they all succeed, and efi_lookup_mapped_addr() then fails to resolve efi_system_table, it means it lives in a region which will become inaccessible to the runtime services themselves after set_virtual_address_map() has been called. So doing any kind of fallback masks a severe firmware bug, which makes me think we should just complain loudly and not enable UEFI bits any further -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] efi/arm64: handle missing virtual mapping for UEFI System Table
If we cannot resolve the virtual address of the UEFI System Table, its physical offset must be missing from the virtual memory map, and there is really no point in proceeding with installing the virtual memory map and the runtime services dispatch table. So back out gracefully. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- v2: - release mappings and free virtmap before bailing arch/arm64/kernel/efi.c | 28 ++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 56c3327bbf79..23942158e0f8 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -416,11 +416,23 @@ static int __init arm64_enter_virtual_mode(void) continue; if (remap_region(md, virt_md)) ++count; + else + goto err_unmap; } efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); - if (efi.systab) - set_bit(EFI_SYSTEM_TABLES, efi.flags); + if (!efi.systab) { + /* +* If we have no virtual mapping for the System Table at this +* point, the memory map doesn't cover the physical offset where +* it resides. This means the System Table will be inaccessible +* to Runtime Services themselves once the virtual mapping is +* installed. +*/ + pr_err(Failed to remap EFI System Table -- buggy firmware?\n); + goto err_unmap; + } + set_bit(EFI_SYSTEM_TABLES, efi.flags); local_irq_save(flags); cpu_switch_mm(idmap_pg_dir, init_mm); @@ -453,5 +465,17 @@ static int __init arm64_enter_virtual_mode(void) set_bit(EFI_RUNTIME_SERVICES, efi.flags); return 0; + +err_unmap: + /* unmap all mappings that succeeded: there are 'count' of those */ + for_each_efi_memory_desc(memmap, md) { + if (!(md-attribute EFI_MEMORY_RUNTIME)) + continue; + if (!count--) + break; + iounmap((__force void *)md-virt_addr); + } + kfree(virtmap); + return -1; } early_initcall(arm64_enter_virtual_mode); -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab
On 4 July 2014 15:38, Catalin Marinas catalin.mari...@arm.com wrote: On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote: We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but then go on an dereference it anyway. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Thanks. I applied this one (the other patch needs to go in via a different route). Eh, actually, I proposed a v2 based on Mark's feedback, but I now see that I accidentally removed you from the To list. http://marc.info/?l=linux-arm-kernelm=140446911506688w=2 -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] efi/arm64: handle missing virtual mapping for UEFI System Table
(adding Catalin) On 4 July 2014 16:42, Mark Salter msal...@redhat.com wrote: On Fri, 2014-07-04 at 12:16 +0200, Ard Biesheuvel wrote: If we cannot resolve the virtual address of the UEFI System Table, its physical offset must be missing from the virtual memory map, and there is really no point in proceeding with installing the virtual memory map and the runtime services dispatch table. So back out gracefully. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- v2: - release mappings and free virtmap before bailing arch/arm64/kernel/efi.c | 28 ++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 56c3327bbf79..23942158e0f8 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -416,11 +416,23 @@ static int __init arm64_enter_virtual_mode(void) continue; if (remap_region(md, virt_md)) ++count; + else + goto err_unmap; How about: if (!remap_region(md, virt_md)) goto err_unmap; ++count; Sure. } efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); - if (efi.systab) - set_bit(EFI_SYSTEM_TABLES, efi.flags); + if (!efi.systab) { + /* + * If we have no virtual mapping for the System Table at this + * point, the memory map doesn't cover the physical offset where + * it resides. This means the System Table will be inaccessible + * to Runtime Services themselves once the virtual mapping is + * installed. + */ + pr_err(Failed to remap EFI System Table -- buggy firmware?\n); + goto err_unmap; + } + set_bit(EFI_SYSTEM_TABLES, efi.flags); local_irq_save(flags); cpu_switch_mm(idmap_pg_dir, init_mm); @@ -453,5 +465,17 @@ static int __init arm64_enter_virtual_mode(void) set_bit(EFI_RUNTIME_SERVICES, efi.flags); return 0; + +err_unmap: + /* unmap all mappings that succeeded: there are 'count' of those */ + for_each_efi_memory_desc(memmap, md) { + if (!(md-attribute EFI_MEMORY_RUNTIME)) + continue; + if (!count--) + break; + iounmap((__force void *)md-virt_addr); + } This is wrong. memmap still belongs to UEFI and hasn't been touched. The new mappings are in virtmap. So, it is even simpler: /* unmap all mappings that succeeded: there are 'count' of those */ for (virt_md = virtmap; count; virt_md++, count--) iounmap((__force void __iomem *)virt_md-virt_addr); Yes, you're right. Will respin. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] efi/arm64: handle missing virtual mapping for UEFI System Table
If we cannot resolve the virtual address of the UEFI System Table, its physical offset must be missing from the virtual memory map, and there is really no point in proceeding with installing the virtual memory map and the runtime services dispatch table. So back out gracefully. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi.c | 28 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 56c3327bbf79..e72f3100958f 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -414,13 +414,24 @@ static int __init arm64_enter_virtual_mode(void) for_each_efi_memory_desc(memmap, md) { if (!(md-attribute EFI_MEMORY_RUNTIME)) continue; - if (remap_region(md, virt_md)) - ++count; + if (!remap_region(md, virt_md)) + goto err_unmap; + ++count; } efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); - if (efi.systab) - set_bit(EFI_SYSTEM_TABLES, efi.flags); + if (!efi.systab) { + /* +* If we have no virtual mapping for the System Table at this +* point, the memory map doesn't cover the physical offset where +* it resides. This means the System Table will be inaccessible +* to Runtime Services themselves once the virtual mapping is +* installed. +*/ + pr_err(Failed to remap EFI System Table -- buggy firmware?\n); + goto err_unmap; + } + set_bit(EFI_SYSTEM_TABLES, efi.flags); local_irq_save(flags); cpu_switch_mm(idmap_pg_dir, init_mm); @@ -453,5 +464,14 @@ static int __init arm64_enter_virtual_mode(void) set_bit(EFI_RUNTIME_SERVICES, efi.flags); return 0; + +err_unmap: + /* unmap all mappings that succeeded: there are 'count' of those */ + for (virt_md = virtmap; count--; virt_md += memmap.desc_size) { + md = virt_md; + iounmap((__force void __iomem *)md-virt_addr); + } + kfree(virtmap); + return -1; } early_initcall(arm64_enter_virtual_mode); -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls
On 4 July 2014 17:45, Catalin Marinas catalin.mari...@arm.com wrote: On Thu, Jun 26, 2014 at 11:09:06AM +0100, Ard Biesheuvel wrote: According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is allowed, and should adhere to the AAPCS64 calling convention, which states that 'only the bottom 64 bits of each value stored in registers v8-v15 need to be preserved' (section 5.1.2). This applies equally to UEFI Runtime Services called by the kernel, so make sure the FP/SIMD register file is preserved in this case. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org While the code looks fine, I think there is a mismatch between what the subject says and what the patch does (enabling EFI_RUNTIME_WRAPPERS). Not entirely. In order to be able to insert calls to kernel_neon_begin()/end() into the runtime services calls, we need a) to supply definitions for efi_call_virt() and __efi_call_virt() that contain those calls to kernel_neon_begin()/end() b) to enable runtime wrappers (which is what uses those definitions) Would you prefer those to be split in 2 patches? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls
On 4 July 2014 18:59, Catalin Marinas catalin.mari...@arm.com wrote: On Fri, Jul 04, 2014 at 04:51:31PM +0100, Ard Biesheuvel wrote: On 4 July 2014 17:45, Catalin Marinas catalin.mari...@arm.com wrote: On Thu, Jun 26, 2014 at 11:09:06AM +0100, Ard Biesheuvel wrote: According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is allowed, and should adhere to the AAPCS64 calling convention, which states that 'only the bottom 64 bits of each value stored in registers v8-v15 need to be preserved' (section 5.1.2). This applies equally to UEFI Runtime Services called by the kernel, so make sure the FP/SIMD register file is preserved in this case. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org While the code looks fine, I think there is a mismatch between what the subject says and what the patch does (enabling EFI_RUNTIME_WRAPPERS). Not entirely. In order to be able to insert calls to kernel_neon_begin()/end() into the runtime services calls, we need a) to supply definitions for efi_call_virt() and __efi_call_virt() that contain those calls to kernel_neon_begin()/end() b) to enable runtime wrappers (which is what uses those definitions) Would you prefer those to be split in 2 patches? No, that's fine. You could just add the above explanation to the commit log. Otherwise: Acked-by: Catalin Marinas catalin.mari...@arm.com OK, thanks. I will add your ack and ask Matt to take it. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] efi/arm64: handle missing virtual mapping for UEFI System Table
On 4 July 2014 19:01, Catalin Marinas catalin.mari...@arm.com wrote: On Fri, Jul 04, 2014 at 05:52:36PM +0100, Mark Salter wrote: On Fri, 2014-07-04 at 17:25 +0200, Ard Biesheuvel wrote: If we cannot resolve the virtual address of the UEFI System Table, its physical offset must be missing from the virtual memory map, and there is really no point in proceeding with installing the virtual memory map and the runtime services dispatch table. So back out gracefully. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- Acked-by: Mark Salter msal...@redhat.com Thanks. Do I take this patch or Matt already has other EFI patches to push? If the latter: Acked-by: Catalin Marinas catalin.mari...@arm.com Thanks. You can take it for 3.16 if you like, otherwise it is probably better if we ask Matt to take it through his tree with some other stuff we have in flight at the moment. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls
According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is allowed, and should adhere to the AAPCS64 calling convention, which states that 'only the bottom 64 bits of each value stored in registers v8-v15 need to be preserved' (section 5.1.2). This applies equally to UEFI Runtime Services called by the kernel, so make sure the FP/SIMD register file is preserved in this case. We do this by enabling the wrappers for UEFI Runtime Services (CONFIG_EFI_RUNTIME_WRAPPERS) and inserting calls to kernel_neon_begin() and kernel_neon_end() into these wrappers. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Acked-by: Catalin Marinas catalin.mari...@arm.com --- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/efi.h | 21 + arch/arm64/kernel/efi.c | 14 +- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a474de346be6..93e11f4d9513 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -299,6 +299,7 @@ config EFI select LIBFDT select UCS2_STRING select EFI_PARAMS_FROM_FDT + select EFI_RUNTIME_WRAPPERS default y help This option provides support for runtime services provided diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index 5a46c4e7f539..375ba342dca6 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -2,6 +2,7 @@ #define _ASM_EFI_H #include asm/io.h +#include asm/neon.h #ifdef CONFIG_EFI extern void efi_init(void); @@ -11,4 +12,24 @@ extern void efi_idmap_init(void); #define efi_idmap_init() #endif +#define efi_call_virt(f, ...) \ +({ \ + efi_##f##_t *__f = efi.systab-runtime-f; \ + efi_status_t __s; \ + \ + kernel_neon_begin();\ + __s = __f(__VA_ARGS__); \ + kernel_neon_end(); \ + __s;\ +}) + +#define __efi_call_virt(f, ...) \ +({ \ + efi_##f##_t *__f = efi.systab-runtime-f; \ + \ + kernel_neon_begin();\ + __f(__VA_ARGS__); \ + kernel_neon_end(); \ +}) + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 14db1f6e8d7f..56c3327bbf79 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -449,19 +449,7 @@ static int __init arm64_enter_virtual_mode(void) /* Set up runtime services function pointers */ runtime = efi.systab-runtime; - efi.get_time = runtime-get_time; - efi.set_time = runtime-set_time; - efi.get_wakeup_time = runtime-get_wakeup_time; - efi.set_wakeup_time = runtime-set_wakeup_time; - efi.get_variable = runtime-get_variable; - efi.get_next_variable = runtime-get_next_variable; - efi.set_variable = runtime-set_variable; - efi.query_variable_info = runtime-query_variable_info; - efi.update_capsule = runtime-update_capsule; - efi.query_capsule_caps = runtime-query_capsule_caps; - efi.get_next_high_mono_count = runtime-get_next_high_mono_count; - efi.reset_system = runtime-reset_system; - + efi_native_runtime_setup(); set_bit(EFI_RUNTIME_SERVICES, efi.flags); return 0; -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code
In order for other archs (such as arm64) to be able to reuse the virtual mode function call wrappers, move them to drivers/firmware/efi/runtime.c. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/x86/Kconfig| 1 + arch/x86/platform/efi/efi.c | 144 +--- drivers/firmware/efi/Kconfig| 7 ++ drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/runtime-wrappers.c | 161 include/linux/efi.h | 2 + 6 files changed, 174 insertions(+), 142 deletions(-) create mode 100644 drivers/firmware/efi/runtime-wrappers.c diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index a8f749ef0fdc..4c3f026aa5ce 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1522,6 +1522,7 @@ config EFI bool EFI runtime service support depends on ACPI select UCS2_STRING + select EFI_RUNTIME_WRAPPERS ---help--- This enables the kernel to use EFI runtime services that are available (such as the EFI variable services). diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 87fc96bcc13c..36d0835210c3 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -104,130 +104,6 @@ static int __init setup_storage_paranoia(char *arg) } early_param(efi_no_storage_paranoia, setup_storage_paranoia); -static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(get_time, tm, tc); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_set_time(efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(set_time, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, -efi_bool_t *pending, -efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(get_wakeup_time, enabled, pending, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) -{ - unsigned long flags; - efi_status_t status; - - spin_lock_irqsave(rtc_lock, flags); - status = efi_call_virt(set_wakeup_time, enabled, tm); - spin_unlock_irqrestore(rtc_lock, flags); - return status; -} - -static efi_status_t virt_efi_get_variable(efi_char16_t *name, - efi_guid_t *vendor, - u32 *attr, - unsigned long *data_size, - void *data) -{ - return efi_call_virt(get_variable, -name, vendor, attr, -data_size, data); -} - -static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, - efi_char16_t *name, - efi_guid_t *vendor) -{ - return efi_call_virt(get_next_variable, -name_size, name, vendor); -} - -static efi_status_t virt_efi_set_variable(efi_char16_t *name, - efi_guid_t *vendor, - u32 attr, - unsigned long data_size, - void *data) -{ - return efi_call_virt(set_variable, -name, vendor, attr, -data_size, data); -} - -static efi_status_t virt_efi_query_variable_info(u32 attr, -u64 *storage_space, -u64 *remaining_space, -u64 *max_variable_size) -{ - if (efi.runtime_version EFI_2_00_SYSTEM_TABLE_REVISION) - return EFI_UNSUPPORTED; - - return efi_call_virt(query_variable_info, attr, storage_space, -remaining_space, max_variable_size); -} - -static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) -{ - return efi_call_virt(get_next_high_mono_count, count); -} - -static void virt_efi_reset_system(int reset_type, - efi_status_t status, - unsigned long data_size, - efi_char16_t *data) -{ - __efi_call_virt(reset_system, reset_type, status
[PATCH v3 0/2] efi/arm64: FP/SIMD preserve/restore during UEFI Runtime Services
Two patches to add FP/SIMD preserve/restore to UEFI Runtime Services: - patch #1 moves runtime services wrappers to generic code so we can reuse them for arm64 - patch #2 enables the wrappers for arm64 and inserts calls to kernel_neon_begin and kernel_neon_end @Matt: please queue up for 3.17 v3: - improve commit message for patch #2 and add Catalin's ack v2: - add Kconfig symbol EFI_RUNTIME_WRAPPERS so we don't break ia64 by enabling it unconditionally Ard Biesheuvel (2): efi/x86: move UEFI Runtime Services wrappers to generic code efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls arch/arm64/Kconfig | 1 + arch/arm64/include/asm/efi.h| 21 + arch/arm64/kernel/efi.c | 14 +-- arch/x86/Kconfig| 1 + arch/x86/platform/efi/efi.c | 144 +--- drivers/firmware/efi/Kconfig| 7 ++ drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/runtime-wrappers.c | 161 include/linux/efi.h | 2 + 9 files changed, 197 insertions(+), 155 deletions(-) create mode 100644 drivers/firmware/efi/runtime-wrappers.c -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services
On 7 July 2014 22:43, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 7 July 2014 22:29, Matt Fleming m...@console-pimps.org wrote: On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote: According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- drivers/firmware/efi/runtime-wrappers.c | 109 +--- 1 file changed, 99 insertions(+), 10 deletions(-) Ard, what's going on with this one? I didn't see it resubmitted along with v3 of efi/arm64: handle missing virtual mapping for UEFI System Table. Note that we already have a lock to serialize access to the UEFI variable services in the form of __efivars-lock, see drivers/firmware/efi/vars.c. It's a spinlock because of the context we may need to create variables in from efi_pstore_write(). As the patch says, the UEFI spec is very clear on which combinations of calls are not reentrant. I don't think the rtc_lock and the efivars-lock quite cover that completely ... After doing a bit more research, I still think there is work needed if we aim to adhere to the UEFI spec, or at least be safe from the hazards it points out. So the current status is: - get/set time calls are serialized with respect to one another using rtc_lock at the wrapper level - get/set variable calls are serialized using the efivars-lock in the efivars module - get_next_variable() calls use the BKL The two things I am most concerned with are: - reset system while other calls are in flight; is this handled implicitly in some other way? - things like settime()/setwakeuptime() and setvariable() poking into the flash at the same time. Perhaps it would be sufficient to have a single spinlock cover all these cases? Or just let efivars grab the rtc_lock as well? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services
On 8 July 2014 11:29, Matt Fleming m...@console-pimps.org wrote: On Tue, 08 Jul, at 10:54:13AM, Ard Biesheuvel wrote: After doing a bit more research, I still think there is work needed if we aim to adhere to the UEFI spec, or at least be safe from the hazards it points out. Note that I never claimed there wasn't a need for an EFI runtime lock, I was just pointing out that you need to consider the efi-pstore scenario, and that a mutex isn't suitable in that case. I did in fact make a half-arsed attempt at introducing a runtime lock here, http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-blkdevid=c0a88ac5b21f3c837121748be2e59e995126a6cb Provided we can get away with a single EFI runtime lock like that patch, your recent efi_call_virt() changes actually make the required patch much simpler, at least for arm64 and x86. Indeed. So the current status is: - get/set time calls are serialized with respect to one another using rtc_lock at the wrapper level The time functions are completely unused on x86, which is why no proper runtime locking exists. It's basically dead code. OK. That may be different on ARM, though ... - get/set variable calls are serialized using the efivars-lock in the efivars module - get_next_variable() calls use the BKL It uses __efivars-lock just like the other variable services. Is that what you meant by BKL? Well, that is what is says in the comment: * ops.get_next_variable() is only called from register_efivars() * or efivar_update_sysfs_entries(), * which is protected by the BKL, so that path is safe. The two things I am most concerned with are: - reset system while other calls are in flight; is this handled implicitly in some other way? No, it isn't handled, so yeah, it needs fixing. I think on x86 we actually wait for other cpus to shutdown before issuing the reset but it's obviously not possible to make that guarantee across architectures. Exactly. - things like settime()/setwakeuptime() and setvariable() poking into the flash at the same time. Perhaps it would be sufficient to have a single spinlock cover all these cases? Or just let efivars grab the rtc_lock as well? I think we need to introduce a separate lock, logically below all other subsystem specific ones (rtc_lock, __efivars-lock, etc). It needs to be the final lock you grab before invoking the runtime services. I don't think the additional complexity of introducing multiple locks to parallelise access to, say, GetTime() and GetVariable(), is really worth the headache. Definitely not without someone making a really compelling case for why they need to squeeze every ounce of performance out of that scenario. I agree. So shall I update my patch to add a single spin_lock that is used by all wrappers? We will likely need the wrappers on ARM as well, only ia64 needs another approach (if desired) -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] efi: implement mandatory locking for UEFI Runtime Services
According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- So this is v2 of the UEFI Runtime Services serialization patch: this time, I use a single spinlock rather than a set of mutexes, resulting in all services to be serialized with respect to all others. Also added handling of NMI state, as this results in some of the restrictions being lifted (x86, ia64 only) One question remains: with the NMI deadlock handling in place, is it really necessary to disable interrupts in all cases? drivers/firmware/efi/runtime-wrappers.c | 167 +--- 1 file changed, 155 insertions(+), 12 deletions(-) diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 10daa4bbb258..5ee3b16498a2 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -14,11 +14,76 @@ * This file is released under the GPLv2. */ +#include linux/bug.h #include linux/efi.h -#include linux/spinlock.h /* spinlock_t */ +#include linux/mutex.h +#include linux/spinlock.h #include asm/efi.h /* + * According to section 7.1 of the UEFI spec, Runtime Services are not fully + * reentrant, and there are particular combinations of calls that need to be + * serialized. + * + * Table 31. Rules for Reentry Into Runtime Services + * ++---+ + * | If previous call is busy in | Forbidden to call | + * ++---+ + * | Any | SetVirtualAddressMap()| + * ++---+ + * | ConvertPointer() | ConvertPointer() | + * ++---+ + * | SetVariable() | ResetSystem() | + * | UpdateCapsule() | | + * | SetTime() | | + * | SetWakeupTime() | | + * | GetNextHighMonotonicCount() | | + * ++---+ + * | GetVariable() | GetVariable() | + * | GetNextVariableName() | GetNextVariableName() | + * | SetVariable() | SetVariable() | + * | QueryVariableInfo() | QueryVariableInfo() | + * | UpdateCapsule() | UpdateCapsule() | + * | QueryCapsuleCapabilities()| QueryCapsuleCapabilities() | + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | + * ++---+ + * | GetTime() | GetTime() | + * | SetTime() | SetTime() | + * | GetWakeupTime() | GetWakeupTime() | + * | SetWakeupTime() | SetWakeupTime() | + * ++---+ + * + * Instead of adding locks for each of the groups, we add a single spinlock + * that serializes all runtime services calls with respect to all others. + */ +static DEFINE_SPINLOCK(efi_runtime_lock); + +/* + * Some runtime services calls can be reentrant under NMI, even if the table + * above says they are not. + * + * Table 32. Functions that may be called after Machine Check ,INIT and NMI + * ++--+ + * | Function | Called after Machine Check, INIT and NMI | + * ++--+ + * | GetTime() | Yes, even if previously busy.| + * | GetVariable() | Yes, even if previously busy | + * | GetNextVariableName() | Yes, even if previously busy | + * | QueryVariableInfo() | Yes, even if previously busy | + * | SetVariable() | Yes, even if previously busy | + * | UpdateCapsule() | Yes, even if previously busy | + * | QueryCapsuleCapabilities()| Yes, even if previously busy | + * | ResetSystem() | Yes, even if previously busy | + * ++--+ + * + * In order to prevent deadlocks under NMI, the wrappers for these functions + * only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi(). + */ +#ifndef efi_in_nmi +#define efi_in_nmi() (0) +#endif
Re: [PATCH v2] efi: implement mandatory locking for UEFI Runtime Services
On 8 July 2014 13:21, Ard Biesheuvel ard.biesheu...@linaro.org wrote: According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- So this is v2 of the UEFI Runtime Services serialization patch: this time, I use a single spinlock rather than a set of mutexes, resulting in all services to be serialized with respect to all others. Also added handling of NMI state, as this results in some of the restrictions being lifted (x86, ia64 only) One question remains: with the NMI deadlock handling in place, is it really necessary to disable interrupts in all cases? I omitted this hunk from the patch by accident: diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 55059a50a01f..d8ee5bb527e5 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -2,6 +2,7 @@ #define _ASM_X86_EFI_H #include asm/i387.h +#include asm/nmi.h /* * We map the EFI regions needed for runtime services non-contiguously, * with preserved alignment on virtual addresses starting from -4G down @@ -52,6 +53,8 @@ extern unsigned long asmlinkage efi_call_phys(void *, ...); kernel_fpu_end(); \ }) +#define efi_in_nmi() in_nmi() + #define efi_ioremap(addr, size, type, attr)ioremap_cache(addr, size) #else /* !CONFIG_X86_32 */ drivers/firmware/efi/runtime-wrappers.c | 167 +--- 1 file changed, 155 insertions(+), 12 deletions(-) diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 10daa4bbb258..5ee3b16498a2 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -14,11 +14,76 @@ * This file is released under the GPLv2. */ +#include linux/bug.h #include linux/efi.h -#include linux/spinlock.h /* spinlock_t */ +#include linux/mutex.h +#include linux/spinlock.h #include asm/efi.h /* + * According to section 7.1 of the UEFI spec, Runtime Services are not fully + * reentrant, and there are particular combinations of calls that need to be + * serialized. + * + * Table 31. Rules for Reentry Into Runtime Services + * ++---+ + * | If previous call is busy in | Forbidden to call | + * ++---+ + * | Any | SetVirtualAddressMap()| + * ++---+ + * | ConvertPointer() | ConvertPointer() | + * ++---+ + * | SetVariable() | ResetSystem() | + * | UpdateCapsule() | | + * | SetTime() | | + * | SetWakeupTime() | | + * | GetNextHighMonotonicCount() | | + * ++---+ + * | GetVariable() | GetVariable() | + * | GetNextVariableName() | GetNextVariableName() | + * | SetVariable() | SetVariable() | + * | QueryVariableInfo() | QueryVariableInfo() | + * | UpdateCapsule() | UpdateCapsule() | + * | QueryCapsuleCapabilities()| QueryCapsuleCapabilities() | + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | + * ++---+ + * | GetTime() | GetTime() | + * | SetTime() | SetTime() | + * | GetWakeupTime() | GetWakeupTime() | + * | SetWakeupTime() | SetWakeupTime() | + * ++---+ + * + * Instead of adding locks for each of the groups, we add a single spinlock + * that serializes all runtime services calls with respect to all others. + */ +static DEFINE_SPINLOCK(efi_runtime_lock); + +/* + * Some runtime services calls can be reentrant under NMI, even if the table + * above says they are not. + * + * Table 32. Functions that may be called after Machine Check ,INIT and NMI + * ++--+ + * | Function | Called after Machine Check, INIT and NMI | + * ++--+ + * | GetTime
[PATCH v3] efi: implement mandatory locking for UEFI Runtime Services
According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Use a spinlock to serialize all Runtime Services with respect to all others, even if this is more than strictly needed. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- v3: Keep value of in_nmi() cached in local scope to help the compiler understand that it cannot change while the function is active v2: So this is v2 of the UEFI Runtime Services serialization patch: this time, I use a single spinlock rather than a set of mutexes, resulting in all services to be serialized with respect to all others. Also added handling of NMI state, as this results in some of the restrictions being lifted (x86, ia64 only) arch/x86/include/asm/efi.h | 2 + drivers/firmware/efi/runtime-wrappers.c | 175 +--- 2 files changed, 165 insertions(+), 12 deletions(-) diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 1eb5f6433ad8..35f67a253e33 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -86,6 +86,8 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size, #endif /* CONFIG_X86_32 */ +#define efi_in_nmi() in_nmi() + extern int add_efi_memmap; extern struct efi_scratch efi_scratch; extern void efi_set_executable(efi_memory_desc_t *md, bool executable); diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 10daa4bbb258..55ac6d7eac1b 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -14,11 +14,76 @@ * This file is released under the GPLv2. */ +#include linux/bug.h #include linux/efi.h -#include linux/spinlock.h /* spinlock_t */ +#include linux/mutex.h +#include linux/spinlock.h #include asm/efi.h /* + * According to section 7.1 of the UEFI spec, Runtime Services are not fully + * reentrant, and there are particular combinations of calls that need to be + * serialized. + * + * Table 31. Rules for Reentry Into Runtime Services + * ++---+ + * | If previous call is busy in | Forbidden to call | + * ++---+ + * | Any | SetVirtualAddressMap()| + * ++---+ + * | ConvertPointer() | ConvertPointer() | + * ++---+ + * | SetVariable() | ResetSystem() | + * | UpdateCapsule() | | + * | SetTime() | | + * | SetWakeupTime() | | + * | GetNextHighMonotonicCount() | | + * ++---+ + * | GetVariable() | GetVariable() | + * | GetNextVariableName() | GetNextVariableName() | + * | SetVariable() | SetVariable() | + * | QueryVariableInfo() | QueryVariableInfo() | + * | UpdateCapsule() | UpdateCapsule() | + * | QueryCapsuleCapabilities()| QueryCapsuleCapabilities() | + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | + * ++---+ + * | GetTime() | GetTime() | + * | SetTime() | SetTime() | + * | GetWakeupTime() | GetWakeupTime() | + * | SetWakeupTime() | SetWakeupTime() | + * ++---+ + * + * Instead of adding locks for each of the groups, we add a single spinlock + * that serializes all runtime services calls with respect to all others. + */ +static DEFINE_SPINLOCK(efi_runtime_lock); + +/* + * Some runtime services calls can be reentrant under NMI, even if the table + * above says they are not. + * + * Table 32. Functions that may be called after Machine Check ,INIT and NMI + * ++--+ + * | Function | Called after Machine Check, INIT and NMI | + * ++--+ + * | GetTime() | Yes, even if previously busy.| + * | GetVariable() | Yes, even if previously busy | + * | GetNextVariableName() | Yes, even if previously busy | + * | QueryVariableInfo() | Yes, even
Re: [PATCH] x86/efi: autoload efivars
On 9 July 2014 08:40, joeyli j...@suse.com wrote: On Tue, Jul 08, 2014 at 01:19:42PM +0100, Ben Hutchings wrote: On Tue, 2014-07-08 at 11:14 +0100, Matt Fleming wrote: On Tue, 08 Jul, at 11:00:58AM, Lee, Chun-Yi wrote: [...] --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -44,6 +44,7 @@ #include linux/io.h #include linux/reboot.h #include linux/bcd.h +#include linux/platform_device.h #include asm/setup.h #include asm/efi.h @@ -780,6 +781,20 @@ void __init efi_late_init(void) efi_bgrt_init(); } +#ifdef CONFIG_EFI_VARS_MODULE +static int __init efi_load_efivars(void) +{ + struct platform_device *pdev; + + if (!efi_enabled(EFI_RUNTIME_SERVICES)) + return 0; + + pdev = platform_device_register_simple(efivars, 0, NULL, 0); + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; +} +device_initcall(efi_load_efivars); +#endif + Could this be moved to drivers/firmware/efi/efi.c? That way the arm64 folks could benefit from it too. It seems like that should work now that efi_enabled() is not specific to x86. Ben. Thanks for Matt and Ban's review, I will move platform device register code to drivers/firmware/efi/efi.c and send version 2 patch. Hi all, I tested the version that is in Matt's -next now and it works fine on arm64. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86/efi: autoload efivars
On 11 July 2014 09:52, Matt Fleming m...@console-pimps.org wrote: On Fri, 11 Jul, at 09:47:23AM, Ard Biesheuvel wrote: Hi all, I tested the version that is in Matt's -next now and it works fine on arm64. Great, thanks Ard. May I add your Tested-by to the patch? Sure, Tested-by: Ard Biesheuvel ard.biesheu...@linaro.org -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] efi/arm64: efistub: don't abort if base of DRAM is occupied
If we fail to relocate the kernel Image to its preferred offset of TEXT_OFFSET bytes above the base of DRAM, accept the lowest alternative mapping available instead of aborting. We may lose a bit of memory at the low end, but we can still proceed normally otherwise. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- This is a proposed bug fix for arm64 platforms that fail to boot through EFI due to the fact that some bits of EFI itself are occupying the low end of DRAM. Note that this code now triggers an 'unused function' warning for efi_relocate_kernel(), as that is no longer used. This warning will disappear automatically once the already queued up EFISTUB refactoring patches will get merged for 3.17. arch/arm64/kernel/efi-stub.c | 18 ++ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 60e98a639ac5..5165b3accefe 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, kernel_size = _edata - _text; if (*image_addr != (dram_base + TEXT_OFFSET)) { kernel_memsize = kernel_size + (_end - _edata); - status = efi_relocate_kernel(sys_table, image_addr, -kernel_size, kernel_memsize, -dram_base + TEXT_OFFSET, -PAGE_SIZE); + status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET, + SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); return status; } - if (*image_addr != (dram_base + TEXT_OFFSET)) { - pr_efi_err(sys_table, Failed to alloc kernel memory\n); - efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; - } - *image_size = kernel_memsize; + memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, + kernel_size); + *image_addr = *reserve_addr + TEXT_OFFSET; + *reserve_size = kernel_memsize; } -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efi/arm64: efistub: don't abort if base of DRAM is occupied
On 14 July 2014 17:25, Ard Biesheuvel ard.biesheu...@linaro.org wrote: If we fail to relocate the kernel Image to its preferred offset of TEXT_OFFSET bytes above the base of DRAM, accept the lowest alternative mapping available instead of aborting. We may lose a bit of memory at the low end, but we can still proceed normally otherwise. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- This is a proposed bug fix for arm64 platforms that fail to boot through EFI due to the fact that some bits of EFI itself are occupying the low end of DRAM. Note that this code now triggers an 'unused function' warning for efi_relocate_kernel(), as that is no longer used. This warning will disappear automatically once the already queued up EFISTUB refactoring patches will get merged for 3.17. arch/arm64/kernel/efi-stub.c | 18 ++ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 60e98a639ac5..5165b3accefe 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, kernel_size = _edata - _text; if (*image_addr != (dram_base + TEXT_OFFSET)) { kernel_memsize = kernel_size + (_end - _edata); - status = efi_relocate_kernel(sys_table, image_addr, -kernel_size, kernel_memsize, -dram_base + TEXT_OFFSET, -PAGE_SIZE); + status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET, + SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); return status; } - if (*image_addr != (dram_base + TEXT_OFFSET)) { - pr_efi_err(sys_table, Failed to alloc kernel memory\n); - efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; - } - *image_size = kernel_memsize; + memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, + kernel_size); + *image_addr = *reserve_addr + TEXT_OFFSET; + *reserve_size = kernel_memsize; This still needs a '+ TEXT_OFFSET' btw -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] arm64: use Image header fields in EFI stub
This is a followup on the RFC series I sent a week ago that changes the EFI stub Image loader to stop using linker arithmetic and build time defines and use data obtained at runtime instead. This series is now rebased on top of Catalin's arm64 for-next/core branch, which contains a relevant set of patches by Mark. This means I could drop my former patch #1 against Documentation/booting.txt. Patch #1 adds asm/image_hdr.h. I incorporated Geoff's feedback to improve the comments and make the header suitable for sharing with userland. Patch #2 contains the changes to the actual stub loader itself. This patch depends on the stub loader bug fix patch I sent out today. It drops all references to linker symbols and uses text_offset and image_size from the Image header, and uses the loaded Image size as reported by EFI. This patch also fixes the corner case where Image happens to be loaded at exactly the right offset, but the allocation is actually too small to satisfy the requirement imposed by image_size as set in the header. Ard Biesheuvel (2): arm64: add C struct definition for Image header arm64/efi: efistub: get text offset and image size from the Image header arch/arm64/include/asm/image_hdr.h | 75 ++ arch/arm64/kernel/Makefile | 2 - arch/arm64/kernel/efi-stub.c | 29 --- 3 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 arch/arm64/include/asm/image_hdr.h -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] arm64: add C struct definition for Image header
In order to be able to interpret the Image header from C code, we need a struct definition that reflects the specification for Image headers as laid out in Documentation/arm64/booting.txt. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/image_hdr.h | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 arch/arm64/include/asm/image_hdr.h diff --git a/arch/arm64/include/asm/image_hdr.h b/arch/arm64/include/asm/image_hdr.h new file mode 100644 index ..9dc74b672124 --- /dev/null +++ b/arch/arm64/include/asm/image_hdr.h @@ -0,0 +1,75 @@ +/* + * image_hdr.h - C struct definition of the arm64 Image header format + * + * Copyright (C) 2014 Linaro Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __ASM_IMAGE_HDR_H +#define __ASM_IMAGE_HDR_H + +#ifdef __KERNEL__ +#include linux/types.h +#else +#include stdint.h +#endif + +/** + * struct arm64_image_hdr - arm64 kernel Image binary header format + * @code0: entry point, first instruction to jump to when booting + * the Image + * @code1: second instruction of entry point + * @text_offset: mandatory offset (little endian) beyond a 2 megabyte + * aligned boundary that needs to be taken into account + * when deciding where to load the image + * @image_size:total size (little endian) of the memory area (counted + * from the offset where the image was loaded) that may be + * used by the booting kernel before memory reservations + * can be honoured + * @flags: little endian bit field + * Bit 0: Kernel endianness. 1 if BE, 0 if LE. + * Bits 1-63: Reserved. + * @res2: reserved, must be 0 + * @res3: reserved, must be 0 + * @res4: reserved, must be 0 + * @magic: Magic number (little endian): ARM\x64 + * @res5: reserved (used for PE COFF offset) + * + * This definition reflects the definition in Documentation/arm64/booting.txt in + * the Linux source tree. + */ +struct arm64_image_hdr { + uint32_t code0; + uint32_t code1; + uint64_t text_offset; + uint64_t image_size; + uint64_t flags; + uint64_t res2; + uint64_t res3; + uint64_t res4; + uint32_t magic; + uint32_t res5; +}; + +static const union { + uint8_t le_val[4]; + uint32_tcpu_val; +} arm64_image_hdr_magic = { + .le_val = ARM\x64 +}; + +/** + * int arm64_image_hdr_check() - check whether hdr points to an arm64 Image + * @hdr: pointer to an arm64 Image + * + * Return: 1 if check is successful, 0 otherwise + */ +static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr) +{ + return hdr-magic == arm64_image_hdr_magic.cpu_val; +} + +#endif -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header
The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense that it needs to use the EFI environment and the Image header at runtime and not rely on the linker or preprocessor to produce values for text offset, image size and kernel size. This patch also fixes the corner case where Image happens to be loaded at exactly the right offset, but the allocation is actually too small to satisfy the requirement imposed by image_size as set in the header. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/Makefile | 2 -- arch/arm64/kernel/efi-stub.c | 29 - 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..99b676eeeb0f 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -4,8 +4,6 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) -CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) \ - -I$(src)/../../../scripts/dtc/libfdt CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 9b61d66e2d20..4ba90b2ef677 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -11,8 +11,7 @@ */ #include linux/efi.h #include asm/efi.h -#include asm/sections.h - +#include asm/image_hdr.h efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, efi_loaded_image_t *image) { efi_status_t status; - unsigned long kernel_size, kernel_memsize = 0; + struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr; + + /* make sure image_addr points to an arm64 kernel Image */ + if (!arm64_image_hdr_check(hdr)) { + pr_efi_err(sys_table, Kernel Image header check failed\n); + return EFI_LOAD_ERROR; + } /* Relocate the image, if required. */ - kernel_size = _edata - _text; - if (*image_addr != (dram_base + TEXT_OFFSET)) { - kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET; - status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M, + if (*image_addr != (dram_base + hdr-text_offset) || + image-image_size hdr-image_size) { + *reserve_size = hdr-text_offset + hdr-image_size; + status = efi_low_alloc(sys_table, *reserve_size, SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); + *reserve_size = 0; return status; } - memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, - kernel_size); - *image_addr = *reserve_addr + TEXT_OFFSET; - *reserve_size = kernel_memsize; + memcpy((void *)*reserve_addr + hdr-text_offset, + (void *)*image_addr, image-image_size); + *image_addr = *reserve_addr + hdr-text_offset; } - - return EFI_SUCCESS; } -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] arm64: add C struct definition for Image header
On 14 July 2014 18:58, Mark Rutland mark.rutl...@arm.com wrote: Hi Ard, On Mon, Jul 14, 2014 at 05:17:50PM +0100, Ard Biesheuvel wrote: In order to be able to interpret the Image header from C code, we need a struct definition that reflects the specification for Image headers as laid out in Documentation/arm64/booting.txt. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/image_hdr.h | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 arch/arm64/include/asm/image_hdr.h diff --git a/arch/arm64/include/asm/image_hdr.h b/arch/arm64/include/asm/image_hdr.h new file mode 100644 index ..9dc74b672124 --- /dev/null +++ b/arch/arm64/include/asm/image_hdr.h @@ -0,0 +1,75 @@ +/* + * image_hdr.h - C struct definition of the arm64 Image header format + * + * Copyright (C) 2014 Linaro Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __ASM_IMAGE_HDR_H +#define __ASM_IMAGE_HDR_H + +#ifdef __KERNEL__ +#include linux/types.h +#else +#include stdint.h +#endif + +/** + * struct arm64_image_hdr - arm64 kernel Image binary header format + * @code0: entry point, first instruction to jump to when booting + * the Image + * @code1: second instruction of entry point + * @text_offset: mandatory offset (little endian) beyond a 2 megabyte + * aligned boundary that needs to be taken into account + * when deciding where to load the image + * @image_size: total size (little endian) of the memory area (counted + * from the offset where the image was loaded) that may be + * used by the booting kernel before memory reservations + * can be honoured + * @flags: little endian bit field + * Bit 0: Kernel endianness. 1 if BE, 0 if LE. + * Bits 1-63: Reserved. + * @res2:reserved, must be 0 + * @res3:reserved, must be 0 + * @res4:reserved, must be 0 + * @magic: Magic number (little endian): ARM\x64 + * @res5:reserved (used for PE COFF offset) + * + * This definition reflects the definition in Documentation/arm64/booting.txt in + * the Linux source tree. + */ Can we not just say See Documentation/arm64/booting.txt rather than duplicating the description? This is at the request of Geoff: he suggested it so we can use generated documentation. Seemed sensible to me ... +struct arm64_image_hdr { + uint32_t code0; + uint32_t code1; + uint64_t text_offset; + uint64_t image_size; + uint64_t flags; + uint64_t res2; + uint64_t res3; + uint64_t res4; + uint32_t magic; + uint32_t res5; +}; + +static const union { + uint8_t le_val[4]; + uint32_tcpu_val; +} arm64_image_hdr_magic = { + .le_val = ARM\x64 +}; + +/** + * int arm64_image_hdr_check() - check whether hdr points to an arm64 Image + * @hdr: pointer to an arm64 Image + * + * Return: 1 if check is successful, 0 otherwise + */ +static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr) +{ + return hdr-magic == arm64_image_hdr_magic.cpu_val; +} Rather than the arm64_image_hdr_magic union trick above, could we not just use the magic inline with a memcmp, e.g. static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr) { return !memcmp(hdr-magic, ARM\x64, 4); } Sure, but I don't think it is necessarily better. Strictly, memcmp() depends on string.h being included, whereas this is just plain C. I'm a little hesitant to expose this to userspace in case the size of the structure grows and userspace starts relying on it having a particular length (though perhaps that's unfounded). Well, the struct does not cover what comes right after it, so in that sense is irrelevant. Perhaps you would like to version the header just in case? (Not such a bad idea anyway) It's also a little unfortunate that we lose the nice endianness annotations here, as it gives a wrong impression of the structure as a set of native-endian fields. Yes, that is indeed unfortunate. I did entertain the idea of using __le[32|64] in the struct, and typedef'ing them to uint[32|64]_t in the !__KERNEL__ case. If we then use memcmp() instead of the union to check the magic, we can do so without triggering sparse endianness warnings. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header
On 14 July 2014 18:54, Mark Rutland mark.rutl...@arm.com wrote: Hi Ard, On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote: The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense that it needs to use the EFI environment and the Image header at runtime and not rely on the linker or preprocessor to produce values for text offset, image size and kernel size. Could you elaborate on _why_ we can't do that, given it's linked into the kernel Image? Are we splitting the stub from the kernel? What's going on? Perhaps Leif can chime in here? I was under the impression that you were aligned in this regard. This patch also fixes the corner case where Image happens to be loaded at exactly the right offset, but the allocation is actually too small to satisfy the requirement imposed by image_size as set in the header. It's not really imposed by image_size. While it's described by image_size it's imposed by the existence of the BSS section and the initial page tables. Yes, that is true. But from stub POV, it doesn't matter /why/ image_size has a particular value. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/Makefile | 2 -- arch/arm64/kernel/efi-stub.c | 29 - 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..99b676eeeb0f 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -4,8 +4,6 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) -CFLAGS_efi-stub.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) \ --I$(src)/../../../scripts/dtc/libfdt CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 9b61d66e2d20..4ba90b2ef677 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -11,8 +11,7 @@ */ #include linux/efi.h #include asm/efi.h -#include asm/sections.h - +#include asm/image_hdr.h efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, efi_loaded_image_t *image) { efi_status_t status; - unsigned long kernel_size, kernel_memsize = 0; + struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr; + + /* make sure image_addr points to an arm64 kernel Image */ + if (!arm64_image_hdr_check(hdr)) { + pr_efi_err(sys_table, Kernel Image header check failed\n); + return EFI_LOAD_ERROR; + } Surely this should always be the case if the stub is linked into the kernel? It would be nice to know the rationale for this. Well, there is an actual issue addressed by this: the PE/COFF .text section covers everything from stext to _edata, so it does not cover the header itself. In fact, PE/COFF does not allow the headers themselves to be covered by a section (or at least, the Tianocore/EDK2 UEFI implementation does not). So the pointer points *outside* of the .text section, and we are assuming that whatever is at that [negative] offset in the file was also copied to the same offset in memory.(For instance, there is no reason to assume that all implementations of EFI will copy data that is not part of any section to RAM when booting an image located in NOR flash) /* Relocate the image, if required. */ - kernel_size = _edata - _text; - if (*image_addr != (dram_base + TEXT_OFFSET)) { - kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET; - status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M, + if (*image_addr != (dram_base + hdr-text_offset) || + image-image_size hdr-image_size) { As far as I can tell the size of the Image (image-image_size) is always going to be less than the effective run time image size (hdr-image_size). Currently, yes. The SizeOfImage field in head.S which I assume image-image_size is derived from (not having a UEFI spec in front of me) seems to cover everything up to _edata but skips the BSS, and initial page tables, but this is covered by the header's image_size field. Am I missing something? Surely we _always_ expect image-image_size to be smaller than hdr-image_size? At some point, we may extend the virtual size of .text all the way to _end. (Without growing the actual payload, the remainder will be zero initialized by the loader) -- Ard. Cheers, Mark. + *reserve_size = hdr-text_offset + hdr-image_size; + status = efi_low_alloc(sys_table, *reserve_size, SZ_2M, reserve_addr); if (status != EFI_SUCCESS
Re: [PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header
On 14 July 2014 20:29, Mark Rutland mark.rutl...@arm.com wrote: On Mon, Jul 14, 2014 at 06:35:32PM +0100, Ard Biesheuvel wrote: On 14 July 2014 18:54, Mark Rutland mark.rutl...@arm.com wrote: Hi Ard, On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote: The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense that it needs to use the EFI environment and the Image header at runtime and not rely on the linker or preprocessor to produce values for text offset, image size and kernel size. Could you elaborate on _why_ we can't do that, given it's linked into the kernel Image? Are we splitting the stub from the kernel? What's going on? Perhaps Leif can chime in here? I was under the impression that you were aligned in this regard. Perhaps we were, and I can see reasons for doing this (e.g. as a step towards making it possible to boot a BE kernel using UEFI), but the commit message doesn't mention any such reason. For the sake of my poor recollection and that of others, some words in the commit message would be helpful. OK This patch also fixes the corner case where Image happens to be loaded at exactly the right offset, but the allocation is actually too small to satisfy the requirement imposed by image_size as set in the header. It's not really imposed by image_size. While it's described by image_size it's imposed by the existence of the BSS section and the initial page tables. Yes, that is true. But from stub POV, it doesn't matter /why/ image_size has a particular value. Sure, the stub is a piece of software with no particular knowledge of anything. However, those of us reading the commit message are not, and for our sake it would be nice to have a description in case we care as to /why/ image_size has a particular value and why that value happens to be useful here. As far as I can see, we needed to allocate memory for the BSS even before the image_size field was introduced, so this isn't a new requirement. All that's needed is a change to the wording of the commit message to explain that we're trying to allocate space for (runtime-initialised) data between _edata and _end rather than how we find out how much space we need for that (reading the image_size field). OK Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/Makefile | 2 -- arch/arm64/kernel/efi-stub.c | 29 - 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..99b676eeeb0f 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -4,8 +4,6 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) -CFLAGS_efi-stub.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) \ --I$(src)/../../../scripts/dtc/libfdt CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 9b61d66e2d20..4ba90b2ef677 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -11,8 +11,7 @@ */ #include linux/efi.h #include asm/efi.h -#include asm/sections.h - +#include asm/image_hdr.h efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, efi_loaded_image_t *image) { efi_status_t status; - unsigned long kernel_size, kernel_memsize = 0; + struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr; + + /* make sure image_addr points to an arm64 kernel Image */ + if (!arm64_image_hdr_check(hdr)) { + pr_efi_err(sys_table, Kernel Image header check failed\n); + return EFI_LOAD_ERROR; + } Surely this should always be the case if the stub is linked into the kernel? It would be nice to know the rationale for this. Well, there is an actual issue addressed by this: the PE/COFF .text section covers everything from stext to _edata, so it does not cover the header itself. In fact, PE/COFF does not allow the headers themselves to be covered by a section (or at least, the Tianocore/EDK2 UEFI implementation does not). So the pointer points *outside* of the .text section, and we are assuming that whatever is at that [negative] offset in the file was also copied to the same offset in memory.(For instance, there is no reason to assume that all implementations of EFI will copy data that is not part of any section to RAM when booting an image located in NOR flash) If we are making assumptions which aren't always valid, then why the hell are we making them in the first place, and then trying
[PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction. However, the header is not covered by any PE/COFF section, so the header may not actually be loaded at the expected offset. So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-entry.S | 2 +- arch/arm64/kernel/head.S | 10 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 619b1dd7bcde..6ef541731d9e 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - mov x21, x0 + add x21, x0, #:lo12:stext_offset /* * Flush dcache covering current runtime addresses diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index a2c1195abb7f..78ddae28b090 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -137,6 +137,8 @@ efi_head: #endif #ifdef CONFIG_EFI + .globl stext_offset + .setstext_offset, stext - efi_head .align 3 pe_header: .ascii PE @@ -160,7 +162,7 @@ optional_header: .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint - .long stext - efi_head// BaseOfCode + .long stext_offset// BaseOfCode extra_header_fields: .quad 0 // ImageBase @@ -177,7 +179,7 @@ extra_header_fields: .long _edata - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header - .long stext - efi_head// SizeOfHeaders + .long stext_offset// SizeOfHeaders .long 0 // CheckSum .short 0xa // Subsystem (EFI application) .short 0 // DllCharacteristics @@ -222,9 +224,9 @@ section_table: .byte 0 .byte 0 // end of 0 padding of section name .long _edata - stext // VirtualSize - .long stext - efi_head// VirtualAddress + .long stext_offset// VirtualAddress .long _edata - stext // SizeOfRawData - .long stext - efi_head// PointerToRawData + .long stext_offset// PointerToRawData .long 0 // PointerToRelocations (0 for executables) .long 0 // PointerToLineNumbers (0 for executables) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] arm64/efi: efistub: jump to 'stext' directly, not through the header
After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction. However, the header is not covered by any PE/COFF section, so the header may not actually be loaded at the expected offset. So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section, by supplying a symbol 'stext_offset' to efi-entry.o which contains the relative offset of stext into the Image. Also replace other open coded calculations of the same value with a reference to 'stext_offset' Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-entry.S | 3 ++- arch/arm64/kernel/head.S | 10 ++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 619b1dd7bcde..a0016d3a17da 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -61,7 +61,8 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - mov x21, x0 + ldr x21, =stext_offset + add x21, x0, x21 /* * Flush dcache covering current runtime addresses diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index a2c1195abb7f..78ddae28b090 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -137,6 +137,8 @@ efi_head: #endif #ifdef CONFIG_EFI + .globl stext_offset + .setstext_offset, stext - efi_head .align 3 pe_header: .ascii PE @@ -160,7 +162,7 @@ optional_header: .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint - .long stext - efi_head// BaseOfCode + .long stext_offset// BaseOfCode extra_header_fields: .quad 0 // ImageBase @@ -177,7 +179,7 @@ extra_header_fields: .long _edata - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header - .long stext - efi_head// SizeOfHeaders + .long stext_offset// SizeOfHeaders .long 0 // CheckSum .short 0xa // Subsystem (EFI application) .short 0 // DllCharacteristics @@ -222,9 +224,9 @@ section_table: .byte 0 .byte 0 // end of 0 padding of section name .long _edata - stext // VirtualSize - .long stext - efi_head// VirtualAddress + .long stext_offset// VirtualAddress .long _edata - stext // SizeOfRawData - .long stext - efi_head// PointerToRawData + .long stext_offset// PointerToRawData .long 0 // PointerToRelocations (0 for executables) .long 0 // PointerToLineNumbers (0 for executables) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text
The static memory footprint of a kernel Image at boot is larger than the Image file itself. Things like .bss data and initial page tables are allocated statically but populated dynamically so their content is not contained in the Image file. However, if EFI has loaded the Image at precisely the desired offset of base of DRAM + TEXT_OFFSET, the Image will be booted in place, and we have to make sure that the allocation done by the EFI loader is large enough. Fix this by growing the PE/COFF .text section to cover the entire static memory footprint. The part of the section that is not covered by the payload will be zero initialised by the EFI loader. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/head.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 78ddae28b090..bbf2489939c2 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -158,7 +158,7 @@ optional_header: .short 0x20b // PE32+ format .byte 0x02// MajorLinkerVersion .byte 0x14// MinorLinkerVersion - .long _edata - stext // SizeOfCode + .long _end - stext// SizeOfCode .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint @@ -176,7 +176,7 @@ extra_header_fields: .short 0 // MinorSubsystemVersion .long 0 // Win32VersionValue - .long _edata - efi_head // SizeOfImage + .long _end - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header .long stext_offset// SizeOfHeaders @@ -223,7 +223,7 @@ section_table: .byte 0 .byte 0 .byte 0 // end of 0 padding of section name - .long _edata - stext // VirtualSize + .long _end - stext// VirtualSize .long stext_offset// VirtualAddress .long _edata - stext // SizeOfRawData .long stext_offset// PointerToRawData -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
On 15 July 2014 13:31, Mark Rutland mark.rutl...@arm.com wrote: diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 619b1dd7bcde..6ef541731d9e 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - mov x21, x0 + add x21, x0, #:lo12:stext_offset I think we can drop the :lo12: here, which will allow us to have a warning if stext_offset is unexpectedly large (I believe this will currently silently mask bits were that to happen?). There is no alternative lo12 relocation that errors out when the value does not fit, so it would have to use a literal instead. Ah, that's a shame. What happens when the value doesn't fit if the linker / assembler don't error out? Obviously, it will jump into the wrong place if stext ever gets pushed beyond a 4k boundary, which is not that likely (current value is 0x160, we may want to up it to 0x200 at some point if we need to increase the PE/COFF section alignment, which some versions of the (poor) PE/COFF documentation say should be 0x200 at the minimum) However, doing ldr x21, =stext_offset works fine as well That sounds like a toolchain bug if they're silently doing the wrong thing. Meh, don't think so, really. You get what you pay for, and :lo12: just isn't supposed to care. (It's mainly used with adrp to get a +/- 4gb PC relative reach) -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] arm64/efi: efistub: reuse EFI mapping for Image if it is lower
The EFI loader may load Image at any available offset. This means Image may reside very close to or at the base of DRAM, in which case the relocation done by efi_entry() results in Image being moved up in memory, which is undesirable (memory below the kernel is currently not usable). To work around this, we check in the stub whether the relocated offset is higher than the original offset. If this is the case, the original and relocated Images switch roles, and we move the original Image inside its original lower mapping while executing from the relocated Image. To ensure the original mapping has sufficient size, we add 2 megs + TEXT_OFFSET of padding to the PE/COFF .text section, this should be sufficient to cover rounding and adding TEXT_OFFSET. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- This was tested on a Foundation Model using the patch below, which forces the Image to be loaded at 0x8000 (base of DRAM). diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 2b7bd5eff776..eeadc073bad3 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -138,12 +138,12 @@ pe_header: .short 0 coff_header: .short 0xaa64 // AArch64 - .short 2 // nr_sections + .short 1 // nr_sections .long 0 // TimeDateStamp .long 0 // PointerToSymbolTable .long 1 // NumberOfSymbols .short section_table - optional_header // SizeOfOptionalHeader - .short 0x206 // Characteristics. + .short 0x207 // Characteristics. // IMAGE_FILE_DEBUG_STRIPPED | // IMAGE_FILE_EXECUTABLE_IMAGE | // IMAGE_FILE_LINE_NUMS_STRIPPED @@ -158,7 +158,7 @@ optional_header: .long stext_offset// BaseOfCode extra_header_fields: - .quad 0 // ImageBase + .quad 0x8000 // ImageBase .long 0x20// SectionAlignment .long 0x8 // FileAlignment .short 0 // MajorOperatingSystemVersion @@ -193,6 +193,7 @@ extra_header_fields: // Section table section_table: +#if 0 /* * The EFI application loader requires a relocation section * because EFI applications must be relocatable. This is a @@ -212,6 +213,7 @@ section_table: .long 0x42100040 // Characteristics (section flags) +#endif .ascii .text .byte 0 .byte 0 arch/arm64/kernel/Makefile| 1 + arch/arm64/kernel/efi-entry.S | 48 +-- arch/arm64/kernel/head.S | 7 --- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..d55da3cd8a97 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -6,6 +6,7 @@ CPPFLAGS_vmlinux.lds:= -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) \ -I$(src)/../../../scripts/dtc/libfdt +AFLAGS_efi-entry.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index a0016d3a17da..d154f3c73937 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -11,6 +11,7 @@ */ #include linux/linkage.h #include linux/init.h +#include linux/sizes.h #include asm/assembler.h @@ -45,10 +46,13 @@ ENTRY(efi_stub_entry) * efi_system_table_t *sys_table, * unsigned long *image_addr) ; */ - adrpx8, _text - add x8, x8, #:lo12:_text + adrpx22, _text + add x22, x22, #:lo12:_text // x22: base of Image + adrpx24, _edata + add x24, x24, #:lo12:_edata + sub x24, x24, x22 // x24: size of Image add x2, sp, 16 - str x8, [x2] + str x22, [x2] bl efi_entry cmn x0, #1 b.eqefi_load_fail @@ -61,19 +65,41 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - ldr x21, =stext_offset - add x21, x0, x21 + ldr x23, =stext_offset + add x21, x0, x23 /* +* Check whether the original allocation done by the EFI loader is more
Re: [RFC PATCH] arm64/efi: efistub: reuse EFI mapping for Image if it is lower
On 16 July 2014 16:10, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-15 at 18:50 +0200, Ard Biesheuvel wrote: The EFI loader may load Image at any available offset. This means Image may reside very close to or at the base of DRAM, in which case the relocation done by efi_entry() results in Image being moved up in memory, which is undesirable (memory below the kernel is currently not usable). To work around this, we check in the stub whether the relocated offset is higher than the original offset. If this is the case, the original and relocated Images switch roles, and we move the original Image inside its original lower mapping while executing from the relocated Image. To ensure the original mapping has sufficient size, we add 2 megs + TEXT_OFFSET of padding to the PE/COFF .text section, this should be sufficient to cover rounding and adding TEXT_OFFSET. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- This was tested on a Foundation Model using the patch below, which forces the Image to be loaded at 0x8000 (base of DRAM). diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 2b7bd5eff776..eeadc073bad3 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -138,12 +138,12 @@ pe_header: .short 0 coff_header: .short 0xaa64 // AArch64 - .short 2 // nr_sections + .short 1 // nr_sections .long 0 // TimeDateStamp .long 0 // PointerToSymbolTable .long 1 // NumberOfSymbols .short section_table - optional_header // SizeOfOptionalHeader - .short 0x206 // Characteristics. + .short 0x207 // Characteristics. // IMAGE_FILE_DEBUG_STRIPPED | // IMAGE_FILE_EXECUTABLE_IMAGE | // IMAGE_FILE_LINE_NUMS_STRIPPED @@ -158,7 +158,7 @@ optional_header: .long stext_offset// BaseOfCode extra_header_fields: - .quad 0 // ImageBase + .quad 0x8000 // ImageBase .long 0x20// SectionAlignment .long 0x8 // FileAlignment .short 0 // MajorOperatingSystemVersion @@ -193,6 +193,7 @@ extra_header_fields: // Section table section_table: +#if 0 /* * The EFI application loader requires a relocation section * because EFI applications must be relocatable. This is a @@ -212,6 +213,7 @@ section_table: .long 0x42100040 // Characteristics (section flags) +#endif .ascii .text .byte 0 .byte 0 arch/arm64/kernel/Makefile| 1 + arch/arm64/kernel/efi-entry.S | 48 +-- arch/arm64/kernel/head.S | 7 --- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..d55da3cd8a97 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -6,6 +6,7 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_efi-stub.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) \ -I$(src)/../../../scripts/dtc/libfdt +AFLAGS_efi-entry.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index a0016d3a17da..d154f3c73937 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -11,6 +11,7 @@ */ #include linux/linkage.h #include linux/init.h +#include linux/sizes.h #include asm/assembler.h @@ -45,10 +46,13 @@ ENTRY(efi_stub_entry) * efi_system_table_t *sys_table, * unsigned long *image_addr) ; */ - adrpx8, _text - add x8, x8, #:lo12:_text + adrpx22, _text + add x22, x22, #:lo12:_text // x22: base of Image Why change x8 to x22? Seems unnecessary. Not only that, it is callee-saved, but you don't save it. This could cause a problem if boot fails and we try to return to firmware. + adrpx24, _edata + add x24, x24, #:lo12:_edata + sub x24, x24, x22 // x24: size of Image Should use a caller-saved register instead of x24. Yes, you are right, I moved it to x22 so I can reuse the value after the call to efi_entry(). I will correct it when/if I do a v2, i.e., if there is any demand
Re: [PATCH v2] arm64/efi: efistub: jump to 'stext' directly, not through the header
On 16 July 2014 23:03, Mark Salter msal...@redhat.com wrote: On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote: On 16 July 2014 21:45, Mark Salter msal...@redhat.com wrote: On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote: On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote: On Tue, 2014-07-15 at 12:58 +0200, Ard Biesheuvel wrote: After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction. However, the header is not covered by any PE/COFF section, so the header may not actually be loaded at the expected offset. So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section, by supplying a symbol 'stext_offset' to efi-entry.o which contains the relative offset of stext into the Image. Also replace other open coded calculations of the same value with a reference to 'stext_offset' Have you actually seen a situation where the header isn't there? Isn't the kernel header actually part of the pe/coff file and firmware loads the whole file into RAM? From my understanding of Ard's earlier comments, this part isn't guaranteed per the UEFI spec. I would rather we weren't relying on implementation details. Could be. I didn't see anything about it in the UEFI spec, but I probably wasn't exhaustive in my search. In any case, there's at least one other place broken if the kernel header isn't included in the loaded image. I have not been able to find anything in the PE/COFF documents that tells you what to put in memory areas that are not covered by a section. Expecting the header to be there is indeed relying on an implementation detail, which seems risky. And indeed, if there are any other (non EFI related) uses of header fields in the kernel, it would be good to have a look at those well, I think we need to come up with a loader which does load an image without kernel header so that we can test. Otherwise, we'll probably end up with buggy code anyway. The stub code assumes the the loaded image pointed to by the system table is the whole image. Seems like we'd need to add code to determine if it is whole kernel image or image without initial header. Stub would have to handle both cases. For instance, one case would want image placed at 2MiB+TEXT_OFFSET, other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header). No, this has nothing to do with misaligned data. The PE/COFF .text section does not start at virtual offset #0 but at virtual offset 'stext - efi_head'. In other words, there is a hole in the virtual image where the header is supposed to be. So if there is no PE/COFF section describing what data should be put at offset #0 by the loader, we can't assume the header is there, even if ImageBase does start at #0 Am I just missing something here? Your arm64/efi: efistub: get text offset and image size from the Image header patch makes no sense if we can't rely on header being there. No, you're right, I reversed my position after doing that work and digging into the details. I will drop those patches and proposed this one instead. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] efi/arm64: handle missing virtual mapping for UEFI System Table
On 7 July 2014 22:38, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 7 July 2014 22:11, Matt Fleming m...@console-pimps.org wrote: On Sat, 05 Jul, at 08:25:35AM, Catalin Marinas wrote: OK, I’ll leave it to Matt then together with the other EFI patches he’s already queuing. Thanks guys. What release are we targetting with this patch? Is it a bug fix for something likely to affect people? Have there been reports of users actually hitting this issue? We're at -rc4 now, so there's still time for getting bug fixes to Linus but we may start being asked for justification from the tip folks en route to Linus. If it's just a case of improving the error paths, can it wait until v3.17? This fixes a potential panic under buggy firmware, but no reports of this actual bug are known, so there is no justification for a late merge for 3.16. Hey Matt, I was wondering if you're on track to queue up your -next branch for 3.17? If so, could you please include $subject? And if you don't mind, could you squash the following fixup into efi: efistub: Convert into static library as well? diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..afaeb734295a 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -4,8 +4,7 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) -CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) \ - -I$(src)/../../../scripts/dtc/libfdt +CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg Much appreciated, Cheers, Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] efi/arm64: handle missing virtual mapping for UEFI System Table
On 18 July 2014 21:31, Matt Fleming m...@console-pimps.org wrote: On Fri, 18 Jul, at 08:42:27PM, Ard Biesheuvel wrote: Hey Matt, I was wondering if you're on track to queue up your -next branch for 3.17? If so, could you please include $subject? And if you don't mind, could you squash the following fixup into efi: efistub: Convert into static library as well? Umm, well crap, how did I manage to drop this patch? I've picked this up now (with all the Acks), thanks. Please could you go through the EFI 'next' branch and just make sure there's nothing missing for v3.17? Thanks, looks lovely. The only thing I notice now is that the commit message from the patch below is missing the second line, which used to read #including .c files to building the or something to that effect. Obviously, the leading hash sign resulted in this line having been dropped, so this is something that has been in the patches all along. IMO there is really no point in bothering fixing this, but perhaps your inner grammar nazi disagrees. Cheers, Ard. I was really hoping to get my queue sent to tip today, but I can always send a second pull if it turns out I did miss something. diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index cdaedad3afe5..afaeb734295a 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -4,8 +4,7 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) -CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) \ - -I$(src)/../../../scripts/dtc/libfdt +CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg Sure, I've fixed this up. -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 02/10] arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text
The static memory footprint of a kernel Image at boot is larger than the Image file itself. Things like .bss data and initial page tables are allocated statically but populated dynamically so their content is not contained in the Image file. However, if EFI has loaded the Image at precisely the desired offset of base of DRAM + TEXT_OFFSET, the Image will be booted in place, and we have to make sure that the allocation done by the EFI loader is large enough. Fix this by growing the PE/COFF .text section to cover the entire static memory footprint. The part of the section that is not covered by the payload will be zero initialised by the EFI loader. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/head.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 5cd1f3491df5..c63f44f20ae3 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -150,7 +150,7 @@ optional_header: .short 0x20b // PE32+ format .byte 0x02// MajorLinkerVersion .byte 0x14// MinorLinkerVersion - .long _edata - stext // SizeOfCode + .long _end - stext// SizeOfCode .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint @@ -168,7 +168,7 @@ extra_header_fields: .short 0 // MinorSubsystemVersion .long 0 // Win32VersionValue - .long _edata - efi_head // SizeOfImage + .long _end - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header .long stext_offset// SizeOfHeaders @@ -215,7 +215,7 @@ section_table: .byte 0 .byte 0 .byte 0 // end of 0 padding of section name - .long _edata - stext // VirtualSize + .long _end - stext// VirtualSize .long stext_offset// VirtualAddress .long _edata - stext // SizeOfRawData .long stext_offset// PointerToRawData -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 04/10] arm64: add EFI little endian constants to linker script
Similar to how text offset and kernel size are mangled to produce little endian constants for the Image header regardless of the endianness of the kernel, this adds a number of constants used in the EFI PE/COFF header which can only be calculated (and byte swapped) by the linker. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/image.h | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kernel/image.h b/arch/arm64/kernel/image.h index 8fae0756e175..f5a2f298810d 100644 --- a/arch/arm64/kernel/image.h +++ b/arch/arm64/kernel/image.h @@ -37,8 +37,10 @@ (((data) 0xff00) 24) |\ (((data) 0x00ff) 40) |\ (((data) 0xff00) 56)) +#define DATA_LE32(data) (DATA_LE64(data) 32) #else #define DATA_LE64(data) ((data) 0x) +#define DATA_LE32(data) ((data) 0x) #endif #ifdef CONFIG_CPU_BIG_ENDIAN @@ -57,6 +59,18 @@ #define HEAD_SYMBOLS \ _kernel_size_le = DATA_LE64(_end - _text); \ _kernel_offset_le = DATA_LE64(TEXT_OFFSET); \ - _kernel_flags_le= DATA_LE64(__HEAD_FLAGS); + _kernel_flags_le= DATA_LE64(__HEAD_FLAGS); \ + EFI_HEAD_SYMBOLS + +#ifdef CONFIG_EFI +#define EFI_HEAD_SYMBOLS \ + _efi_stext_offset_le= DATA_LE32(stext_offset); \ + _efi_code_virtsize_le = DATA_LE32(_end - _text - stext_offset); \ + _efi_code_rawsize_le= DATA_LE32(_edata - _text - stext_offset); \ + _efi_image_size_le = DATA_LE32(_end - _text); \ + _efi_entry_point_le = DATA_LE32(efi_stub_entry - _text); +#else +#define EFI_HEAD_SYMBOLS +#endif #endif /* __ASM_IMAGE_H */ -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 03/10] arm64: add macros to emit little endian ASM constants
The Image header contains many constants that should be emitted in little endian regardless of the endianness of the kernel. Add helper macros le16, le32 and le64 to asm/assembler.h to aid with this. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/assembler.h | 18 ++ 1 file changed, 18 insertions(+) diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 5901480bfdca..7db7c946f73f 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -155,3 +155,21 @@ lr .reqx30 // link register #endif orr \rd, \lbits, \hbits, lsl #32 .endm + + /* +* Define LE constants +*/ + .macro le16, x + .byte \x 0xff + .byte (\x 8) 0xff + .endm + + .macro le32, x + le16\x + le16\x 16 + .endm + + .macro le64, x + le32\x + le32\x 32 + .endm -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 00/10] arm64: boot BE kernels from UEFI
This series adds support for booting BE kernels from UEFI. As UEFI is defined to be strictly little endian, some workarounds are required to combine a little endian EFI stub with a big endian kernel. Also, runtime services need to be wrapped so they can be executed in little endian mode. Patches #1 and #2 have been sent to the list before, but are included for completeness. Patch #3, #4 and #5 modify the PE/COFF header definition so it is always emitted in little endian, regardless of the endianness of the kernel. Patch #6 removes references to linker defined symbols like _text and _edata from the stub, as the stub is now a standalone little endian binary that is wrapped in a big endian object file, and _text and _edata are unavailable or meaningless in that context. Patch #7 adds the Makefile changes and some code to build the standalone stub and wrap it. Patch #8 updates the kernel code that references data in UEFI memory to byte reverse it if required. Patch #9 adds runtime services wrappers for the variable store services so they can be called from the big endian kernel. Other runtime services are not implemented for now. Patch #10 enables everything by adding the Kconfig logic. This is tested on Foundation model and FVP Base model. Booting works fine on both, however, enumerating the variable store works only on FVP Base, and is not debuggable on Foundation Model, so I would really appreciate any insights into the code in patch #9 that may be causing this. There is some trickery regarding en-/disabling caches and MMU and surely I have gotten something wrong there. Ard Biesheuvel (10): arm64/efi: efistub: jump to 'stext' directly, not through the header arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text arm64: add macros to emit little endian ASM constants arm64: add EFI little endian constants to linker script arm64/efi: update the PE/COFF header to be endian agnostic arm64/efi: efistub: avoid using linker defined constants arm64/efi: efistub: add support for booting a BE kernel arm64/efi: use LE accessors to access UEFI data arm64/efi: enable minimal UEFI Runtime Services for big endian arm64: Kconfig: enable UEFI on BE kernels arch/arm64/Kconfig | 10 ++- arch/arm64/include/asm/assembler.h | 18 + arch/arm64/include/asm/efi.h| 2 + arch/arm64/kernel/Makefile | 7 +- arch/arm64/kernel/efi-be-call.S | 55 +++ arch/arm64/kernel/efi-be-runtime.c | 104 arch/arm64/kernel/efi-entry.S | 41 --- arch/arm64/kernel/efi-stub.c| 11 ++- arch/arm64/kernel/efi.c | 68 +++--- arch/arm64/kernel/efistub-le/Makefile | 52 ++ arch/arm64/kernel/efistub-le/efi-le-entry.S | 13 arch/arm64/kernel/efistub-le/efistub-le.lds | 35 ++ arch/arm64/kernel/efistub-le/le.h | 12 arch/arm64/kernel/efistub-le/strstr.c | 20 ++ arch/arm64/kernel/head.S| 50 +++-- arch/arm64/kernel/image.h | 16 - drivers/firmware/efi/efi.c | 26 --- drivers/firmware/efi/efivars.c | 2 +- drivers/firmware/efi/libstub/fdt.c | 4 ++ 19 files changed, 466 insertions(+), 80 deletions(-) create mode 100644 arch/arm64/kernel/efi-be-call.S create mode 100644 arch/arm64/kernel/efi-be-runtime.c create mode 100644 arch/arm64/kernel/efistub-le/Makefile create mode 100644 arch/arm64/kernel/efistub-le/efi-le-entry.S create mode 100644 arch/arm64/kernel/efistub-le/efistub-le.lds create mode 100644 arch/arm64/kernel/efistub-le/le.h create mode 100644 arch/arm64/kernel/efistub-le/strstr.c -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 01/10] arm64/efi: efistub: jump to 'stext' directly, not through the header
After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction. However, the header is not covered by any PE/COFF section, so the header may not actually be loaded at the expected offset. So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section, by supplying a symbol 'stext_offset' to efi-entry.o which contains the relative offset of stext into the Image. Also replace other open coded calculations of the same value with a reference to 'stext_offset' Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-entry.S | 3 ++- arch/arm64/kernel/head.S | 10 ++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 619b1dd7bcde..a0016d3a17da 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -61,7 +61,8 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - mov x21, x0 + ldr x21, =stext_offset + add x21, x0, x21 /* * Flush dcache covering current runtime addresses diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 69dafe9621fd..5cd1f3491df5 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -129,6 +129,8 @@ efi_head: #endif #ifdef CONFIG_EFI + .globl stext_offset + .setstext_offset, stext - efi_head .align 3 pe_header: .ascii PE @@ -152,7 +154,7 @@ optional_header: .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint - .long stext - efi_head// BaseOfCode + .long stext_offset// BaseOfCode extra_header_fields: .quad 0 // ImageBase @@ -169,7 +171,7 @@ extra_header_fields: .long _edata - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header - .long stext - efi_head// SizeOfHeaders + .long stext_offset// SizeOfHeaders .long 0 // CheckSum .short 0xa // Subsystem (EFI application) .short 0 // DllCharacteristics @@ -214,9 +216,9 @@ section_table: .byte 0 .byte 0 // end of 0 padding of section name .long _edata - stext // VirtualSize - .long stext - efi_head// VirtualAddress + .long stext_offset// VirtualAddress .long _edata - stext // SizeOfRawData - .long stext - efi_head// PointerToRawData + .long stext_offset// PointerToRawData .long 0 // PointerToRelocations (0 for executables) .long 0 // PointerToLineNumbers (0 for executables) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 09/10] arm64/efi: enable minimal UEFI Runtime Services for big endian
This enables the UEFI Runtime Services needed to manipulate the non-volatile variable store when running under a BE kernel. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/efi.h | 2 + arch/arm64/kernel/efi-be-call.S| 55 arch/arm64/kernel/efi-be-runtime.c | 104 + arch/arm64/kernel/efi.c| 15 ++ 4 files changed, 176 insertions(+) create mode 100644 arch/arm64/kernel/efi-be-call.S create mode 100644 arch/arm64/kernel/efi-be-runtime.c diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index a34fd3b12e2b..44e642b6ab61 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -44,4 +44,6 @@ extern void efi_idmap_init(void); #define efi_call_early(f, ...) sys_table_arg-boottime-f(__VA_ARGS__) +extern void efi_be_runtime_setup(void); + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi-be-call.S b/arch/arm64/kernel/efi-be-call.S new file mode 100644 index ..24c92a4c352b --- /dev/null +++ b/arch/arm64/kernel/efi-be-call.S @@ -0,0 +1,55 @@ + +#include linux/linkage.h + + .text + .align 3 +ENTRY(efi_be_phys_call) + /* +* Entered at physical address with 1:1 mapping enabled. +*/ + stp x29, x30, [sp, #-96]! + mov x29, sp + str x27, [sp, #16] + + ldr x8, =efi_be_phys_call // virt address of this function + adr x9, efi_be_phys_call// phys address of this function + sub x9, x8, x9 // calculate virt to phys offset in x9 + + /* preserve all inputs */ + stp x0, x1, [sp, #32] + stp x2, x3, [sp, #48] + stp x4, x5, [sp, #64] + stp x6, x7, [sp, #80] + + /* get phys address of stack */ + sub sp, sp, x9 + + /* switch to LE, disable MMU and D-cache but leave I-cache enabled */ + mrs x27, sctlr_el1 + bic x8, x27, #1 2// clear SCTLR.C + msr sctlr_el1, x8 + + bl flush_cache_all + + /* restore inputs but rotated by 1 register */ + ldp x7, x0, [sp, #32] + ldp x1, x2, [sp, #48] + ldp x3, x4, [sp, #64] + ldp x5, x6, [sp, #80] + + bic x8, x27, #1 2// clear SCTLR.C + bic x8, x8, #1 0 // clear SCTLR.M + bic x8, x8, #1 25// clear SCTLR.EE + msr sctlr_el1, x8 + isb + + blr x7 + + /* switch back to BE and reenable MMU and D-cache */ + msr sctlr_el1, x27 + + mov sp, x29 + ldr x27, [sp, #16] + ldp x29, x30, [sp], #96 + ret +ENDPROC(efi_be_phys_call) diff --git a/arch/arm64/kernel/efi-be-runtime.c b/arch/arm64/kernel/efi-be-runtime.c new file mode 100644 index ..62e6c441b77b --- /dev/null +++ b/arch/arm64/kernel/efi-be-runtime.c @@ -0,0 +1,104 @@ + +#include linux/efi.h +#include linux/spinlock.h +#include asm/efi.h +#include asm/neon.h +#include asm/tlbflush.h + +static efi_runtime_services_t *runtime; +static efi_status_t (*efi_be_call)(phys_addr_t func, ...); + +static DEFINE_SPINLOCK(efi_be_rt_lock); + +static unsigned long efi_be_call_pre(void) +{ + unsigned long flags; + + kernel_neon_begin(); + spin_lock_irqsave(efi_be_rt_lock, flags); + cpu_switch_mm(idmap_pg_dir, init_mm); + flush_tlb_all(); + return flags; +} + +static void efi_be_call_post(unsigned long flags) +{ + cpu_switch_mm(current, current-active_mm); + flush_tlb_all(); + spin_unlock_irqrestore(efi_be_rt_lock, flags); + kernel_neon_end(); +} + +static efi_status_t efi_be_get_variable(efi_char16_t *name, + efi_guid_t *vendor, + u32 *attr, + unsigned long *data_size, + void *data) +{ + unsigned long flags; + efi_status_t status; + + *data_size = cpu_to_le64(*data_size); + flags = efi_be_call_pre(); + status = efi_be_call(le64_to_cpu(runtime-get_variable), +virt_to_phys(name), virt_to_phys(vendor), +virt_to_phys(attr), virt_to_phys(data_size), +virt_to_phys(data)); + efi_be_call_post(flags); + *attr = le32_to_cpu(*attr); + *data_size = le64_to_cpu(*data_size); + return status; +} + +static efi_status_t efi_be_get_next_variable(unsigned long *name_size, +efi_char16_t *name, +efi_guid_t *vendor) +{ + unsigned long flags; + efi_status_t status; + + *name_size = cpu_to_le64(*name_size); + flags = efi_be_call_pre(); + status = efi_be_call(le64_to_cpu(runtime-get_next_variable
[RFC PATCH 05/10] arm64/efi: update the PE/COFF header to be endian agnostic
Update the PE/COFF header to use explicit little endian constants and use explicit little endian linker symbols so that the PE/COFF header is always emitted in little endian regardless of the endiannes of the kernel. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/head.S | 48 ++-- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index c63f44f20ae3..5179d3df1024 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -123,7 +123,10 @@ efi_head: .byte 0x4d .byte 0x64 #ifdef CONFIG_EFI - .long pe_header - efi_head// Offset to the PE header. + .byte pe_header - efi_head// Offset to the PE header. + .byte 0 + .byte 0 + .byte 0 #else .word 0 // reserved #endif @@ -136,30 +139,31 @@ pe_header: .ascii PE .short 0 coff_header: - .short 0xaa64 // AArch64 - .short 2 // nr_sections + le160xaa64 // AArch64 + le162 // nr_sections .long 0 // TimeDateStamp .long 0 // PointerToSymbolTable - .long 1 // NumberOfSymbols - .short section_table - optional_header // SizeOfOptionalHeader - .short 0x206 // Characteristics. + le321 // NumberOfSymbols + .byte section_table - optional_header // SizeOfOptionalHeader + .byte 0 + le160x206 // Characteristics. // IMAGE_FILE_DEBUG_STRIPPED | // IMAGE_FILE_EXECUTABLE_IMAGE | // IMAGE_FILE_LINE_NUMS_STRIPPED optional_header: - .short 0x20b // PE32+ format + le160x20b // PE32+ format .byte 0x02// MajorLinkerVersion .byte 0x14// MinorLinkerVersion - .long _end - stext// SizeOfCode + .long _efi_code_virtsize_le // SizeOfCode .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData - .long efi_stub_entry - efi_head // AddressOfEntryPoint - .long stext_offset// BaseOfCode + .long _efi_entry_point_le // AddressOfEntryPoint + .long _efi_stext_offset_le// BaseOfCode extra_header_fields: .quad 0 // ImageBase - .long 0x20// SectionAlignment - .long 0x8 // FileAlignment + le320x20// SectionAlignment + le320x8 // FileAlignment .short 0 // MajorOperatingSystemVersion .short 0 // MinorOperatingSystemVersion .short 0 // MajorImageVersion @@ -168,19 +172,19 @@ extra_header_fields: .short 0 // MinorSubsystemVersion .long 0 // Win32VersionValue - .long _end - efi_head // SizeOfImage + .long _efi_image_size_le // SizeOfImage // Everything before the kernel image is considered part of the header - .long stext_offset// SizeOfHeaders + .long _efi_stext_offset_le// SizeOfHeaders .long 0 // CheckSum - .short 0xa // Subsystem (EFI application) + le160xa // Subsystem (EFI application) .short 0 // DllCharacteristics .quad 0 // SizeOfStackReserve .quad 0 // SizeOfStackCommit .quad 0 // SizeOfHeapReserve .quad 0 // SizeOfHeapCommit .long 0 // LoaderFlags - .long 0x6 // NumberOfRvaAndSizes + le320x6 // NumberOfRvaAndSizes .quad 0 // ExportTable .quad 0 // ImportTable @@ -208,23 +212,23 @@ section_table: .long 0
[RFC PATCH 10/10] arm64: Kconfig: enable UEFI on BE kernels
This changes the Kconfig logic to allow EFI to be enabled on a BE kernel build. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/Kconfig | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index e9d8af2fc389..9fa1383acbd3 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -301,16 +301,20 @@ config CMDLINE_FORCE config EFI_STUB bool +config EFI_LE_STUB + bool + config EFI bool UEFI runtime support - depends on OF !CPU_BIG_ENDIAN + depends on OF select LIBFDT select UCS2_STRING select EFI_PARAMS_FROM_FDT select EFI_RUNTIME_WRAPPERS - select EFI_STUB + select EFI_STUB if !CPU_BIG_ENDIAN + select EFI_LE_STUB if CPU_BIG_ENDIAN select EFI_ARMSTUB - default y + default y if !CPU_BIG_ENDIAN help This option provides support for runtime services provided by UEFI firmware (such as non-volatile variables, realtime -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 07/10] arm64/efi: efistub: add support for booting a BE kernel
This adds support to boot a big endian kernel from UEFI firmware, which is always little endian. The EFI stub itself is built as little endian, and embedded into a big endian kernel image. To enable this, we need to build all the stub's dependencies as little endian, including FDT parsing code and string functions. This is accomplished by building little endian versions of those support files and link them into a static library which is used by the inner stub build. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/Makefile | 7 +++- arch/arm64/kernel/efi-entry.S | 42 +-- arch/arm64/kernel/efistub-le/Makefile | 52 + arch/arm64/kernel/efistub-le/efi-le-entry.S | 13 arch/arm64/kernel/efistub-le/efistub-le.lds | 35 +++ arch/arm64/kernel/efistub-le/le.h | 12 +++ arch/arm64/kernel/efistub-le/strstr.c | 20 +++ drivers/firmware/efi/libstub/fdt.c | 4 +++ 8 files changed, 173 insertions(+), 12 deletions(-) create mode 100644 arch/arm64/kernel/efistub-le/Makefile create mode 100644 arch/arm64/kernel/efistub-le/efi-le-entry.S create mode 100644 arch/arm64/kernel/efistub-le/efistub-le.lds create mode 100644 arch/arm64/kernel/efistub-le/le.h create mode 100644 arch/arm64/kernel/efistub-le/strstr.c diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index afaeb734295a..942cd042e93e 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -27,7 +27,12 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND) += sleep.o suspend.o arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o arm64-obj-$(CONFIG_KGDB) += kgdb.o -arm64-obj-$(CONFIG_EFI)+= efi.o efi-stub.o efi-entry.o + +arm64-efi-obj-y:= efi.o +arm64-efi-obj-$(CONFIG_EFI_STUB) += efi-stub.o efi-entry.o +arm64-efi-obj-$(CONFIG_EFI_LE_STUB)+= efistub-le/ +arm64-efi-obj-$(CONFIG_CPU_BIG_ENDIAN) += efi-be-runtime.o efi-be-call.o +arm64-obj-$(CONFIG_EFI)+= $(arm64-efi-obj-y) obj-y += $(arm64-obj-y) vdso/ obj-m += $(arm64-obj-m) diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index a0016d3a17da..89f34bb86cfd 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -34,8 +34,34 @@ ENTRY(efi_stub_entry) * Create a stack frame to save FP/LR with extra space * for image_addr variable passed to efi_entry(). */ - stp x29, x30, [sp, #-32]! + stp x29, x30, [sp, #-48]! + stp x22, x23, [sp, #32] +#ifdef CONFIG_EFI_LE_STUB + adr x4, efi_stub_entry + ldp w8, w9, [x4, #-32] +STUB_BE(revw8, w8 ) +STUB_BE(revw9, w9 ) + add x8, x4, w8, sxtw// x8: base of Image + add x9, x4, w9, sxtw// x9: offset of linux_banner + + ldp x22, x23, [x4, #-24]// x22: size of Image +STUB_BE(revx23, x23) // x23: stext offset + + /* +* Get a pointer to linux_banner in the outer image and store it +* in this image. +*/ + adrpx4, le_linux_banner + str x9, [x4, #:lo12:le_linux_banner] +#else + adrpx8, _text + add x8, x8, #:lo12:_text// x8: base of Image + adrpx9, _edata + add x9, x9, #:lo12:_edata + sub x22, x9, x8 // x22: size of Image + ldr x23, =stext_offset // x23: stext offset +#endif /* * Call efi_entry to do the real work. * x0 and x1 are already set up by firmware. Current runtime @@ -45,8 +71,6 @@ ENTRY(efi_stub_entry) * efi_system_table_t *sys_table, * unsigned long *image_addr) ; */ - adrpx8, _text - add x8, x8, #:lo12:_text add x2, sp, 16 str x8, [x2] bl efi_entry @@ -61,18 +85,13 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - ldr x21, =stext_offset - add x21, x0, x21 + add x21, x0, x23 /* * Flush dcache covering current runtime addresses * of kernel text/data. Then flush all of icache. */ - adrpx1, _text - add x1, x1, #:lo12:_text - adrpx2, _edata - add x2, x2, #:lo12:_edata - sub x1, x2, x1 + mov x1, x22 bl __flush_dcache_area ic ialluis @@ -103,7 +122,8 @@ ENTRY(efi_stub_entry) efi_load_fail: mov x0, #EFI_LOAD_ERROR - ldp
Re: [PATCH v2] arm64/efi: efistub: jump to 'stext' directly, not through the header
On 17 July 2014 16:09, Mark Salter msal...@redhat.com wrote: On Wed, 2014-07-16 at 23:13 +0200, Ard Biesheuvel wrote: On 16 July 2014 23:03, Mark Salter msal...@redhat.com wrote: On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote: On 16 July 2014 21:45, Mark Salter msal...@redhat.com wrote: On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote: On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote: On Tue, 2014-07-15 at 12:58 +0200, Ard Biesheuvel wrote: After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction. However, the header is not covered by any PE/COFF section, so the header may not actually be loaded at the expected offset. So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section, by supplying a symbol 'stext_offset' to efi-entry.o which contains the relative offset of stext into the Image. Also replace other open coded calculations of the same value with a reference to 'stext_offset' Have you actually seen a situation where the header isn't there? Isn't the kernel header actually part of the pe/coff file and firmware loads the whole file into RAM? From my understanding of Ard's earlier comments, this part isn't guaranteed per the UEFI spec. I would rather we weren't relying on implementation details. Could be. I didn't see anything about it in the UEFI spec, but I probably wasn't exhaustive in my search. In any case, there's at least one other place broken if the kernel header isn't included in the loaded image. I have not been able to find anything in the PE/COFF documents that tells you what to put in memory areas that are not covered by a section. Expecting the header to be there is indeed relying on an implementation detail, which seems risky. And indeed, if there are any other (non EFI related) uses of header fields in the kernel, it would be good to have a look at those well, I think we need to come up with a loader which does load an image without kernel header so that we can test. Otherwise, we'll probably end up with buggy code anyway. The stub code assumes the the loaded image pointed to by the system table is the whole image. Seems like we'd need to add code to determine if it is whole kernel image or image without initial header. Stub would have to handle both cases. For instance, one case would want image placed at 2MiB+TEXT_OFFSET, other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header). No, this has nothing to do with misaligned data. The PE/COFF .text section does not start at virtual offset #0 but at virtual offset 'stext - efi_head'. In other words, there is a hole in the virtual image where the header is supposed to be. So if there is no PE/COFF section describing what data should be put at offset #0 by the loader, we can't assume the header is there, even if ImageBase does start at #0 I get that. You're supposing UEFI will always allocate memory for the full image, but only sometimes copy the PE/COFF headers. I can see your point from a PE/COFF perspective, but not so much from the UEFI spec perspective where the language leads me to think it treats the PE/COFF images as one unit wrt loading. In any case, it really isn't worth arguing about. I don't have any objection to the patch since it won't break anything from my perspective and it'll protect against breakage which could possibly occur with some firmware implementations. OK, thanks. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 09/10] arm64/efi: enable minimal UEFI Runtime Services for big endian
On 23 July 2014 11:34, Mark Rutland mark.rutl...@arm.com wrote: Hi Ard, This is certainly a neat feature, and I definitely want to be able to boot BE kernels via UEFI. Good! However, I'm wary of calling EFI in a physical (i.e. idmap with dcaches off) context. I'm not sure anyone else does that, and I'm not sure whether that's going to work (both because of the cache maintenance requirements and the expectations of a given UEFI implementation w.r.t. memory cacheability). I have developed an alternate version in the mean time that switches to a LE idmap (so with D-cache enabled), but this is an imperfect solution as well, as (like in the MMU off case), the vector base virtual address cannot be resolved when the EE bit is cleared (as TTBR1 points to a BE page table) so any exception taken locks the machine hard. I am not sure if this can be solved in any way other than changing exception levels. Or install an alternate vector table for the duration of the runtime services call that flips the EE bit back, restores VBAR to its original address, and jumps into it. None of this is very sexy, though ... I'd hoped we'd be able to use a LE EL0 context to call the runtime services in, but I'm not sure that's possible by the spec :( Nope, they should be called at the exception level UEFI was started in (as Leif tells me) As I understand it, we shouldn't need these runtime services to simply boot a BE kernel. Well, the significance of the variable store related Runtime Services is that they are used by an installer (through efibootmgr) to program the kernel command line. Hence the choice for just these services in the minimal implementation. -- Ard. On Mon, Jul 21, 2014 at 04:16:24PM +0100, Ard Biesheuvel wrote: This enables the UEFI Runtime Services needed to manipulate the non-volatile variable store when running under a BE kernel. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/efi.h | 2 + arch/arm64/kernel/efi-be-call.S| 55 arch/arm64/kernel/efi-be-runtime.c | 104 + arch/arm64/kernel/efi.c| 15 ++ 4 files changed, 176 insertions(+) create mode 100644 arch/arm64/kernel/efi-be-call.S create mode 100644 arch/arm64/kernel/efi-be-runtime.c diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index a34fd3b12e2b..44e642b6ab61 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -44,4 +44,6 @@ extern void efi_idmap_init(void); #define efi_call_early(f, ...) sys_table_arg-boottime-f(__VA_ARGS__) +extern void efi_be_runtime_setup(void); + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi-be-call.S b/arch/arm64/kernel/efi-be-call.S new file mode 100644 index ..24c92a4c352b --- /dev/null +++ b/arch/arm64/kernel/efi-be-call.S @@ -0,0 +1,55 @@ + +#include linux/linkage.h + + .text + .align 3 +ENTRY(efi_be_phys_call) + /* + * Entered at physical address with 1:1 mapping enabled. + */ + stp x29, x30, [sp, #-96]! + mov x29, sp + str x27, [sp, #16] + + ldr x8, =efi_be_phys_call // virt address of this function + adr x9, efi_be_phys_call// phys address of this function + sub x9, x8, x9 // calculate virt to phys offset in x9 + + /* preserve all inputs */ + stp x0, x1, [sp, #32] + stp x2, x3, [sp, #48] + stp x4, x5, [sp, #64] + stp x6, x7, [sp, #80] + + /* get phys address of stack */ + sub sp, sp, x9 + + /* switch to LE, disable MMU and D-cache but leave I-cache enabled */ + mrs x27, sctlr_el1 + bic x8, x27, #1 2// clear SCTLR.C + msr sctlr_el1, x8 + + bl flush_cache_all What is the cache flush for? The only thing that flush_cache_all can do is empty the local architected caches, and it can only do that when said caches are disabled. Any other use is unsafe; we have no guarantee that the cache is empty (or even clean), and we have no guarantee that prior writes have made it to the PoC. Even when the caches are disabled, flush_cache_all can only guarantee that the local architected caches are empty. There is no guarantee that the dirty data made it to the PoC. + + /* restore inputs but rotated by 1 register */ + ldp x7, x0, [sp, #32] + ldp x1, x2, [sp, #48] + ldp x3, x4, [sp, #64] + ldp x5, x6, [sp, #80] + + bic x8, x27, #1 2// clear SCTLR.C + bic x8, x8, #1 0 // clear SCTLR.M + bic x8, x8, #1 25// clear SCTLR.EE + msr sctlr_el1, x8 + isb + + blr x7 Is it safe to call EFI functions with the D-cache disabled? Do the functions not care about the memory attributes for their own sue (e.g. for exclusives)? Do they not care
Re: [RFC PATCH 09/10] arm64/efi: enable minimal UEFI Runtime Services for big endian
On 07/23/2014 12:59 PM, Ard Biesheuvel wrote: On 23 July 2014 11:34, Mark Rutland mark.rutl...@arm.com wrote: Hi Ard, This is certainly a neat feature, and I definitely want to be able to boot BE kernels via UEFI. Good! However, I'm wary of calling EFI in a physical (i.e. idmap with dcaches off) context. I'm not sure anyone else does that, and I'm not sure whether that's going to work (both because of the cache maintenance requirements and the expectations of a given UEFI implementation w.r.t. memory cacheability). I have developed an alternate version in the mean time that switches to a LE idmap (so with D-cache enabled), but this is an imperfect solution as well, as (like in the MMU off case), the vector base virtual address cannot be resolved when the EE bit is cleared (as TTBR1 points to a BE page table) so any exception taken locks the machine hard. I am not sure if this can be solved in any way other than changing exception levels. Or install an alternate vector table for the duration of the runtime services call that flips the EE bit back, restores VBAR to its original address, and jumps into it. None of this is very sexy, though ... I'd hoped we'd be able to use a LE EL0 context to call the runtime services in, but I'm not sure that's possible by the spec :( Nope, they should be called at the exception level UEFI was started in (as Leif tells me) As I understand it, we shouldn't need these runtime services to simply boot a BE kernel. Well, the significance of the variable store related Runtime Services is that they are used by an installer (through efibootmgr) to program the kernel command line. Hence the choice for just these services in the minimal implementation. The below patch is an alternate approach with a LE id mapping in efi_pg_dir. (Patch that sets it up omitted). This dodges all the concerns related to caching, hopefully, as the LE id mapping and the BE id mapping in idmap_pg_dir should agree on the memory attributes of all common mappings. This also addresses the FIQ and exception concerns, although I fully realise that this is likely too controversial. Suggestions for less controversial approaches are highly appreciated. As said, booting a BE kernel is useful by itself, but without being able to use efibootmgr it is a bit crippled. -- Ard. diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index a34fd3b12e2b..2eeae5ae55b2 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -44,4 +44,6 @@ extern void efi_idmap_init(void); #define efi_call_early(f, ...) sys_table_arg-boottime-f(__VA_ARGS__) +extern int efi_be_runtime_setup(void); + #endif /* _ASM_EFI_H */ diff --git a/arch/arm64/kernel/efi-be-call.S b/arch/arm64/kernel/efi-be-call.S new file mode 100644 index ..8da53a225fab --- /dev/null +++ b/arch/arm64/kernel/efi-be-call.S @@ -0,0 +1,129 @@ + +#include linux/linkage.h + + .macro flush_tlb_all + dsb ishst + tlbivmalle1is + dsb ish + isb + .endm + + .text + /* +* Alternate vector table so we can trap exceptions while in LE mode +* and make the world sane again before letting the kernel handle the +* exception as usual. Clobbers x30. +*/ + .align 12 +.Lvectors: + .irpc i, 0123456789abcdef + .align 7 + /* switch back to BE and temporarily disable MMU */ + mrs x30, sctlr_el1 + bic x30, x30, #1 0 // clear SCTLR.M + orr x30, x30, #1 25 // set SCTLR.EE + msr sctlr_el1, x30 + isb + + /* needed as TLBs are permitted to cache the EE bit */ + flush_tlb_all + + /* re-install BE idmap */ + adrpx30, idmap_pg_dir + msr ttbr0_el1, x30 + mrs x30, sctlr_el1 + orr x30, x30, #1 0 // set SCTLR.M + msr sctlr_el1, x30 // re-enable MMU + isb + + /* +* Use the virtual and physical addresses of 'vectors' to restore the +* virtual offset of sp. +*/ + adrpx30, vectors + add x30, x30, #:lo12:vectors + sub sp, sp, x30 + ldr x30, =vectors + add sp, sp, x30 + + /* reinstall vector table */ + msr vbar_el1, x30 // restore VBAR to 'vectors' + isb + + add x30, x30, #(0x\i * 0x80) // jump to real vector + ret + .endr + +ENTRY(efi_be_phys_call) + /* +* Entered at physical address with 1:1 mapping enabled and interrupts +* disabled. +*/ + stp x29, x30, [sp, #-48]! + mov x29, sp + stp x25, x26, [sp, #16] + stp x27, x28, [sp, #32] + + ldr x8, =efi_be_phys_call // virt address of this function + adr x9, efi_be_phys_call// phys address of this function + sub x9, x8, x9 // calculate virt
Re: [PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied
On 29 July 2014 17:29, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote: If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) Tested on Mustang (with loss of 2MB free memory). Great, thanks! diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 60e98a639ac5..460c00c41e57 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, kernel_size = _edata - _text; if (*image_addr != (dram_base + TEXT_OFFSET)) { kernel_memsize = kernel_size + (_end - _edata); - status = efi_relocate_kernel(sys_table, image_addr, - kernel_size, kernel_memsize, - dram_base + TEXT_OFFSET, - PAGE_SIZE); Can we make efi_relocate_kernel static inline to get rid of the defined but unused warning? I have some patches pending in the EFI tree to turn the stub into a static library, and that already takes care of this issue. Otherwise: Acked-by: Mark Salter msal...@redhat.com Cheers, Ard. + status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET, +SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); return status; } - if (*image_addr != (dram_base + TEXT_OFFSET)) { - pr_efi_err(sys_table, Failed to alloc kernel memory\n); - efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; - } - *image_size = kernel_memsize; + memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, +kernel_size); + *image_addr = *reserve_addr + TEXT_OFFSET; + *reserve_size = kernel_memsize + TEXT_OFFSET; } -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied
On 29 July 2014 20:27, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-29 at 20:17 +0200, Ard Biesheuvel wrote: On 29 July 2014 17:29, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote: If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) Tested on Mustang (with loss of 2MB free memory). Great, thanks! diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 60e98a639ac5..460c00c41e57 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, kernel_size = _edata - _text; if (*image_addr != (dram_base + TEXT_OFFSET)) { kernel_memsize = kernel_size + (_end - _edata); - status = efi_relocate_kernel(sys_table, image_addr, - kernel_size, kernel_memsize, - dram_base + TEXT_OFFSET, - PAGE_SIZE); Can we make efi_relocate_kernel static inline to get rid of the defined but unused warning? I have some patches pending in the EFI tree to turn the stub into a static library, and that already takes care of this issue. That's fine if the static library stub patch goes in first. If this patch goes in first, then let's avoid the warning since it is easy to do. My idea was to ask Matt to take patches #2 and #3. I may have to fix them up slightly to apply correctly, but that's fine. Changing efi_relocate_kernel to static inline would need to go through Matt's tree as well, so there's probably no point in doing that in any case. Patch #1 needs to go through the arm64, I guess. This means UEFI boot on APM Mustang will be broken during the short time between the x86/tip tree and the arm64 tree being merged for 3.17 (assuming 3.17 is still open). I think we should be able to tolerate that, right? Otherwise: Acked-by: Mark Salter msal...@redhat.com Cheers, Ard. + status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET, +SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); return status; } - if (*image_addr != (dram_base + TEXT_OFFSET)) { - pr_efi_err(sys_table, Failed to alloc kernel memory\n); - efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; - } - *image_size = kernel_memsize; + memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, +kernel_size); + *image_addr = *reserve_addr + TEXT_OFFSET; + *reserve_size = kernel_memsize + TEXT_OFFSET; } -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied
On 29 July 2014 21:20, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-29 at 20:46 +0200, Ard Biesheuvel wrote: On 29 July 2014 20:27, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-29 at 20:17 +0200, Ard Biesheuvel wrote: On 29 July 2014 17:29, Mark Salter msal...@redhat.com wrote: On Tue, 2014-07-29 at 12:49 +0200, Ard Biesheuvel wrote: If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/efi-stub.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) Tested on Mustang (with loss of 2MB free memory). Great, thanks! diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 60e98a639ac5..460c00c41e57 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -60,20 +60,16 @@ static efi_status_t handle_kernel_image(efi_system_table_t *sys_table, kernel_size = _edata - _text; if (*image_addr != (dram_base + TEXT_OFFSET)) { kernel_memsize = kernel_size + (_end - _edata); - status = efi_relocate_kernel(sys_table, image_addr, - kernel_size, kernel_memsize, - dram_base + TEXT_OFFSET, - PAGE_SIZE); Can we make efi_relocate_kernel static inline to get rid of the defined but unused warning? I have some patches pending in the EFI tree to turn the stub into a static library, and that already takes care of this issue. That's fine if the static library stub patch goes in first. If this patch goes in first, then let's avoid the warning since it is easy to do. My idea was to ask Matt to take patches #2 and #3. I may have to fix them up slightly to apply correctly, but that's fine. Changing efi_relocate_kernel to static inline would need to go through Matt's tree as well, so there's probably no point in doing that in any case. I'm not following. I would say there is no point in not doing that. You have a patch which causes a build warning. Let's avoid that unless there's a good reason not too. In this case, it seems easy enough to avoid unless I'm missing something. Agreed. The static library patches are already queued up in x86/tip, so if we add a patch that changes efi_relocate_kernel() in drivers/firmware/efi, it either goes through efi and comes after the static lib patches, which makes it redundant, or it goes through arm64 and causes a conflict. Patch #1 needs to go through the arm64, I guess. This means UEFI boot on APM Mustang will be broken during the short time between the x86/tip tree and the arm64 tree being merged for 3.17 (assuming 3.17 is still open). I think we should be able to tolerate that, right? Breaking bisect would be really bad. I think all three should be pulled from the same tree. What's wrong with getting an ack from Catalin or Will on the first patch and having all three go through tip? Patch one is needed for EFI boot functionality even if it isn't specifically EFI code. This won't be the last time this sort of situation arises. Ah, right, I forgot about bisect. So yes, patch #1 should definitely go in before #2 and #3, which is indeed easiest if they get merged through the same tree. So yes, taking all of these through the EFI tree is indeed the way to go ... -- Ard. Otherwise: Acked-by: Mark Salter msal...@redhat.com Cheers, Ard. + status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET, +SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); return status; } - if (*image_addr != (dram_base + TEXT_OFFSET)) { - pr_efi_err(sys_table, Failed to alloc kernel memory\n); - efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_ERROR; - } - *image_size = kernel_memsize; + memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, +kernel_size); + *image_addr = *reserve_addr + TEXT_OFFSET; + *reserve_size = kernel_memsize + TEXT_OFFSET; } -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text
The static memory footprint of a kernel Image at boot is larger than the Image file itself. Things like .bss data and initial page tables are allocated statically but populated dynamically so their content is not contained in the Image file. However, if EFI (or GRUB) has loaded the Image at precisely the desired offset of base of DRAM + TEXT_OFFSET, the Image will be booted in place, and we have to make sure that the allocation done by the PE/COFF loader is large enough. Fix this by growing the PE/COFF .text section to cover the entire static memory footprint. The part of the section that is not covered by the payload will be zero initialised by the PE/COFF loader. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Acked-by: Mark Salter msal...@redhat.com --- arch/arm64/kernel/head.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 144f10567f82..b6ca95aee348 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -151,7 +151,7 @@ optional_header: .short 0x20b // PE32+ format .byte 0x02// MajorLinkerVersion .byte 0x14// MinorLinkerVersion - .long _edata - stext // SizeOfCode + .long _end - stext// SizeOfCode .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint @@ -169,7 +169,7 @@ extra_header_fields: .short 0 // MinorSubsystemVersion .long 0 // Win32VersionValue - .long _edata - efi_head // SizeOfImage + .long _end - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header .long stext - efi_head// SizeOfHeaders @@ -216,7 +216,7 @@ section_table: .byte 0 .byte 0 .byte 0 // end of 0 padding of section name - .long _edata - stext // VirtualSize + .long _end - stext// VirtualSize .long stext - efi_head// VirtualAddress .long _edata - stext // SizeOfRawData .long stext - efi_head// PointerToRawData -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] arm64/efi: efistub: don't abort if base of DRAM is occupied
If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org Acked-by: Mark Salter msal...@redhat.com --- arch/arm64/kernel/efi-stub.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c index 1317fef8dde9..d27dd982ff26 100644 --- a/arch/arm64/kernel/efi-stub.c +++ b/arch/arm64/kernel/efi-stub.c @@ -28,20 +28,16 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table, kernel_size = _edata - _text; if (*image_addr != (dram_base + TEXT_OFFSET)) { kernel_memsize = kernel_size + (_end - _edata); - status = efi_relocate_kernel(sys_table, image_addr, -kernel_size, kernel_memsize, -dram_base + TEXT_OFFSET, -PAGE_SIZE); + status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET, + SZ_2M, reserve_addr); if (status != EFI_SUCCESS) { pr_efi_err(sys_table, Failed to relocate kernel\n); return status; } - if (*image_addr != (dram_base + TEXT_OFFSET)) { - pr_efi_err(sys_table, Failed to alloc kernel memory\n); - efi_free(sys_table, kernel_memsize, *image_addr); - return EFI_LOAD_ERROR; - } - *image_size = kernel_memsize; + memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr, + kernel_size); + *image_addr = *reserve_addr + TEXT_OFFSET; + *reserve_size = kernel_memsize + TEXT_OFFSET; } -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c index 0347d38eea29..4f93c67e63de 100644 --- a/arch/arm64/kernel/smp_spin_table.c +++ b/arch/arm64/kernel/smp_spin_table.c @@ -20,6 +20,7 @@ #include linux/init.h #include linux/of.h #include linux/smp.h +#include linux/types.h #include asm/cacheflush.h #include asm/cpu_ops.h @@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu) static int smp_spin_table_cpu_prepare(unsigned int cpu) { - void **release_addr; + __le64 __iomem *release_addr; if (!cpu_release_addr[cpu]) return -ENODEV; - release_addr = __va(cpu_release_addr[cpu]); + /* +* The cpu-release-addr may or may not be inside the linear mapping. +* As ioremap_cache will either give us a new mapping or reuse the +* existing linear mapping, we can use it to cover both cases. In +* either case the memory will be MT_NORMAL. +*/ + release_addr = ioremap_cache(cpu_release_addr[cpu], +sizeof(*release_addr)); + if (!release_addr) + return -ENOMEM; /* * We write the release address as LE regardless of the native @@ -79,15 +89,17 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu) * boot-loader's endianess before jumping. This is mandated by * the boot protocol. */ - release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen)); - - __flush_dcache_area(release_addr, sizeof(release_addr[0])); + writeq_relaxed(__pa(secondary_holding_pen), release_addr); + __flush_dcache_area((__force void *)release_addr, + sizeof(*release_addr)); /* * Send an event to wake up the secondary CPU. */ sev(); + iounmap(release_addr); + return 0; } -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3 v2] arm64/efi: improve TEXT_OFFSET handling
Resending this series sent out yesterday with only minor changes and acks etc added. In summary: patch #3 relaxes the requirements imposed by the EFI stub on where Image may be loaded, but this breaks APM Mustang (if booting via UEFI) if patch #1 does not go in first. Patch #2 prevents potential boot issues when Image is loaded such that the stub does not have to relocate it. @Will: as discussed on the list yesterday, these patches should be kept in sequence when going upstream, so it is best to take them through a single tree. However, patch #3 will not apply cleanly to the arm64 tree until after 3.16-rc1 is released as it depends on a trivial change going in through x86/tip [efi]. [https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=nextid=6091c9c447370c4717ec9975813c874af490eb36] If you are ok with these patches, how would you like to proceed? Patches #1 and #2 could go in straight away (through arm64), and I can send Catalin and you a gentle reminder once -rc1 is released to take #3? Or instead, ack them and ask Matt to queue them for 3.17-late? It would be nice if this makes 3.17 as it fixes actual boot problems on hardware that is under development. Changes in v2: - add (__force void *) cast to patch #1, as suggested in LAKML discussion - add tested-by/acked-by lines - rebased patch #3 onto efi-next Mark Rutland (1): arm64: spin-table: handle unmapped cpu-release-addrs Ard Biesheuvel (2): arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text arm64/efi: efistub: don't abort if base of DRAM is occupied arch/arm64/kernel/efi-stub.c | 18 ++ arch/arm64/kernel/head.S | 6 +++--- arch/arm64/kernel/smp_spin_table.c | 21 - 3 files changed, 25 insertions(+), 20 deletions(-) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 14:00, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain Clarification: every Runtime Services region known to it as being normal DRAM, which may cover this area ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 14:49, Mark Rutland mark.rutl...@arm.com wrote: On Wed, Jul 30, 2014 at 01:42:58PM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote: On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. Even then, I think ioremap hits a WARN_ON if pfn_valid. Ok, that's that idea dead then. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Of we just skip the cache flush if !pfn_valid. I don't think that's always safe given Ard's comment that the EFI code will possibly have a mapping covering the region created by ioremap_cache. Ard, what exactly does the EFI code map with ioremap_cache, and why? Actually, after re-reading the spec and the code, perhaps this is not an issue. The EFI __init code calls ioremap_cache() for all regions described by the UEFI memory map as requiring a virtual mapping (EFI_MEMORY_RUNTIME): this is primarily runtime services code and data regions and perhaps some I/O mappings for flash or other peripherals that UEFI owns and needs to access during Runtime Services calls. Mark Salter mentioned that APM Mustang's spin table lives in an EFI_RESERVED_TYPE region, which presumably would not have the EFI_MEMORY_RUNTIME attribute set, as it has nothing to do with the UEFI Runtime Services. This means that no cached mapping should already exist for that region. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 15:10, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 30 July 2014 14:49, Mark Rutland mark.rutl...@arm.com wrote: On Wed, Jul 30, 2014 at 01:42:58PM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote: On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. Even then, I think ioremap hits a WARN_ON if pfn_valid. Ok, that's that idea dead then. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Of we just skip the cache flush if !pfn_valid. I don't think that's always safe given Ard's comment that the EFI code will possibly have a mapping covering the region created by ioremap_cache. Ard, what exactly does the EFI code map with ioremap_cache, and why? Actually, after re-reading the spec and the code, perhaps this is not an issue. The EFI __init code calls ioremap_cache() for all regions described by the UEFI memory map as requiring a virtual mapping (EFI_MEMORY_RUNTIME): this is primarily runtime services code and data regions and perhaps some I/O mappings for flash or other peripherals that UEFI owns and needs to access during Runtime Services calls. Mark Salter mentioned that APM Mustang's spin table lives in an EFI_RESERVED_TYPE region, which presumably would not have the EFI_MEMORY_RUNTIME attribute set, as it has nothing to do with the UEFI Runtime Services. This means that no cached mapping should already exist for that region. That said, there is another potential snag: the UEFI spec for AArch64 does not allow regions residing in the same 64k phys frame to have different memory attributes, and in order to meet this requirement, an EFI_RESERVED_TYPE region could supposedly still be described with a EFI_MEMORY_WB (cacheable) attribute set (e.g. if it shares the 64k phys frame with Runtime Services Code or Data) -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 04/10] arm64: add EFI little endian constants to linker script
On 30 July 2014 16:18, Matt Fleming m...@console-pimps.org wrote: On Mon, 21 Jul, at 05:16:19PM, Ard Biesheuvel wrote: Similar to how text offset and kernel size are mangled to produce little endian constants for the Image header regardless of the endianness of the kernel, this adds a number of constants used in the EFI PE/COFF header which can only be calculated (and byte swapped) by the linker. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/image.h | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) Where is this file? I can't find it in Linus' tree. Apologies, I failed to mention that this series depends on arm64 for-next/core https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/log/?h=for-next/core -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 31 July 2014 12:04, Will Deacon will.dea...@arm.com wrote: On Thu, Jul 31, 2014 at 10:58:54AM +0100, Mark Rutland wrote: On Thu, Jul 31, 2014 at 10:45:15AM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). No. The DT is describing the memory which is present, and the subset thereof which should not be used under normal circumstances. That's a static property of the system. Where the OS happens to get loaded and what it is able to address is a dynamic property of the OS (and possibly the bootloader). The DT cannot have knowledge of this. It's always true that the OS should not blindly use memreserve'd memory. The fact that it cannot address it in the linear mapping is orthogonal. In which case, I think asserting that /memreserve/ implies DRAM is pretty fragile and not actually enforced anywhere. Sure, we can say `don't do that', but I'd prefer to have the kernel detect this dynamically. The point is whether we can assume that cpu-release-addr always resides in DRAM, not whether /memreserve/ implies DRAM. The former should be the case for all current implementations, because we only ever access it through the linear mapping. This means that rather than worrying about all the corner cases where cpu-release-addr may share its 64k physical frame with device registers etc, couldn't we just update booting.txt to state that cpu-release-addr should be chosen such that it can be mapped with a 64k granule MT_NORMAL mapping? -- Ard. Does dtc check that the /memreserve/ region is actually a subset of the memory node? Will -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] arm64/efi: use id mapping for Runtime Services
There are 2 interesting pieces of information in the UEFI spec section 2.3.6 regarding the mapping of runtime regions: (a) the firmware should not request a virtual mapping for configuration tables, even though they are marked as EfiRuntimeServicesData; (b) calling SetVirtualAddressMap() is optional, and it is equally appropriate to call Runtime Services using an identity mapping. So we can eliminate some of the complexity around UEFI Runtime Services by not using a virtual mapping at all, and calling the services at their physical address. This is especially useful under kexec, as SetVirtualAddressMap() may only be called once, and there is no guarantee that mappings are stable between different kexec'd kernels. The fallout for other in-kernel users of UEFI data structures should be negligible, as they cannot legally access those data structures through pre-existing virtual mappings anyway (point (a) above) It should also be noted that, as the kernel side of the address space (TTBR1) is retained, the stack and pointer function arguments remain accessible to the runtime service while the id mapping is active. Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/include/asm/efi.h | 24 -- arch/arm64/kernel/efi.c | 106 ++- 2 files changed, 23 insertions(+), 107 deletions(-) diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index a34fd3b12e2b..d42a21e79b39 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -1,8 +1,10 @@ #ifndef _ASM_EFI_H #define _ASM_EFI_H +#include asm/cacheflush.h #include asm/io.h #include asm/neon.h +#include asm/tlbflush.h #ifdef CONFIG_EFI extern void efi_init(void); @@ -12,23 +14,37 @@ extern void efi_idmap_init(void); #define efi_idmap_init() #endif +static inline void switch_pgd(pgd_t *pgd, struct mm_struct *mm) +{ + cpu_switch_mm(pgd, mm); + flush_tlb_all(); + if (icache_is_aivivt()) + __flush_icache_all(); +} + #define efi_call_virt(f, ...) \ ({ \ - efi_##f##_t *__f = efi.systab-runtime-f; \ + efi_##f##_t *__f; \ efi_status_t __s; \ \ - kernel_neon_begin();\ + kernel_neon_begin(); /* disables preemption */ \ + switch_pgd(idmap_pg_dir, init_mm); \ + __f = efi.systab-runtime-f; \ __s = __f(__VA_ARGS__); \ + switch_pgd(current-active_mm-pgd, current-active_mm);\ kernel_neon_end(); \ __s;\ }) #define __efi_call_virt(f, ...) \ ({ \ - efi_##f##_t *__f = efi.systab-runtime-f; \ + efi_##f##_t *__f; \ \ - kernel_neon_begin();\ + kernel_neon_begin(); /* disables preemption */ \ + switch_pgd(idmap_pg_dir, init_mm); \ + __f = efi.systab-runtime-f; \ __f(__VA_ARGS__); \ + switch_pgd(current-active_mm-pgd, current-active_mm);\ kernel_neon_end(); \ }) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index e72f3100958f..d620a031e7bf 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -27,8 +27,6 @@ struct efi_memory_map memmap; -static efi_runtime_services_t *runtime; - static u64 efi_system_table; static int uefi_debug __initdata; @@ -340,51 +338,9 @@ void __init efi_idmap_init(void) efi_setup_idmap(); } -static int __init remap_region(efi_memory_desc_t *md, void **new) -{ - u64 paddr, vaddr, npages, size; - - paddr = md-phys_addr; - npages = md-num_pages; - memrange_efi_to_native(paddr, npages); - size = npages PAGE_SHIFT; - - if (is_normal_ram(md)) - vaddr = (__force u64)ioremap_cache(paddr, size); - else - vaddr = (__force u64)ioremap(paddr, size); - - if (!vaddr) { - pr_err(Unable to remap 0x%llx pages @ %p\n, - npages, (void *)paddr); - return 0