Skip to contentSkip to conversation

efi: more fixes and general cleanups for v5.6

PATCH New Ard Biesheuvel 1 participant 15 messages Dec 30, 2019

Subsystems (3)
x86 EFI x86 platform uv hpe superdome flex
lore
Share & tools
lore

Ard Biesheuvel

author maintainer
#0 lore raw

v2 cover efi: more fixes and general cleanups for v5.6

This is a combined v2 followup to the v1s of

efi: more fixes and general cleanups for v5.6 [0] efi/x86: clean up and simplify runtime call wrappers [1]

sent out last week. I have some other changes pending regarding the mapping of EFI runtime memory with restricted permissions [2], but I will get back to those after finishing up with the changes below.

Changes since v1:
- Fix logic in #3 for running 64-bit kernels on 32-bit firmware when mixed mode support is not compiled in. In this case, we can still boot but we will not have access to runtime services.
- Refactor the C wrappers involved in calling SetVirtualAddressMap, which were kludgy and hard to follow (#6, #7)
- Incorporate review feedback from Arvind and Andy regarding the asm wrappers (#8, #9, #10), and simplify them a bit further,
- Drop efi_runtime_init32/64 entirely since they turned out to be unnecessary. (#11)
- Add another pair of cleanup patches for the runtime services code (#13, #14)

Branch can be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-more-cleanup-for-v5.6-v2

[0] https://lore.kernel.org/linux-efi/[email protected]/
[1] https://lore.kernel.org/linux-efi/[email protected]/
[2] https://lore.kernel.org/linux-efi/[email protected]/

Cc Ingo Molnar <[email protected]>
Cc Arvind Sankar <[email protected]>
Cc Hans de Goede <[email protected]>
Cc Andy Lutomirski <[email protected]>

Ard Biesheuvel (14):
efi/libstub: fix boot argument handling in mixed mode entry code efi/libstub: use correct system table pointer in mixed mode efi_free() efi/x86: re-disable RT services for 32-bit kernels running on 64-bit
EFI
efi/x86: map the entire EFI vendor string before copying it efi/x86: avoid redundant cast of EFI firmware service pointer efi/x86: split off some old memmap handling into separate routines efi/x86: split SetVirtualAddresMap() wrappers into 32 and 64 bit
versions
efi/x86: simplify i386 efi_call_phys() firmware call wrapper efi/x86: simplify 64-bit EFI firmware call wrapper efi/x86: simplify mixed mode call wrapper
efi/x86: drop two near identical versions of efi_runtime_init() efi/x86: clean up efi_systab_init() routine for legibility efi/x86: don't panic or BUG() on non-critical error conditions efi/x86: remove unreachable code in kexec_enter_virtual_mode()

 arch/x86/boot/compressed/eboot.c     |   3 +-
 arch/x86/boot/compressed/head_64.S   |  17 +-
 arch/x86/include/asm/efi.h           |  43 +--
 arch/x86/platform/efi/Makefile       |   1 -
 arch/x86/platform/efi/efi.c          | 354 +++++++-------------
 arch/x86/platform/efi/efi_32.c       |  22 +-
 arch/x86/platform/efi/efi_64.c       | 129 ++++---
 arch/x86/platform/efi/efi_stub_32.S  | 109 +-----
 arch/x86/platform/efi/efi_stub_64.S  |  39 +--
 arch/x86/platform/efi/efi_thunk_64.S | 121 ++-----
 arch/x86/platform/uv/bios_uv.c       |   7 +-
 include/linux/efi.h                  |  23 +-
 12 files changed, 288 insertions(+), 580 deletions(-)
Signature · 1 line
2.20.1
author maintainer
#1 lore raw

PATCH v2 1/14 efi/libstub: fix boot argument handling in mixed mode entry code

The mixed mode refactor actually broke mixed mode by failing to pass the bootparam structure to startup_32(). This went unnoticed because it apparently has a high tolerance for being passed random junk, and still boots fine in some cases. So let's fix this by populating %esi as required when entering via efi32_stub_entry, and while at it, preserve the arguments themselves instead of their address in memory (via the stack pointer) since that memory could be clobbered before we get to it.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/head_64.S | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
arch/x86/boot/compressed/head_64.S +10 -7
index a6f3ee9ca61d..44a6bb6964b5 100644
@@ -208,13 +208,12 @@ SYM_FUNC_START(startup_32)
 	pushl	$__KERNEL_CS
 	leal	startup_64(%ebp), %eax
 #ifdef CONFIG_EFI_MIXED
-	movl	efi32_boot_args(%ebp), %ebx
-	cmp	$0, %ebx
+	movl	efi32_boot_args(%ebp), %edi
+	cmp	$0, %edi
 	jz	1f
 	leal	handover_entry(%ebp), %eax
-	movl	0(%ebx), %edi
-	movl	4(%ebx), %esi
-	movl	8(%ebx), %edx
+	movl	%esi, %edx
+	movl	efi32_boot_args+4(%ebp), %esi
 	movl	$0x0, %ecx
 1:
 #endif

@@ -232,12 +231,16 @@ SYM_FUNC_END(startup_32)
 	.org 0x190
 SYM_FUNC_START(efi32_stub_entry)
 	add	$0x4, %esp		/* Discard return address */
+	popl	%ecx
+	popl	%edx
+	popl	%esi
 
 	call	1f
 1:	pop	%ebp
 	subl	$1b, %ebp
 
-	movl	%esp, efi32_boot_args(%ebp)
+	movl	%ecx, efi32_boot_args(%ebp)
+	movl	%edx, efi32_boot_args+4(%ebp)
 	sgdtl	efi32_boot_gdt(%ebp)
 
 	/* Disable paging */

@@ -628,7 +631,7 @@ SYM_DATA_START_LOCAL(gdt)
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
 #ifdef CONFIG_EFI_MIXED
-SYM_DATA_LOCAL(efi32_boot_args, .long 0)
+SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0)
 #endif
 
 /*
Signature · 1 line
2.20.1
author maintainer
#2 lore raw

PATCH v2 2/14 efi/libstub: use correct system table pointer in mixed mode efi_free()

In reply to Ard Biesheuvel

There is a special implementation for mixed mode of the efi_free() function, to work around the incompatibility of the mixed mode plumbing with the prototype of the EFI FreePages boot service, which takes a 64-bit physical address as its first argument.

Calling FreePages in mixed mode involves passing the mixed mode address of the FreePages code from the mixed mode version of the EFI system table, and the current code dereferences the ordinary system table instead, producing the wrong results. So fix this by using the efi_table_attr() macro.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/eboot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/eboot.c +2 -1
index da04948d75ed..98477f3529f6 100644
@@ -901,7 +901,8 @@ void efi_free(unsigned long size, unsigned long addr)
 	if (efi_is_native())
 		efi_free_native(size, addr);
 	else
-		efi64_thunk(efi_system_table()->boottime->mixed_mode.free_pages,
+		efi64_thunk(efi_table_attr(efi_system_table(),
+					   boottime)->mixed_mode.free_pages,
 			    addr, 0, DIV_ROUND_UP(size, EFI_PAGE_SIZE));
 }
 #endif
Signature · 1 line
2.20.1
author maintainer
#3 lore raw

PATCH v2 3/14 efi/x86: re-disable RT services for 32-bit kernels running on 64-bit EFI

In reply to Ard Biesheuvel

Commit a8147dba75b1 ("efi/x86: Rename efi_is_native() to efi_is_mixed()") renamed and refactored efi_is_native() into efi_is_mixed(), but failed to take into account that these are not diametrical opposites.

Mixed mode is a construct that permits 64-bit kernels to boot on 32-bit firmware, but there is another non-native combination which is supported, i.e., 32-bit kernels booting on 64-bit firmware, but only for boot and not for runtime services. Also, mixed mode can be disabled in Kconfig, in which case the 64-bit kernel can still be booted from 32-bit firmware, but without access to runtime services.

Due to this oversight, efi_runtime_supported() now incorrectly returns true for such configurations, resulting in crashes at boot. So fix this by making efi_runtime_supported() aware of this.

As a side effect, some efi_thunk_xxx() stubs have become obsolete, so remove them as well.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/include/asm/efi.h | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)
arch/x86/include/asm/efi.h +2 -16
index 2d1378f19b74..b35b5d423e9d 100644
@@ -163,10 +163,10 @@ static inline bool efi_is_mixed(void)
 
 static inline bool efi_runtime_supported(void)
 {
-	if (!efi_is_mixed())
+	if (IS_ENABLED(CONFIG_X86_64) == efi_enabled(EFI_64BIT))
 		return true;
 
-	if (!efi_enabled(EFI_OLD_MEMMAP))
+	if (IS_ENABLED(CONFIG_EFI_MIXED) && !efi_enabled(EFI_OLD_MEMMAP))
 		return true;
 
 	return false;

@@ -176,7 +176,6 @@ extern void parse_efi_setup(u64 phys_addr, u32 data_len);
 
 extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
 
-#ifdef CONFIG_EFI_MIXED
 extern void efi_thunk_runtime_setup(void);
 extern efi_status_t efi_thunk_set_virtual_address_map(
 	void *phys_set_virtual_address_map,

@@ -184,19 +183,6 @@ extern efi_status_t efi_thunk_set_virtual_address_map(
 	unsigned long descriptor_size,
 	u32 descriptor_version,
 	efi_memory_desc_t *virtual_map);
-#else
-static inline void efi_thunk_runtime_setup(void) {}
-static inline efi_status_t efi_thunk_set_virtual_address_map(
-	void *phys_set_virtual_address_map,
-	unsigned long memory_map_size,
-	unsigned long descriptor_size,
-	u32 descriptor_version,
-	efi_memory_desc_t *virtual_map)
-{
-	return EFI_SUCCESS;
-}
-#endif /* CONFIG_EFI_MIXED */
-
 
 /* arch specific definitions used by the stub code */
Signature · 1 line
2.20.1
author maintainer
#4 lore raw

PATCH v2 4/14 efi/x86: map the entire EFI vendor string before copying it

In reply to Ard Biesheuvel

Fix a couple of issues with the way we map and copy the vendor string:
- we map only 2 bytes, which usually works since you get at least a page, but if the vendor string happens to cross a page boundary, a crash will result
- only call early_memunmap() if early_memremap() succeeded, or we will call it with a NULL address which it doesn't like,
- while at it, switch to early_memremap_ro(), and array indexing rather than pointer dereferencing to read the CHAR16 characters.

Fixes 5b83683f32b1 ("x86: EFI runtime service support")
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
arch/x86/platform/efi/efi.c +7 -6
index d96953d9d4e7..3ce32c31bb61 100644
@@ -541,7 +541,6 @@ void __init efi_init(void)
 	efi_char16_t *c16;
 	char vendor[100] = "unknown";
 	int i = 0;
-	void *tmp;
 
 #ifdef CONFIG_X86_32
 	if (boot_params.efi_info.efi_systab_hi ||

@@ -566,14 +565,16 @@ void __init efi_init(void)
 	/*
 	 * Show what we know for posterity
 	 */
-	c16 = tmp = early_memremap(efi.systab->fw_vendor, 2);
+	c16 = early_memremap_ro(efi.systab->fw_vendor,
+				sizeof(vendor) * sizeof(efi_char16_t));
 	if (c16) {
-		for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i)
-			vendor[i] = *c16++;
+		for (i = 0; i < sizeof(vendor) - 1 && c16[i]; ++i)
+			vendor[i] = c16[i];
 		vendor[i] = '\0';
-	} else
+		early_memunmap(c16, sizeof(vendor) * sizeof(efi_char16_t));
+	} else {
 		pr_err("Could not map the firmware vendor!\n");
-	early_memunmap(tmp, 2);
+	}
 
 	pr_info("EFI v%u.%.02u by %s\n",
 		efi.systab->hdr.revision >> 16,
Signature · 1 line
2.20.1
author maintainer
#5 lore raw

PATCH v2 5/14 efi/x86: avoid redundant cast of EFI firmware service pointer

In reply to Ard Biesheuvel

All EFI firmware call prototypes have been annotated as __efiapi, permitting us to attach attributes regarding the calling convention by overriding __efiapi to an architecture specific value.

On 32-bit x86, EFI firmware calls use the plain calling convention where all arguments are passed via the stack, and cleaned up by the caller. Let's add this to the __efiapi definition so we no longer need to cast the function pointers before invoking them.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/include/asm/efi.h | 8 +-------
 include/linux/efi.h        | 4 +++-
 2 files changed, 4 insertions(+), 8 deletions(-)
arch/x86/include/asm/efi.h +1 -7
index b35b5d423e9d..09c3fc468793 100644
@@ -51,13 +51,7 @@ extern asmlinkage unsigned long efi_call_phys(void *, ...);
 })
 
 
-/*
- * Wrap all the virtual calls in a way that forces the parameters on the stack.
- */
-#define arch_efi_call_virt(p, f, args...)				\
-({									\
-	((efi_##f##_t __attribute__((regparm(0)))*) p->f)(args);	\
-})
+#define arch_efi_call_virt(p, f, args...)	p->f(args)
 
 #define efi_ioremap(addr, size, type, attr)	ioremap_cache(addr, size)
include/linux/efi.h +3 -1
index 726673e98990..952c1659dfd9 100644
@@ -48,8 +48,10 @@ typedef u16 efi_char16_t;		/* UNICODE character */
 typedef u64 efi_physical_addr_t;
 typedef void *efi_handle_t;
 
-#ifdef CONFIG_X86_64
+#if defined(CONFIG_X86_64)
 #define __efiapi __attribute__((ms_abi))
+#elif defined(CONFIG_X86_32)
+#define __efiapi __attribute__((regparm(0)))
 #else
 #define __efiapi
 #endif
Signature · 1 line
2.20.1
author maintainer
#6 lore raw

PATCH v2 6/14 efi/x86: split off some old memmap handling into separate routines

In reply to Ard Biesheuvel

In a subsequent patch, we will fold the prolog/epilog routines that are part of the support code to call SetVirtualAddressMap() with a 1:1 mapping into the callers. However, the 64-bit version mostly consists of ugly mapping code that is only used when efi=old_map is in effect, which is extremely rare. So let's move this code out of the way so it does not clutter the common code.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi_64.c | 35 ++++++++++++--------
 1 file changed, 22 insertions(+), 13 deletions(-)
arch/x86/platform/efi/efi_64.c +22 -13
index 03c2ed3c645c..a72bbabbc595 100644
@@ -72,7 +72,9 @@ static void __init early_code_mapping_set_exec(int executable)
 	}
 }
 
-pgd_t * __init efi_call_phys_prolog(void)
+void __init efi_old_memmap_phys_epilog(pgd_t *save_pgd);
+
+pgd_t * __init efi_old_memmap_phys_prolog(void)
 {
 	unsigned long vaddr, addr_pgd, addr_p4d, addr_pud;
 	pgd_t *save_pgd, *pgd_k, *pgd_efi;

@@ -82,11 +84,6 @@ pgd_t * __init efi_call_phys_prolog(void)
 	int pgd;
 	int n_pgds, i, j;
 
-	if (!efi_enabled(EFI_OLD_MEMMAP)) {
-		efi_switch_mm(&efi_mm);
-		return efi_mm.pgd;
-	}
-
 	early_code_mapping_set_exec(1);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);

@@ -143,11 +140,11 @@ pgd_t * __init efi_call_phys_prolog(void)
 	__flush_tlb_all();
 	return save_pgd;
 out:
-	efi_call_phys_epilog(save_pgd);
+	efi_old_memmap_phys_epilog(save_pgd);
 	return NULL;
 }
 
-void __init efi_call_phys_epilog(pgd_t *save_pgd)
+void __init efi_old_memmap_phys_epilog(pgd_t *save_pgd)
 {
 	/*
 	 * After the lock is released, the original page table is restored.

@@ -158,11 +155,6 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	p4d_t *p4d;
 	pud_t *pud;
 
-	if (!efi_enabled(EFI_OLD_MEMMAP)) {
-		efi_switch_mm(efi_scratch.prev_mm);
-		return;
-	}
-
 	nr_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE);
 
 	for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++) {

@@ -193,6 +185,23 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	early_code_mapping_set_exec(0);
 }
 
+pgd_t * __init efi_call_phys_prolog(void)
+{
+	if (efi_enabled(EFI_OLD_MEMMAP))
+		return efi_old_memmap_phys_prolog();
+
+	efi_switch_mm(&efi_mm);
+	return efi_mm.pgd;
+}
+
+void __init efi_call_phys_epilog(pgd_t *save_pgd)
+{
+	if (efi_enabled(EFI_OLD_MEMMAP))
+		efi_old_memmap_phys_epilog(save_pgd);
+	else
+		efi_switch_mm(efi_scratch.prev_mm);
+}
+
 EXPORT_SYMBOL_GPL(efi_mm);
 
 /*
Signature · 1 line
2.20.1
author maintainer
#7 lore raw

PATCH v2 7/14 efi/x86: split SetVirtualAddresMap() wrappers into 32 and 64 bit versions

In reply to Ard Biesheuvel

Split the phys_efi_set_virtual_address_map() routine into 32 and 64 bit versions, so we can simplify them individually in subsequent patches.

There is very little overlap between the logic anyway, and this has already been factored out in prolog/epilog routines which are completely different between 32 bit and 64 bit. So let's take it one step further, and get rid of the overlap completely.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/include/asm/efi.h     |  8 ++--
 arch/x86/platform/efi/efi.c    | 30 +-----------
 arch/x86/platform/efi/efi_32.c | 21 ++++++--
 arch/x86/platform/efi/efi_64.c | 50 +++++++++++++-------
 4 files changed, 55 insertions(+), 54 deletions(-)
arch/x86/include/asm/efi.h +4 -4
index 09c3fc468793..e29e5dc0b750 100644
@@ -61,8 +61,6 @@ extern asmlinkage unsigned long efi_call_phys(void *, ...);
 
 extern asmlinkage u64 efi_call(void *fp, ...);
 
-#define efi_call_phys(f, args...)		efi_call((f), args)
-
 /*
  * struct efi_scratch - Scratch space used while switching to/from efi_mm
  * @phys_stack: stack used during EFI Mixed Mode

@@ -115,8 +113,6 @@ extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
 extern struct efi_scratch efi_scratch;
 extern void __init efi_set_executable(efi_memory_desc_t *md, bool executable);
 extern int __init efi_memblock_x86_reserve_range(void);
-extern pgd_t * __init efi_call_phys_prolog(void);
-extern void __init efi_call_phys_epilog(pgd_t *save_pgd);
 extern void __init efi_print_memmap(void);
 extern void __init efi_memory_uc(u64 addr, unsigned long size);
 extern void __init efi_map_region(efi_memory_desc_t *md);

@@ -177,6 +173,10 @@ extern efi_status_t efi_thunk_set_virtual_address_map(
 	unsigned long descriptor_size,
 	u32 descriptor_version,
 	efi_memory_desc_t *virtual_map);
+efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
+					 unsigned long descriptor_size,
+					 u32 descriptor_version,
+					 efi_memory_desc_t *virtual_map);
 
 /* arch specific definitions used by the stub code */
arch/x86/platform/efi/efi.c +2 -28
index 3ce32c31bb61..50f8123e658a 100644
@@ -54,7 +54,7 @@
 #include <asm/x86_init.h>
 #include <asm/uv/uv.h>
 
-static struct efi efi_phys __initdata;
+struct efi efi_phys __initdata;
 static efi_system_table_t efi_systab __initdata;
 
 static efi_config_table_type_t arch_tables[] __initdata = {

@@ -97,32 +97,6 @@ static int __init setup_add_efi_memmap(char *arg)
 }
 early_param("add_efi_memmap", setup_add_efi_memmap);
 
-static efi_status_t __init phys_efi_set_virtual_address_map(
-	unsigned long memory_map_size,
-	unsigned long descriptor_size,
-	u32 descriptor_version,
-	efi_memory_desc_t *virtual_map)
-{
-	efi_status_t status;
-	unsigned long flags;
-	pgd_t *save_pgd;
-
-	save_pgd = efi_call_phys_prolog();
-	if (!save_pgd)
-		return EFI_ABORTED;
-
-	/* Disable interrupts around EFI calls: */
-	local_irq_save(flags);
-	status = efi_call_phys(efi_phys.set_virtual_address_map,
-			       memory_map_size, descriptor_size,
-			       descriptor_version, virtual_map);
-	local_irq_restore(flags);
-
-	efi_call_phys_epilog(save_pgd);
-
-	return status;
-}
-
 void __init efi_find_mirror(void)
 {
 	efi_memory_desc_t *md;

@@ -1042,7 +1016,7 @@ static void __init __efi_enter_virtual_mode(void)
 	efi_sync_low_kernel_mappings();
 
 	if (!efi_is_mixed()) {
-		status = phys_efi_set_virtual_address_map(
+		status = efi_set_virtual_address_map(
 				efi.memmap.desc_size * count,
 				efi.memmap.desc_size,
 				efi.memmap.desc_version,
arch/x86/platform/efi/efi_32.c +16 -5
index 9959657127f4..185950ade0e9 100644
@@ -66,9 +66,16 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
-pgd_t * __init efi_call_phys_prolog(void)
+extern struct efi efi_phys;
+
+efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
+						unsigned long descriptor_size,
+						u32 descriptor_version,
+						efi_memory_desc_t *virtual_map)
 {
 	struct desc_ptr gdt_descr;
+	efi_status_t status;
+	unsigned long flags;
 	pgd_t *save_pgd;
 
 	/* Current pgd is swapper_pg_dir, we'll restore it later: */

@@ -80,14 +87,18 @@ pgd_t * __init efi_call_phys_prolog(void)
 	gdt_descr.size = GDT_SIZE - 1;
 	load_gdt(&gdt_descr);
 
-	return save_pgd;
-}
+	/* Disable interrupts around EFI calls: */
+	local_irq_save(flags);
+	status = efi_call_phys(efi_phys.set_virtual_address_map,
+			       memory_map_size, descriptor_size,
+			       descriptor_version, virtual_map);
+	local_irq_restore(flags);
 
-void __init efi_call_phys_epilog(pgd_t *save_pgd)
-{
 	load_fixmap_gdt(0);
 	load_cr3(save_pgd);
 	__flush_tlb_all();
+
+	return status;
 }
 
 void __init efi_runtime_update_mappings(void)
arch/x86/platform/efi/efi_64.c +33 -17

59 lines not on this page · raw message

Signature · 1 line
2.20.1
author maintainer
#8 lore raw

PATCH v2 8/14 efi/x86: simplify i386 efi_call_phys() firmware call wrapper

In reply to Ard Biesheuvel

The variadic efi_call_phys() wrapper that exists on i386 was originally created to call into any EFI firmware runtime service, but in practice, we only use it once, to call SetVirtualAddressMap() during early boot.
The flexibility provided by the variadic nature also makes it type unsafe, and makes the assembler code more complicated than needed, since it has to deal with an unknown number of arguments living on the stack.

So clean this up, by renaming the helper to efi_call_svam(), and dropping the unneeded complexity. Let's also drop the reference to the efi_phys struct and grab the address from the EFI system table directly.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/include/asm/efi.h          |   3 -
 arch/x86/platform/efi/efi_32.c      |   5 +-
 arch/x86/platform/efi/efi_stub_32.S | 109 +++-----------------
 3 files changed, 20 insertions(+), 97 deletions(-)
arch/x86/include/asm/efi.h +0 -3
index e29e5dc0b750..cb08035b89a0 100644
@@ -35,9 +35,6 @@
 #define ARCH_EFI_IRQ_FLAGS_MASK	X86_EFLAGS_IF
 
 #ifdef CONFIG_X86_32
-
-extern asmlinkage unsigned long efi_call_phys(void *, ...);
-
 #define arch_efi_call_virt_setup()					\
 ({									\
 	kernel_fpu_begin();						\
arch/x86/platform/efi/efi_32.c +3 -2
index 185950ade0e9..71dddd1620f9 100644
@@ -66,7 +66,8 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
-extern struct efi efi_phys;
+efi_status_t efi_call_svam(efi_set_virtual_address_map_t *__efiapi *,
+			   u32, u32, u32, void *);
 
 efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
 						unsigned long descriptor_size,

@@ -89,7 +90,7 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
 
 	/* Disable interrupts around EFI calls: */
 	local_irq_save(flags);
-	status = efi_call_phys(efi_phys.set_virtual_address_map,
+	status = efi_call_svam(&efi.systab->runtime->set_virtual_address_map,
 			       memory_map_size, descriptor_size,
 			       descriptor_version, virtual_map);
 	local_irq_restore(flags);
arch/x86/platform/efi/efi_stub_32.S +17 -92

135 lines not on this page · raw message

Signature · 1 line
2.20.1
author maintainer
#9 lore raw

PATCH v2 9/14 efi/x86: simplify 64-bit EFI firmware call wrapper

In reply to Ard Biesheuvel

The efi_call() wrapper used to invoke EFI runtime services serves a number of purposes:
- realign the stack to 16 bytes
- preserve FP and CR0 register state
- translate from SysV to MS calling convention.

Preserving CR0.TS is no longer necessary in Linux, and preserving the FP register state is also redundant in most cases, since efi_call() is almost always used from within the scope of a pair of kernel_fpu_begin()/ kernel_fpu_end() calls, with the exception of the early call to SetVirtualAddressMap() and the SGI UV support code.

So let's add a pair of kernel_fpu_begin()/_end() calls there as well, and remove the unnecessary code from the assembly implementation of efi_call(), and only keep the pieces that deal with the stack alignment and the ABI translation.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/Makefile      |  1 -
 arch/x86/platform/efi/efi_64.c      |  3 ++
 arch/x86/platform/efi/efi_stub_64.S | 39 ++------------------
 arch/x86/platform/uv/bios_uv.c      |  7 +++-
 4 files changed, 12 insertions(+), 38 deletions(-)
arch/x86/platform/efi/Makefile +0 -1
index fe29f3f5d384..7ec3a8b31f8b 100644
@@ -1,6 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 OBJECT_FILES_NON_STANDARD_efi_thunk_$(BITS).o := y
-OBJECT_FILES_NON_STANDARD_efi_stub_$(BITS).o := y
 
 obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
arch/x86/platform/efi/efi_64.c +3 -0
index b7fb2fd24830..809db917cf3e 100644
@@ -1019,6 +1019,8 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
 		efi_switch_mm(&efi_mm);
 	}
 
+	kernel_fpu_begin();
+
 	/* Disable interrupts around EFI calls: */
 	local_irq_save(flags);
 	status = efi_call(efi.systab->runtime->set_virtual_address_map,

@@ -1026,6 +1028,7 @@ efi_status_t __init efi_set_virtual_address_map(unsigned long memory_map_size,
 			  descriptor_version, virtual_map);
 	local_irq_restore(flags);
 
+	kernel_fpu_end();
 
 	if (save_pgd)
 		efi_old_memmap_phys_epilog(save_pgd);
arch/x86/platform/efi/efi_stub_64.S +4 -35
index b1d2313fe3bf..e7e1020f4ccb 100644
@@ -8,41 +8,12 @@
  */
 
 #include <linux/linkage.h>
-#include <asm/segment.h>
-#include <asm/msr.h>
-#include <asm/processor-flags.h>
-#include <asm/page_types.h>
-
-#define SAVE_XMM			\
-	mov %rsp, %rax;			\
-	subq $0x70, %rsp;		\
-	and $~0xf, %rsp;		\
-	mov %rax, (%rsp);		\
-	mov %cr0, %rax;			\
-	clts;				\
-	mov %rax, 0x8(%rsp);		\
-	movaps %xmm0, 0x60(%rsp);	\
-	movaps %xmm1, 0x50(%rsp);	\
-	movaps %xmm2, 0x40(%rsp);	\
-	movaps %xmm3, 0x30(%rsp);	\
-	movaps %xmm4, 0x20(%rsp);	\
-	movaps %xmm5, 0x10(%rsp)
-
-#define RESTORE_XMM			\
-	movaps 0x60(%rsp), %xmm0;	\
-	movaps 0x50(%rsp), %xmm1;	\
-	movaps 0x40(%rsp), %xmm2;	\
-	movaps 0x30(%rsp), %xmm3;	\
-	movaps 0x20(%rsp), %xmm4;	\
-	movaps 0x10(%rsp), %xmm5;	\
-	mov 0x8(%rsp), %rsi;		\
-	mov %rsi, %cr0;			\
-	mov (%rsp), %rsp
+#include <asm/nospec-branch.h>
 
 SYM_FUNC_START(efi_call)
 	pushq %rbp
 	movq %rsp, %rbp
-	SAVE_XMM
+	and $~0xf, %rsp
 	mov 16(%rbp), %rax
 	subq $48, %rsp
 	mov %r9, 32(%rsp)

@@ -50,9 +21,7 @@ SYM_FUNC_START(efi_call)
 	mov %r8, %r9
 	mov %rcx, %r8
 	mov %rsi, %rcx
-	call *%rdi
-	addq $48, %rsp
-	RESTORE_XMM
-	popq %rbp
+	CALL_NOSPEC %rdi
+	leave
 	ret
 SYM_FUNC_END(efi_call)
arch/x86/platform/uv/bios_uv.c +5 -2
index ece9cb9c1189..5c0e2eb5d87c 100644
@@ -34,10 +34,13 @@ static s64 __uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3,
 	 * If EFI_OLD_MEMMAP is set, we need to fall back to using our old EFI
 	 * callback method, which uses efi_call() directly, with the kernel page tables:
 	 */
-	if (unlikely(efi_enabled(EFI_OLD_MEMMAP)))
+	if (unlikely(efi_enabled(EFI_OLD_MEMMAP))) {
+		kernel_fpu_begin();
 		ret = efi_call((void *)__va(tab->function), (u64)which, a1, a2, a3, a4, a5);
-	else
+		kernel_fpu_end();
+	} else {
 		ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5);
+	}
 
 	return ret;
 }
Signature · 1 line
2.20.1
author maintainer
#10 lore raw

PATCH v2 10/14 efi/x86: simplify mixed mode call wrapper

In reply to Ard Biesheuvel

Calling 32-bit EFI runtime services from a 64-bit OS involves switching back to the flat mapping with a stack carved out of memory that is 32-bit addressable.

There is no need to actually execute the 64-bit part of this routine from the flat mapping as well, as long as the entry and return address fit in 32 bits. There is also no need to preserve part of the calling context in global variables: we can simply push the old stack pointer value to the new stack, and keep the return address from the code32 section in EBX.

While at it, move the conditional check whether to invoke the mixed mode version of SetVirtualAddressMap() into the 64-bit implementation of the wrapper routine.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/include/asm/efi.h           |   6 -
 arch/x86/platform/efi/efi.c          |  19 +--
 arch/x86/platform/efi/efi_64.c       |  66 +++++++----
 arch/x86/platform/efi/efi_thunk_64.S | 121 ++++----------------
 4 files changed, 67 insertions(+), 145 deletions(-)
arch/x86/include/asm/efi.h +0 -6
index cb08035b89a0..e7e9c6e057f9 100644
@@ -164,12 +164,6 @@ extern void parse_efi_setup(u64 phys_addr, u32 data_len);
 extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
 
 extern void efi_thunk_runtime_setup(void);
-extern efi_status_t efi_thunk_set_virtual_address_map(
-	void *phys_set_virtual_address_map,
-	unsigned long memory_map_size,
-	unsigned long descriptor_size,
-	u32 descriptor_version,
-	efi_memory_desc_t *virtual_map);
 efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
 					 unsigned long descriptor_size,
 					 u32 descriptor_version,
arch/x86/platform/efi/efi.c +4 -15
index 50f8123e658a..e4d3afac7be3 100644
@@ -1015,21 +1015,10 @@ static void __init __efi_enter_virtual_mode(void)
 
 	efi_sync_low_kernel_mappings();
 
-	if (!efi_is_mixed()) {
-		status = efi_set_virtual_address_map(
-				efi.memmap.desc_size * count,
-				efi.memmap.desc_size,
-				efi.memmap.desc_version,
-				(efi_memory_desc_t *)pa);
-	} else {
-		status = efi_thunk_set_virtual_address_map(
-				efi_phys.set_virtual_address_map,
-				efi.memmap.desc_size * count,
-				efi.memmap.desc_size,
-				efi.memmap.desc_version,
-				(efi_memory_desc_t *)pa);
-	}
-
+	status = efi_set_virtual_address_map(efi.memmap.desc_size * count,
+					     efi.memmap.desc_size,
+					     efi.memmap.desc_version,
+					     (efi_memory_desc_t *)pa);
 	if (status != EFI_SUCCESS) {
 		pr_alert("Unable to switch EFI into virtual mode (status=%lx)!\n",
 			 status);
arch/x86/platform/efi/efi_64.c +43 -23

107 lines not on this page · raw message

arch/x86/platform/efi/efi_thunk_64.S +20 -101

148 lines not on this page · raw message

Signature · 1 line
2.20.1
author maintainer
#11 lore raw

PATCH v2 11/14 efi/x86: drop two near identical versions of efi_runtime_init()

In reply to Ard Biesheuvel

The routines efi_runtime_init32() and efi_runtime_init64() are almost indistinguishable, and the only relevant difference is the offset in the runtime struct from where to obtain the physical address of the SetVirtualAddressMap() routine.

However, this address is only used once, when installing the virtual address map that the OS will use to invoke EFI runtime services, and at the time of the call, we will necessarily be running with a 1:1 mapping, and so there is no need to do the map/unmap dance here to retrieve the address. In fact, in the preceding changes to these users, we stopped using the address recorded here entirely.

So let's just get rid of all this code since it no longer serves a purpose. While at it, tweak the logic so that we handle unsupported and disable EFI runtime services in the same way, and unmap the EFI memory map in both cases.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi.c | 95 ++------------------
 include/linux/efi.h         | 19 ----
 2 files changed, 5 insertions(+), 109 deletions(-)
arch/x86/platform/efi/efi.c +5 -90
index e4d3afac7be3..67cb0fd18777 100644
@@ -429,87 +429,6 @@ static int __init efi_systab_init(void *phys)
 	return 0;
 }
 
-static int __init efi_runtime_init32(void)
-{
-	efi_runtime_services_32_t *runtime;
-
-	runtime = early_memremap((unsigned long)efi.systab->runtime,
-			sizeof(efi_runtime_services_32_t));
-	if (!runtime) {
-		pr_err("Could not map the runtime service table!\n");
-		return -ENOMEM;
-	}
-
-	/*
-	 * We will only need *early* access to the SetVirtualAddressMap
-	 * EFI runtime service. All other runtime services will be called
-	 * via the virtual mapping.
-	 */
-	efi_phys.set_virtual_address_map =
-			(efi_set_virtual_address_map_t *)
-			(unsigned long)runtime->set_virtual_address_map;
-	early_memunmap(runtime, sizeof(efi_runtime_services_32_t));
-
-	return 0;
-}
-
-static int __init efi_runtime_init64(void)
-{
-	efi_runtime_services_64_t *runtime;
-
-	runtime = early_memremap((unsigned long)efi.systab->runtime,
-			sizeof(efi_runtime_services_64_t));
-	if (!runtime) {
-		pr_err("Could not map the runtime service table!\n");
-		return -ENOMEM;
-	}
-
-	/*
-	 * We will only need *early* access to the SetVirtualAddressMap
-	 * EFI runtime service. All other runtime services will be called
-	 * via the virtual mapping.
-	 */
-	efi_phys.set_virtual_address_map =
-			(efi_set_virtual_address_map_t *)
-			(unsigned long)runtime->set_virtual_address_map;
-	early_memunmap(runtime, sizeof(efi_runtime_services_64_t));
-
-	return 0;
-}
-
-static int __init efi_runtime_init(void)
-{
-	int rv;
-
-	/*
-	 * Check out the runtime services table. We need to map
-	 * the runtime services table so that we can grab the physical
-	 * address of several of the EFI runtime functions, needed to
-	 * set the firmware into virtual mode.
-	 *
-	 * When EFI_PARAVIRT is in force then we could not map runtime
-	 * service memory region because we do not have direct access to it.
-	 * However, runtime services are available through proxy functions
-	 * (e.g. in case of Xen dom0 EFI implementation they call special
-	 * hypercall which executes relevant EFI functions) and that is why
-	 * they are always enabled.
-	 */
-
-	if (!efi_enabled(EFI_PARAVIRT)) {
-		if (efi_enabled(EFI_64BIT))
-			rv = efi_runtime_init64();
-		else
-			rv = efi_runtime_init32();
-
-		if (rv)
-			return rv;
-	}
-
-	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
-
-	return 0;
-}
-
 void __init efi_init(void)
 {
 	efi_char16_t *c16;

@@ -567,13 +486,13 @@ void __init efi_init(void)
 
 	if (!efi_runtime_supported())
 		pr_info("No EFI runtime due to 32/64-bit mismatch with kernel\n");
-	else {
-		if (efi_runtime_disabled() || efi_runtime_init()) {
-			efi_memmap_unmap();
-			return;
-		}
+
+	if (!efi_runtime_supported() || efi_runtime_disabled()) {
+		efi_memmap_unmap();
+		return;
 	}
 
+	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	efi_clean_memmap();
 
 	if (efi_enabled(EFI_DBG))

@@ -934,8 +853,6 @@ static void __init kexec_enter_virtual_mode(void)
 
 	efi_native_runtime_setup();
 
-	efi.set_virtual_address_map = NULL;
-
 	if (efi_enabled(EFI_OLD_MEMMAP) && (__supported_pte_mask & _PAGE_NX))
 		runtime_code_page_mkexec();
 #endif

@@ -1040,8 +957,6 @@ static void __init __efi_enter_virtual_mode(void)
 	else
 		efi_thunk_runtime_setup();
 
-	efi.set_virtual_address_map = NULL;
-
 	/*
 	 * Apply more restrictive page table mapping attributes now that
 	 * SVAM() has been called and the firmware has performed all
include/linux/efi.h +0 -19
index 952c1659dfd9..ee68ea6f85ff 100644
@@ -539,24 +539,6 @@ typedef struct {
 	u32 query_variable_info;
 } efi_runtime_services_32_t;
 
-typedef struct {
-	efi_table_hdr_t hdr;
-	u64 get_time;
-	u64 set_time;
-	u64 get_wakeup_time;
-	u64 set_wakeup_time;
-	u64 set_virtual_address_map;
-	u64 convert_pointer;
-	u64 get_variable;
-	u64 get_next_variable;
-	u64 set_variable;
-	u64 get_next_high_mono_count;
-	u64 reset_system;
-	u64 update_capsule;
-	u64 query_capsule_caps;
-	u64 query_variable_info;
-} efi_runtime_services_64_t;
-
 typedef efi_status_t efi_get_time_t (efi_time_t *tm, efi_time_cap_t *tc);
 typedef efi_status_t efi_set_time_t (efi_time_t *tm);
 typedef efi_status_t efi_get_wakeup_time_t (efi_bool_t *enabled, efi_bool_t *pending,

@@ -946,7 +928,6 @@ extern struct efi {
 	efi_query_capsule_caps_t *query_capsule_caps;
 	efi_get_next_high_mono_count_t *get_next_high_mono_count;
 	efi_reset_system_t *reset_system;
-	efi_set_virtual_address_map_t *set_virtual_address_map;
 	struct efi_memory_map memmap;
 	unsigned long flags;
 } efi;
Signature · 1 line
2.20.1
author maintainer
#12 lore raw

PATCH v2 12/14 efi/x86: clean up efi_systab_init() routine for legibility

In reply to Ard Biesheuvel

Clean up the efi_systab_init() routine which maps the EFI system table and copies the relevant pieces of data out of it.

The current routine is very difficult to read, so let's clean that up. Also, switch to a R/O mapping of the system table since that is all we need.

Finally, use a plain u64 variable to record the physical address of the system table instead of pointlessly stashing it in a struct efi that is never used for anything else.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi.c | 166 ++++++++++----------
 1 file changed, 82 insertions(+), 84 deletions(-)
arch/x86/platform/efi/efi.c +82 -84
index 67cb0fd18777..53ed0b123641 100644
@@ -54,8 +54,8 @@
 #include <asm/x86_init.h>
 #include <asm/uv/uv.h>
 
-struct efi efi_phys __initdata;
 static efi_system_table_t efi_systab __initdata;
+static u64 efi_systab_phys __initdata;
 
 static efi_config_table_type_t arch_tables[] __initdata = {
 #ifdef CONFIG_X86_UV

@@ -327,89 +327,90 @@ void __init efi_print_memmap(void)
 	}
 }
 
-static int __init efi_systab_init(void *phys)
+static int __init efi_systab_init(u64 phys)
 {
+	int size = efi_enabled(EFI_64BIT) ? sizeof(efi_system_table_64_t)
+					  : sizeof(efi_system_table_32_t);
+	bool over4g = false;
+	void *p;
+
+	p = early_memremap_ro(phys, size);
+	if (p == NULL) {
+		pr_err("Couldn't map the system table!\n");
+		return -ENOMEM;
+	}
+
 	if (efi_enabled(EFI_64BIT)) {
-		efi_system_table_64_t *systab64;
-		struct efi_setup_data *data = NULL;
-		u64 tmp = 0;
+		const efi_system_table_64_t *systab64 = p;
+
+		efi_systab.hdr			= systab64->hdr;
+		efi_systab.fw_vendor		= systab64->fw_vendor;
+		efi_systab.fw_revision		= systab64->fw_revision;
+		efi_systab.con_in_handle	= systab64->con_in_handle;
+		efi_systab.con_in		= systab64->con_in;
+		efi_systab.con_out_handle	= systab64->con_out_handle;
+		efi_systab.con_out		= (void *)(unsigned long)systab64->con_out;
+		efi_systab.stderr_handle	= systab64->stderr_handle;
+		efi_systab.stderr		= systab64->stderr;
+		efi_systab.runtime		= (void *)(unsigned long)systab64->runtime;
+		efi_systab.boottime		= (void *)(unsigned long)systab64->boottime;
+		efi_systab.nr_tables		= systab64->nr_tables;
+		efi_systab.tables		= systab64->tables;
+
+		over4g = systab64->con_in_handle	> U32_MAX ||
+			 systab64->con_in		> U32_MAX ||
+			 systab64->con_out_handle	> U32_MAX ||
+			 systab64->con_out		> U32_MAX ||
+			 systab64->stderr_handle	> U32_MAX ||
+			 systab64->stderr		> U32_MAX ||
+			 systab64->boottime		> U32_MAX;
 
 		if (efi_setup) {
-			data = early_memremap(efi_setup, sizeof(*data));
-			if (!data)
+			struct efi_setup_data *data;
+
+			data = early_memremap_ro(efi_setup, sizeof(*data));
+			if (!data) {
+				early_memunmap(p, size);
 				return -ENOMEM;
-		}
-		systab64 = early_memremap((unsigned long)phys,
-					 sizeof(*systab64));
-		if (systab64 == NULL) {
-			pr_err("Couldn't map the system table!\n");
-			if (data)
-				early_memunmap(data, sizeof(*data));
-			return -ENOMEM;
-		}
+			}
+
+			efi_systab.fw_vendor	= (unsigned long)data->fw_vendor;
+			efi_systab.runtime	= (void *)(unsigned long)data->runtime;
+			efi_systab.tables	= (unsigned long)data->tables;
+
+			over4g |= data->fw_vendor	> U32_MAX ||
+				  data->runtime		> U32_MAX ||
+				  data->tables		> U32_MAX;
 
-		efi_systab.hdr = systab64->hdr;
-		efi_systab.fw_vendor = data ? (unsigned long)data->fw_vendor :
-					      systab64->fw_vendor;
-		tmp |= data ? data->fw_vendor : systab64->fw_vendor;
-		efi_systab.fw_revision = systab64->fw_revision;
-		efi_systab.con_in_handle = systab64->con_in_handle;
-		tmp |= systab64->con_in_handle;
-		efi_systab.con_in = systab64->con_in;
-		tmp |= systab64->con_in;
-		efi_systab.con_out_handle = systab64->con_out_handle;
-		tmp |= systab64->con_out_handle;
-		efi_systab.con_out = (void *)(unsigned long)systab64->con_out;
-		tmp |= systab64->con_out;
-		efi_systab.stderr_handle = systab64->stderr_handle;
-		tmp |= systab64->stderr_handle;
-		efi_systab.stderr = systab64->stderr;
-		tmp |= systab64->stderr;
-		efi_systab.runtime = data ?
-				     (void *)(unsigned long)data->runtime :
-				     (void *)(unsigned long)systab64->runtime;
-		tmp |= data ? data->runtime : systab64->runtime;
-		efi_systab.boottime = (void *)(unsigned long)systab64->boottime;
-		tmp |= systab64->boottime;
-		efi_systab.nr_tables = systab64->nr_tables;
-		efi_systab.tables = data ? (unsigned long)data->tables :
-					   systab64->tables;
-		tmp |= data ? data->tables : systab64->tables;
-
-		early_memunmap(systab64, sizeof(*systab64));
-		if (data)
 			early_memunmap(data, sizeof(*data));
-#ifdef CONFIG_X86_32
-		if (tmp >> 32) {
-			pr_err("EFI data located above 4GB, disabling EFI.\n");
-			return -EINVAL;
+		} else {
+			over4g |= systab64->fw_vendor	> U32_MAX ||
+				  systab64->runtime	> U32_MAX ||
+				  systab64->tables	> U32_MAX;
 		}
-#endif
 	} else {
-		efi_system_table_32_t *systab32;
-
-		systab32 = early_memremap((unsigned long)phys,
-					 sizeof(*systab32));
-		if (systab32 == NULL) {
-			pr_err("Couldn't map the system table!\n");
-			return -ENOMEM;
-		}
+		const efi_system_table_32_t *systab32 = p;
+
+		efi_systab.hdr			= systab32->hdr;
+		efi_systab.fw_vendor		= systab32->fw_vendor;
+		efi_systab.fw_revision		= systab32->fw_revision;
+		efi_systab.con_in_handle	= systab32->con_in_handle;
+		efi_systab.con_in		= systab32->con_in;
+		efi_systab.con_out_handle	= systab32->con_out_handle;
+		efi_systab.con_out		= (void *)(unsigned long)systab32->con_out;
+		efi_systab.stderr_handle	= systab32->stderr_handle;
+		efi_systab.stderr		= systab32->stderr;
+		efi_systab.runtime		= (void *)(unsigned long)systab32->runtime;
+		efi_systab.boottime		= (void *)(unsigned long)systab32->boottime;
+		efi_systab.nr_tables		= systab32->nr_tables;
+		efi_systab.tables		= systab32->tables;
+	}
 
-		efi_systab.hdr = systab32->hdr;
-		efi_systab.fw_vendor = systab32->fw_vendor;
-		efi_systab.fw_revision = systab32->fw_revision;
-		efi_systab.con_in_handle = systab32->con_in_handle;
-		efi_systab.con_in = systab32->con_in;
-		efi_systab.con_out_handle = systab32->con_out_handle;
-		efi_systab.con_out = (void *)(unsigned long)systab32->con_out;
-		efi_systab.stderr_handle = systab32->stderr_handle;
-		efi_systab.stderr = systab32->stderr;
-		efi_systab.runtime = (void *)(unsigned long)systab32->runtime;
-		efi_systab.boottime = (void *)(unsigned long)systab32->boottime;
-		efi_systab.nr_tables = systab32->nr_tables;
-		efi_systab.tables = systab32->tables;
+	early_memunmap(p, size);
 
-		early_memunmap(systab32, sizeof(*systab32));
+	if (IS_ENABLED(CONFIG_X86_32) && over4g) {
+		pr_err("EFI data located above 4GB, disabling EFI.\n");
+		return -EINVAL;
 	}
 
 	efi.systab = &efi_systab;

@@ -435,20 +436,17 @@ void __init efi_init(void)
 	char vendor[100] = "unknown";
 	int i = 0;
 
-#ifdef CONFIG_X86_32
-	if (boot_params.efi_info.efi_systab_hi ||
-	    boot_params.efi_info.efi_memmap_hi) {
+	if (IS_ENABLED(CONFIG_X86_32) &&
+	    (boot_params.efi_info.efi_systab_hi ||
+	     boot_params.efi_info.efi_memmap_hi)) {
 		pr_info("Table located above 4GB, disabling EFI.\n");
 		return;
 	}
-	efi_phys.systab = (efi_system_table_t *)boot_params.efi_info.efi_systab;
-#else
-	efi_phys.systab = (efi_system_table_t *)
-			  (boot_params.efi_info.efi_systab |
-			  ((__u64)boot_params.efi_info.efi_systab_hi<<32));
-#endif
 
-	if (efi_systab_init(efi_phys.systab))
+	efi_systab_phys = boot_params.efi_info.efi_systab |
+			  ((__u64)boot_params.efi_info.efi_systab_hi << 32);
+
+	if (efi_systab_init(efi_systab_phys))
 		return;
 
 	efi.config_table = (unsigned long)efi.systab->tables;

@@ -601,7 +599,7 @@ static void __init get_systab_virt_addr(efi_memory_desc_t *md)
 
 	size = md->num_pages << EFI_PAGE_SHIFT;
 	end = md->phys_addr + size;
-	systab = (u64)(unsigned long)efi_phys.systab;
+	systab = efi_systab_phys;
 	if (md->phys_addr <= systab && systab < end) {
 		systab += md->virt_addr - md->phys_addr;
 		efi.systab = (efi_system_table_t *)(unsigned long)systab;
Signature · 1 line
2.20.1
author maintainer
#13 lore raw

PATCH v2 13/14 efi/x86: don't panic or BUG() on non-critical error conditions

In reply to Ard Biesheuvel

The logic in __efi_enter_virtual_mode() does a number of steps in sequence, all of which may fail in one way or the other. In most cases, we simply print an error and disable EFI runtime services support, but in some cases, we BUG() or panic() and bring down the system when encountering conditions that we could easily handle in the same way.

While at it, replace a pointless page-to-virt-phys conversion with one that goes straight from struct page to physical.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi.c    | 28 ++++++++++----------
 arch/x86/platform/efi/efi_64.c |  9 ++++---
 2 files changed, 19 insertions(+), 18 deletions(-)
arch/x86/platform/efi/efi.c +14 -14
index 53ed0b123641..4f539bfdc051 100644
@@ -889,16 +889,14 @@ static void __init __efi_enter_virtual_mode(void)
 
 	if (efi_alloc_page_tables()) {
 		pr_err("Failed to allocate EFI page tables\n");
-		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
-		return;
+		goto err;
 	}
 
 	efi_merge_regions();
 	new_memmap = efi_map_regions(&count, &pg_shift);
 	if (!new_memmap) {
 		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
-		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
-		return;
+		goto err;
 	}
 
 	pa = __pa(new_memmap);

@@ -912,8 +910,7 @@ static void __init __efi_enter_virtual_mode(void)
 
 	if (efi_memmap_init_late(pa, efi.memmap.desc_size * count)) {
 		pr_err("Failed to remap late EFI memory map\n");
-		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
-		return;
+		goto err;
 	}
 
 	if (efi_enabled(EFI_DBG)) {

@@ -921,12 +918,11 @@ static void __init __efi_enter_virtual_mode(void)
 		efi_print_memmap();
 	}
 
-	BUG_ON(!efi.systab);
+	if (WARN_ON(!efi.systab))
+		goto err;
 
-	if (efi_setup_page_tables(pa, 1 << pg_shift)) {
-		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
-		return;
-	}
+	if (efi_setup_page_tables(pa, 1 << pg_shift))
+		goto err;
 
 	efi_sync_low_kernel_mappings();
 

@@ -935,9 +931,9 @@ static void __init __efi_enter_virtual_mode(void)
 					     efi.memmap.desc_version,
 					     (efi_memory_desc_t *)pa);
 	if (status != EFI_SUCCESS) {
-		pr_alert("Unable to switch EFI into virtual mode (status=%lx)!\n",
-			 status);
-		panic("EFI call to SetVirtualAddressMap() failed!");
+		pr_err("Unable to switch EFI into virtual mode (status=%lx)!\n",
+		       status);
+		goto err;
 	}
 
 	efi_free_boot_services();

@@ -964,6 +960,10 @@ static void __init __efi_enter_virtual_mode(void)
 
 	/* clean DUMMY object */
 	efi_delete_dummy_variable();
+	return;
+
+err:
+	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 }
 
 void __init efi_enter_virtual_mode(void)
arch/x86/platform/efi/efi_64.c +5 -4
index 90db36ddabed..514c7fbf43a3 100644
@@ -384,11 +384,12 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 0;
 
 	page = alloc_page(GFP_KERNEL|__GFP_DMA32);
-	if (!page)
-		panic("Unable to allocate EFI runtime stack < 4GB\n");
+	if (!page) {
+		pr_err("Unable to allocate EFI runtime stack < 4GB\n");
+		return 1;
+	}
 
-	efi_scratch.phys_stack = virt_to_phys(page_address(page));
-	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
+	efi_scratch.phys_stack = page_to_phys(page + 1); /* stack grows down */
 
 	npages = (_etext - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
Signature · 1 line
2.20.1
author maintainer
#14 lore raw

PATCH v2 14/14 efi/x86: remove unreachable code in kexec_enter_virtual_mode()

In reply to Ard Biesheuvel

Remove some code that is guaranteed to be unreachable, given that we have already bailed by this time if EFI_OLD_MEMMAP is set.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi.c | 3 ---
 1 file changed, 3 deletions(-)
arch/x86/platform/efi/efi.c +0 -3
index 4f539bfdc051..b931c4bea284 100644
@@ -850,9 +850,6 @@ static void __init kexec_enter_virtual_mode(void)
 	efi.runtime_version = efi_systab.hdr.revision;
 
 	efi_native_runtime_setup();
-
-	if (efi_enabled(EFI_OLD_MEMMAP) && (__supported_pte_mask & _PAGE_NX))
-		runtime_code_page_mkexec();
 #endif
 }
Signature · 1 line
2.20.1