[PATCH v2 00/11] efi: Clean up runtime wrapper and wire it up for PRM

[PATCH v2 00/11] efi: Clean up runtime wrapper and wire it up for PRM

[Ard Biesheuvel]

ACPI PRM uses the EFI runtime services infrastructure, but currently, it
issues the firmware calls directly, instead of going through the
wrappers and handing off the call to the EFI workqueue.

Given that ACPI PRM is used for vendor code rather than core EFI runtime
services, it would be nice if these calls get sandboxed in the same way
as EFI runtime services calls are. This ensures that any faults
occurring in the firmware are handled gracefully and don't bring down
the kernel.

Another reason for using the work queue is the fact that on some
platforms, the EFI memory regions are remapped into the lower address
space, and this means that sampling the instruction pointer in a perf
interrupt may cause confusion about whether the task is running in user
space or in the firmware.

So let's move the ACPI PRM calls into the EFI runtime wrapper
infrastructure. Before that, let's clean it up a bit.

Changes since v2:
- add patches to move EFI runtime setup/teardown sequences out of line
- add patch to deduplicate setup/teardown function calls in the
  workqueue handler
- some whitespace cleanup and added a missing __init
- add RFC patches to drop EFI runtime asm trampoline from x86
- add Rafael's ack to the patch that touches drivers/acpi/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>

Ard Biesheuvel (11):
  efi/x86: Move EFI runtime call setup/teardown helpers out of line
  efi/arm64: Move EFI runtime call setup/teardown helpers out of line
  efi/riscv: Move EFI runtime call setup/teardown helpers out of line
  efi/runtime-wrappers: Use type safe encapsulation of call arguments
  efi/runtime-wrapper: Move workqueue manipulation out of line
  efi/runtime-wrappers: Remove duplicated macro for service returning
    void
  efi/runtime-wrappers: Don't duplicate setup/teardown code
  acpi/prmt: Use EFI runtime sandbox to invoke PRM handlers
  efi/runtime-wrappers: Clean up white space and add __init annotation
  efi/x86: Realign EFI runtime stack
  efi/x86: Rely on compiler to emit MS ABI calls

 arch/arm64/include/asm/efi.h            |  18 +-
 arch/arm64/kernel/efi.c                 |  16 +-
 arch/riscv/include/asm/efi.h            |  10 +-
 arch/x86/Makefile                       |   3 +
 arch/x86/include/asm/efi.h              |  43 +--
 arch/x86/include/asm/uv/bios.h          |   3 +-
 arch/x86/platform/efi/Makefile          |   6 +-
 arch/x86/platform/efi/efi_32.c          |  12 +
 arch/x86/platform/efi/efi_64.c          |  21 +-
 arch/x86/platform/efi/efi_stub_64.S     |  27 --
 arch/x86/platform/uv/Makefile           |   1 +
 arch/x86/platform/uv/bios_uv.c          |   4 +-
 drivers/acpi/Kconfig                    |   2 +-
 drivers/acpi/prmt.c                     |   8 +-
 drivers/firmware/efi/Makefile           |   1 +
 drivers/firmware/efi/riscv-runtime.c    |  15 +-
 drivers/firmware/efi/runtime-wrappers.c | 376 +++++++++++++-------
 include/linux/efi.h                     |  52 ++-
 18 files changed, 353 insertions(+), 265 deletions(-)
 delete mode 100644 arch/x86/platform/efi/efi_stub_64.S

-- 
2.39.2

[PATCH v2 01/11] efi/x86: Move EFI runtime call setup/teardown helpers out of line

[Ard Biesheuvel]

Only the arch_efi_call_virt() macro that some architectures override
needs to be a macro, given that it is variadic and encapsulates calls
via function pointers that have different prototypes.

The associated setup and teardown code are not special in this regard,
and don't need to be instantiated at each call site. So turn them into
ordinary C functions and move them out of line.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h     | 32 ++------------------
 arch/x86/platform/efi/efi_32.c | 12 ++++++++
 arch/x86/platform/efi/efi_64.c | 19 ++++++++++--
 3 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index b0994ae3bc23f84d..c4555b269a1b2474 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -91,19 +91,6 @@ static inline void efi_fpu_end(void)
 
 #ifdef CONFIG_X86_32
 #define EFI_X86_KERNEL_ALLOC_LIMIT		(SZ_512M - 1)
-
-#define arch_efi_call_virt_setup()					\
-({									\
-	efi_fpu_begin();						\
-	firmware_restrict_branch_speculation_start();			\
-})
-
-#define arch_efi_call_virt_teardown()					\
-({									\
-	firmware_restrict_branch_speculation_end();			\
-	efi_fpu_end();							\
-})
-
 #else /* !CONFIG_X86_32 */
 #define EFI_X86_KERNEL_ALLOC_LIMIT		EFI_ALLOC_LIMIT
 
@@ -116,14 +103,6 @@ extern bool efi_disable_ibt_for_runtime;
 	__efi_call(__VA_ARGS__);					\
 })
 
-#define arch_efi_call_virt_setup()					\
-({									\
-	efi_sync_low_kernel_mappings();					\
-	efi_fpu_begin();						\
-	firmware_restrict_branch_speculation_start();			\
-	efi_enter_mm();							\
-})
-
 #undef arch_efi_call_virt
 #define arch_efi_call_virt(p, f, args...) ({				\
 	u64 ret, ibt = ibt_save(efi_disable_ibt_for_runtime);		\
@@ -132,13 +111,6 @@ extern bool efi_disable_ibt_for_runtime;
 	ret;								\
 })
 
-#define arch_efi_call_virt_teardown()					\
-({									\
-	efi_leave_mm();							\
-	firmware_restrict_branch_speculation_end();			\
-	efi_fpu_end();							\
-})
-
 #ifdef CONFIG_KASAN
 /*
  * CONFIG_KASAN may redefine memset to __memset.  __memset function is present
@@ -168,8 +140,8 @@ extern void efi_delete_dummy_variable(void);
 extern void efi_crash_gracefully_on_page_fault(unsigned long phys_addr);
 extern void efi_free_boot_services(void);
 
-void efi_enter_mm(void);
-void efi_leave_mm(void);
+void arch_efi_call_virt_setup(void);
+void arch_efi_call_virt_teardown(void);
 
 /* kexec external ABI */
 struct efi_setup_data {
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index e06a199423c0fedd..b2cc7b4552a16630 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -140,3 +140,15 @@ void __init efi_runtime_update_mappings(void)
 		}
 	}
 }
+
+void arch_efi_call_virt_setup(void)
+{
+	efi_fpu_begin();
+	firmware_restrict_branch_speculation_start();
+}
+
+void arch_efi_call_virt_teardown(void)
+{
+	firmware_restrict_branch_speculation_end();
+	efi_fpu_end();
+}
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 77f7ac3668cb4ac0..91d31ac422d6cde7 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -474,19 +474,34 @@ void __init efi_dump_pagetable(void)
  * can not change under us.
  * It should be ensured that there are no concurrent calls to this function.
  */
-void efi_enter_mm(void)
+static void efi_enter_mm(void)
 {
 	efi_prev_mm = current->active_mm;
 	current->active_mm = &efi_mm;
 	switch_mm(efi_prev_mm, &efi_mm, NULL);
 }
 
-void efi_leave_mm(void)
+static void efi_leave_mm(void)
 {
 	current->active_mm = efi_prev_mm;
 	switch_mm(&efi_mm, efi_prev_mm, NULL);
 }
 
+void arch_efi_call_virt_setup(void)
+{
+	efi_sync_low_kernel_mappings();
+	efi_fpu_begin();
+	firmware_restrict_branch_speculation_start();
+	efi_enter_mm();
+}
+
+void arch_efi_call_virt_teardown(void)
+{
+	efi_leave_mm();
+	firmware_restrict_branch_speculation_end();
+	efi_fpu_end();
+}
+
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
 /*
-- 
2.39.2

[PATCH v2 02/11] efi/arm64: Move EFI runtime call setup/teardown helpers out of line

[Ard Biesheuvel]

Only the arch_efi_call_virt() macro that some architectures override
needs to be a macro, given that it is variadic and encapsulates calls
via function pointers that have different prototypes.

The associated setup and teardown code are not special in this regard,
and don't need to be instantiated at each call site. So turn them into
ordinary C functions and move them out of line.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/efi.h | 18 +++---------------
 arch/arm64/kernel/efi.c      | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 4cf2cb053bc8df95..f482b994c608ccf3 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -30,28 +30,16 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md,
 				bool has_bti);
 
-#define arch_efi_call_virt_setup()					\
-({									\
-	efi_virtmap_load();						\
-	__efi_fpsimd_begin();						\
-	raw_spin_lock(&efi_rt_lock);					\
-})
-
 #undef arch_efi_call_virt
 #define arch_efi_call_virt(p, f, args...)				\
 	__efi_rt_asm_wrapper((p)->f, #f, args)
 
-#define arch_efi_call_virt_teardown()					\
-({									\
-	raw_spin_unlock(&efi_rt_lock);					\
-	__efi_fpsimd_end();						\
-	efi_virtmap_unload();						\
-})
-
-extern raw_spinlock_t efi_rt_lock;
 extern u64 *efi_rt_stack_top;
 efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
 
+void arch_efi_call_virt_setup(void);
+void arch_efi_call_virt_teardown(void);
+
 /*
  * efi_rt_stack_top[-1] contains the value the stack pointer had before
  * switching to the EFI runtime stack.
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index baab8dd3ead3c27a..49efbdbd6f7ae766 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -158,7 +158,21 @@ asmlinkage efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f)
 	return s;
 }
 
-DEFINE_RAW_SPINLOCK(efi_rt_lock);
+static DEFINE_RAW_SPINLOCK(efi_rt_lock);
+
+void arch_efi_call_virt_setup(void)
+{
+	efi_virtmap_load();
+	__efi_fpsimd_begin();
+	raw_spin_lock(&efi_rt_lock);
+}
+
+void arch_efi_call_virt_teardown(void)
+{
+	raw_spin_unlock(&efi_rt_lock);
+	__efi_fpsimd_end();
+	efi_virtmap_unload();
+}
 
 asmlinkage u64 *efi_rt_stack_top __ro_after_init;
 
-- 
2.39.2

[PATCH v2 03/11] efi/riscv: Move EFI runtime call setup/teardown helpers out of line

[Ard Biesheuvel]

Only the arch_efi_call_virt() macro that some architectures override
needs to be a macro, given that it is variadic and encapsulates calls
via function pointers that have different prototypes.

The associated setup and teardown code are not special in this regard,
and don't need to be instantiated at each call site. So turn them into
ordinary C functions and move them out of line.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/riscv/include/asm/efi.h         | 10 ++--------
 drivers/firmware/efi/riscv-runtime.c | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
index 29e9a0d84b16682f..8a6a128ec57fc02b 100644
--- a/arch/riscv/include/asm/efi.h
+++ b/arch/riscv/include/asm/efi.h
@@ -21,12 +21,6 @@ extern void efi_init(void);
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md, bool);
 
-#define arch_efi_call_virt_setup()      ({		\
-		sync_kernel_mappings(efi_mm.pgd);	\
-		efi_virtmap_load();			\
-	})
-#define arch_efi_call_virt_teardown()   efi_virtmap_unload()
-
 #define ARCH_EFI_IRQ_FLAGS_MASK (SR_IE | SR_SPIE)
 
 /* Load initrd anywhere in system RAM */
@@ -46,8 +40,8 @@ static inline unsigned long efi_get_kimg_min_align(void)
 
 #define EFI_KIMG_PREFERRED_ADDRESS	efi_get_kimg_min_align()
 
-void efi_virtmap_load(void);
-void efi_virtmap_unload(void);
+void arch_efi_call_virt_setup(void);
+void arch_efi_call_virt_teardown(void);
 
 unsigned long stext_offset(void);
 
diff --git a/drivers/firmware/efi/riscv-runtime.c b/drivers/firmware/efi/riscv-runtime.c
index d0daacd2c903f182..09525fb5c240e668 100644
--- a/drivers/firmware/efi/riscv-runtime.c
+++ b/drivers/firmware/efi/riscv-runtime.c
@@ -130,14 +130,25 @@ static int __init riscv_enable_runtime_services(void)
 }
 early_initcall(riscv_enable_runtime_services);
 
-void efi_virtmap_load(void)
+static void efi_virtmap_load(void)
 {
 	preempt_disable();
 	switch_mm(current->active_mm, &efi_mm, NULL);
 }
 
-void efi_virtmap_unload(void)
+static void efi_virtmap_unload(void)
 {
 	switch_mm(&efi_mm, current->active_mm, NULL);
 	preempt_enable();
 }
+
+void arch_efi_call_virt_setup(void)
+{
+	sync_kernel_mappings(efi_mm.pgd);
+	efi_virtmap_load();
+}
+
+void arch_efi_call_virt_teardown(void)
+{
+	efi_virtmap_unload();
+}
-- 
2.39.2

[PATCH v2 04/11] efi/runtime-wrappers: Use type safe encapsulation of call arguments

[Ard Biesheuvel]

The current code that marshalls the EFI runtime call arguments to hand
them off to a async helper does so in a type unsafe and slightly messy
manner - everything is cast to void* except for some integral types that
are passed by reference and dereferenced on the receiver end.

Let's clean this up a bit, and record the arguments of each runtime
service invocation exactly as they are issued, in a manner that permits
the compiler to check the types of the arguments at both ends.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 196 +++++++++++++-------
 include/linux/efi.h                     |  18 +-
 2 files changed, 138 insertions(+), 76 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index a400c4312c829f18..66066e058757c1b5 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -44,20 +44,90 @@
 #define __efi_call_virt(f, args...) \
 	__efi_call_virt_pointer(efi.runtime, f, args)
 
+union efi_rts_args {
+	struct {
+		efi_time_t 	*time;
+		efi_time_cap_t	*capabilities;
+	} GET_TIME;
+
+	struct {
+		efi_time_t	*time;
+	} SET_TIME;
+
+	struct {
+		efi_bool_t	*enabled;
+		efi_bool_t	*pending;
+		efi_time_t	*time;
+	} GET_WAKEUP_TIME;
+
+	struct {
+		efi_bool_t	enable;
+		efi_time_t	*time;
+	} SET_WAKEUP_TIME;
+
+	struct {
+		efi_char16_t	*name;
+		efi_guid_t	*vendor;
+		u32		*attr;
+		unsigned long	*data_size;
+		void		*data;
+	} GET_VARIABLE;
+
+	struct {
+		unsigned long	*name_size;
+		efi_char16_t	*name;
+		efi_guid_t 	*vendor;
+	} GET_NEXT_VARIABLE;
+
+	struct {
+		efi_char16_t	*name;
+		efi_guid_t	*vendor;
+		u32		attr;
+		unsigned long	data_size;
+		void		*data;
+	} SET_VARIABLE;
+
+	struct {
+		u32		attr;
+		u64		*storage_space;
+		u64		*remaining_space;
+		u64		*max_variable_size;
+	} QUERY_VARIABLE_INFO;
+
+	struct {
+		u32		*high_count;
+	} GET_NEXT_HIGH_MONO_COUNT;
+
+	struct {
+		efi_capsule_header_t **capsules;
+		unsigned long	count;
+		unsigned long	sg_list;
+	} UPDATE_CAPSULE;
+
+	struct {
+		efi_capsule_header_t **capsules;
+		unsigned long	count;
+		u64		*max_size;
+		int		*reset_type;
+	} QUERY_CAPSULE_CAPS;
+};
+
 struct efi_runtime_work efi_rts_work;
 
 /*
- * efi_queue_work:	Queue efi_runtime_service() and wait until it's done
- * @rts:		efi_runtime_service() function identifier
- * @rts_arg<1-5>:	efi_runtime_service() function arguments
+ * efi_queue_work:	Queue EFI runtime service call and wait for completion
+ * @_rts:		EFI runtime service function identifier
+ * @_args:		Arguments to pass to the EFI runtime service
  *
  * Accesses to efi_runtime_services() are serialized by a binary
  * semaphore (efi_runtime_lock) and caller waits until the work is
  * finished, hence _only_ one work is queued at a time and the caller
  * thread waits for completion.
  */
-#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)		\
+#define efi_queue_work(_rts, _args...)					\
 ({									\
+	efi_rts_work.efi_rts_id = EFI_ ## _rts;				\
+	efi_rts_work.args = &(union efi_rts_args){ ._rts = { _args }};	\
 	efi_rts_work.status = EFI_ABORTED;				\
 									\
 	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {			\
@@ -68,12 +138,6 @@ struct efi_runtime_work efi_rts_work;
 									\
 	init_completion(&efi_rts_work.efi_rts_comp);			\
 	INIT_WORK(&efi_rts_work.work, efi_call_rts);			\
-	efi_rts_work.arg1 = _arg1;					\
-	efi_rts_work.arg2 = _arg2;					\
-	efi_rts_work.arg3 = _arg3;					\
-	efi_rts_work.arg4 = _arg4;					\
-	efi_rts_work.arg5 = _arg5;					\
-	efi_rts_work.efi_rts_id = _rts;					\
 									\
 	/*								\
 	 * queue_work() returns 0 if work was already on queue,         \
@@ -170,73 +234,78 @@ extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock);
 /*
  * Calls the appropriate efi_runtime_service() with the appropriate
  * arguments.
- *
- * Semantics followed by efi_call_rts() to understand efi_runtime_work:
- * 1. If argument was a pointer, recast it from void pointer to original
- * pointer type.
- * 2. If argument was a value, recast it from void pointer to original
- * pointer type and dereference it.
  */
 static void efi_call_rts(struct work_struct *work)
 {
-	void *arg1, *arg2, *arg3, *arg4, *arg5;
+	const union efi_rts_args *args = efi_rts_work.args;
 	efi_status_t status = EFI_NOT_FOUND;
 
-	arg1 = efi_rts_work.arg1;
-	arg2 = efi_rts_work.arg2;
-	arg3 = efi_rts_work.arg3;
-	arg4 = efi_rts_work.arg4;
-	arg5 = efi_rts_work.arg5;
-
 	switch (efi_rts_work.efi_rts_id) {
 	case EFI_GET_TIME:
-		status = efi_call_virt(get_time, (efi_time_t *)arg1,
-				       (efi_time_cap_t *)arg2);
+		status = efi_call_virt(get_time,
+				       args->GET_TIME.time,
+				       args->GET_TIME.capabilities);
 		break;
 	case EFI_SET_TIME:
-		status = efi_call_virt(set_time, (efi_time_t *)arg1);
+		status = efi_call_virt(set_time,
+				       args->SET_TIME.time);
 		break;
 	case EFI_GET_WAKEUP_TIME:
-		status = efi_call_virt(get_wakeup_time, (efi_bool_t *)arg1,
-				       (efi_bool_t *)arg2, (efi_time_t *)arg3);
+		status = efi_call_virt(get_wakeup_time,
+				       args->GET_WAKEUP_TIME.enabled,
+				       args->GET_WAKEUP_TIME.pending,
+				       args->GET_WAKEUP_TIME.time);
 		break;
 	case EFI_SET_WAKEUP_TIME:
-		status = efi_call_virt(set_wakeup_time, *(efi_bool_t *)arg1,
-				       (efi_time_t *)arg2);
+		status = efi_call_virt(set_wakeup_time,
+				       args->SET_WAKEUP_TIME.enable,
+				       args->SET_WAKEUP_TIME.time);
 		break;
 	case EFI_GET_VARIABLE:
-		status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
-				       (efi_guid_t *)arg2, (u32 *)arg3,
-				       (unsigned long *)arg4, (void *)arg5);
+		status = efi_call_virt(get_variable,
+				       args->GET_VARIABLE.name,
+				       args->GET_VARIABLE.vendor,
+				       args->GET_VARIABLE.attr,
+				       args->GET_VARIABLE.data_size,
+				       args->GET_VARIABLE.data);
 		break;
 	case EFI_GET_NEXT_VARIABLE:
-		status = efi_call_virt(get_next_variable, (unsigned long *)arg1,
-				       (efi_char16_t *)arg2,
-				       (efi_guid_t *)arg3);
+		status = efi_call_virt(get_next_variable,
+				       args->GET_NEXT_VARIABLE.name_size,
+				       args->GET_NEXT_VARIABLE.name,
+				       args->GET_NEXT_VARIABLE.vendor);
 		break;
 	case EFI_SET_VARIABLE:
-		status = efi_call_virt(set_variable, (efi_char16_t *)arg1,
-				       (efi_guid_t *)arg2, *(u32 *)arg3,
-				       *(unsigned long *)arg4, (void *)arg5);
+		status = efi_call_virt(set_variable,
+				       args->SET_VARIABLE.name,
+				       args->SET_VARIABLE.vendor,
+				       args->SET_VARIABLE.attr,
+				       args->SET_VARIABLE.data_size,
+				       args->SET_VARIABLE.data);
 		break;
 	case EFI_QUERY_VARIABLE_INFO:
-		status = efi_call_virt(query_variable_info, *(u32 *)arg1,
-				       (u64 *)arg2, (u64 *)arg3, (u64 *)arg4);
+		status = efi_call_virt(query_variable_info,
+				       args->QUERY_VARIABLE_INFO.attr,
+				       args->QUERY_VARIABLE_INFO.storage_space,
+				       args->QUERY_VARIABLE_INFO.remaining_space,
+				       args->QUERY_VARIABLE_INFO.max_variable_size);
 		break;
 	case EFI_GET_NEXT_HIGH_MONO_COUNT:
-		status = efi_call_virt(get_next_high_mono_count, (u32 *)arg1);
+		status = efi_call_virt(get_next_high_mono_count,
+				       args->GET_NEXT_HIGH_MONO_COUNT.high_count);
 		break;
 	case EFI_UPDATE_CAPSULE:
 		status = efi_call_virt(update_capsule,
-				       (efi_capsule_header_t **)arg1,
-				       *(unsigned long *)arg2,
-				       *(unsigned long *)arg3);
+				       args->UPDATE_CAPSULE.capsules,
+				       args->UPDATE_CAPSULE.count,
+				       args->UPDATE_CAPSULE.sg_list);
 		break;
 	case EFI_QUERY_CAPSULE_CAPS:
 		status = efi_call_virt(query_capsule_caps,
-				       (efi_capsule_header_t **)arg1,
-				       *(unsigned long *)arg2, (u64 *)arg3,
-				       (int *)arg4);
+				       args->QUERY_CAPSULE_CAPS.capsules,
+				       args->QUERY_CAPSULE_CAPS.count,
+				       args->QUERY_CAPSULE_CAPS.max_size,
+				       args->QUERY_CAPSULE_CAPS.reset_type);
 		break;
 	default:
 		/*
@@ -256,7 +325,7 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_GET_TIME, tm, tc, NULL, NULL, NULL);
+	status = efi_queue_work(GET_TIME, tm, tc);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -267,7 +336,7 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm)
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_SET_TIME, tm, NULL, NULL, NULL, NULL);
+	status = efi_queue_work(SET_TIME, tm);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -280,8 +349,7 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_GET_WAKEUP_TIME, enabled, pending, tm, NULL,
-				NULL);
+	status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -292,8 +360,7 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_SET_WAKEUP_TIME, &enabled, tm, NULL, NULL,
-				NULL);
+	status = efi_queue_work(SET_WAKEUP_TIME, enabled, tm);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -308,7 +375,7 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_GET_VARIABLE, name, vendor, attr, data_size,
+	status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size,
 				data);
 	up(&efi_runtime_lock);
 	return status;
@@ -322,8 +389,7 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_GET_NEXT_VARIABLE, name_size, name, vendor,
-				NULL, NULL);
+	status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -338,7 +404,7 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_SET_VARIABLE, name, vendor, &attr, &data_size,
+	status = efi_queue_work(SET_VARIABLE, name, vendor, attr, data_size,
 				data);
 	up(&efi_runtime_lock);
 	return status;
@@ -373,8 +439,8 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_QUERY_VARIABLE_INFO, &attr, storage_space,
-				remaining_space, max_variable_size, NULL);
+	status = efi_queue_work(QUERY_VARIABLE_INFO, attr, storage_space,
+				remaining_space, max_variable_size);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -405,8 +471,7 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL,
-				NULL, NULL);
+	status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -437,8 +502,7 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_UPDATE_CAPSULE, capsules, &count, &sg_list,
-				NULL, NULL);
+	status = efi_queue_work(UPDATE_CAPSULE, capsules, count, sg_list);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -455,8 +519,8 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_queue_work(EFI_QUERY_CAPSULE_CAPS, capsules, &count,
-				max_size, reset_type, NULL);
+	status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, count,
+				max_size, reset_type);
 	up(&efi_runtime_lock);
 	return status;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ab088c662e88d07b..b8c446da608371d2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1265,23 +1265,21 @@ enum efi_rts_ids {
 	EFI_QUERY_CAPSULE_CAPS,
 };
 
+union efi_rts_args;
+
 /*
  * efi_runtime_work:	Details of EFI Runtime Service work
- * @arg<1-5>:		EFI Runtime Service function arguments
+ * @args:		Pointer to union describing the arguments
  * @status:		Status of executing EFI Runtime Service
  * @efi_rts_id:		EFI Runtime Service function identifier
  * @efi_rts_comp:	Struct used for handling completions
  */
 struct efi_runtime_work {
-	void *arg1;
-	void *arg2;
-	void *arg3;
-	void *arg4;
-	void *arg5;
-	efi_status_t status;
-	struct work_struct work;
-	enum efi_rts_ids efi_rts_id;
-	struct completion efi_rts_comp;
+	union efi_rts_args	*args;
+	efi_status_t		status;
+	struct work_struct	work;
+	enum efi_rts_ids	efi_rts_id;
+	struct completion	efi_rts_comp;
 };
 
 extern struct efi_runtime_work efi_rts_work;
-- 
2.39.2

[PATCH v2 05/11] efi/runtime-wrapper: Move workqueue manipulation out of line

[Ard Biesheuvel]

efi_queue_work() is a macro that implements the non-trivial manipulation
of the EFI runtime workqueue and completion data structure, most of
which is generic, and could be shared between all the users of the
macro. So move it out of the macro and into a new helper function.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 61 +++++++++++---------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 66066e058757c1b5..ee5c9a3e50604398 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -125,34 +125,8 @@ struct efi_runtime_work efi_rts_work;
  * thread waits for completion.
  */
 #define efi_queue_work(_rts, _args...)					\
-({									\
-	efi_rts_work.efi_rts_id = EFI_ ## _rts;				\
-	efi_rts_work.args = &(union efi_rts_args){ ._rts = { _args }};	\
-	efi_rts_work.status = EFI_ABORTED;				\
-									\
-	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {			\
-		pr_warn_once("EFI Runtime Services are disabled!\n");	\
-		efi_rts_work.status = EFI_DEVICE_ERROR;			\
-		goto exit;						\
-	}								\
-									\
-	init_completion(&efi_rts_work.efi_rts_comp);			\
-	INIT_WORK(&efi_rts_work.work, efi_call_rts);			\
-									\
-	/*								\
-	 * queue_work() returns 0 if work was already on queue,         \
-	 * _ideally_ this should never happen.                          \
-	 */								\
-	if (queue_work(efi_rts_wq, &efi_rts_work.work))			\
-		wait_for_completion(&efi_rts_work.efi_rts_comp);	\
-	else								\
-		pr_err("Failed to queue work to efi_rts_wq.\n");	\
-									\
-	WARN_ON_ONCE(efi_rts_work.status == EFI_ABORTED);		\
-exit:									\
-	efi_rts_work.efi_rts_id = EFI_NONE;				\
-	efi_rts_work.status;						\
-})
+	__efi_queue_work(EFI_ ## _rts,					\
+			 &(union efi_rts_args){ ._rts = { _args }})
 
 #ifndef arch_efi_save_flags
 #define arch_efi_save_flags(state_flags)	local_save_flags(state_flags)
@@ -319,6 +293,37 @@ static void efi_call_rts(struct work_struct *work)
 	complete(&efi_rts_work.efi_rts_comp);
 }
 
+static efi_status_t __efi_queue_work(enum efi_rts_ids id,
+				     union efi_rts_args *args)
+{
+	efi_rts_work.efi_rts_id = id;
+	efi_rts_work.args = args;
+	efi_rts_work.status = EFI_ABORTED;
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {
+		pr_warn_once("EFI Runtime Services are disabled!\n");
+		efi_rts_work.status = EFI_DEVICE_ERROR;
+		goto exit;
+	}
+
+	init_completion(&efi_rts_work.efi_rts_comp);
+	INIT_WORK(&efi_rts_work.work, efi_call_rts);
+
+	/*
+	 * queue_work() returns 0 if work was already on queue,
+	 * _ideally_ this should never happen.
+	 */
+	if (queue_work(efi_rts_wq, &efi_rts_work.work))
+		wait_for_completion(&efi_rts_work.efi_rts_comp);
+	else
+		pr_err("Failed to queue work to efi_rts_wq.\n");
+
+	WARN_ON_ONCE(efi_rts_work.status == EFI_ABORTED);
+exit:
+	efi_rts_work.efi_rts_id = EFI_NONE;
+	return efi_rts_work.status;
+}
+
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
 	efi_status_t status;
-- 
2.39.2

[PATCH v2 06/11] efi/runtime-wrappers: Remove duplicated macro for service returning void

[Ard Biesheuvel]

__efi_call_virt() exists as an alternative for efi_call_virt() for the
sole reason that ResetSystem() returns void, and so we cannot use a call
to it in the RHS of an assignment.

Given that there is only a single user, let's drop the macro, and expand
it into the caller. That way, the remaining macro can be tightened
somewhat in terms of type safety too.

Note that the use of typeof() on the runtime service invocation does not
result in an actual call being made, but it does require a few pointer
types to be fixed up and converted into the proper function pointer
prototypes.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/uv/bios.h          |  3 ++-
 drivers/acpi/prmt.c                     |  2 +-
 drivers/firmware/efi/runtime-wrappers.c |  9 +++++---
 include/linux/efi.h                     | 23 ++++----------------
 4 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 1b6455f881f90e9c..501409f1258a196d 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -115,7 +115,8 @@ struct uv_arch_type_entry {
 struct uv_systab {
 	char signature[4];	/* must be UV_SYSTAB_SIG */
 	u32 revision;		/* distinguish different firmware revs */
-	u64 function;		/* BIOS runtime callback function ptr */
+	u64 (__efiapi *function)(enum uv_bios_cmd, ...);
+				/* BIOS runtime callback function ptr */
 	u32 size;		/* systab size (starting with _VERSION_UV4) */
 	struct {
 		u32 type:8;	/* type of entry */
diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
index 3d4c4620f9f95309..71b9adaaf33b93cf 100644
--- a/drivers/acpi/prmt.c
+++ b/drivers/acpi/prmt.c
@@ -53,7 +53,7 @@ static LIST_HEAD(prm_module_list);
 
 struct prm_handler_info {
 	guid_t guid;
-	void *handler_addr;
+	efi_status_t (__efiapi *handler_addr)(u64, void *);
 	u64 static_data_buffer_addr;
 	u64 acpi_param_buffer_addr;
 
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index ee5c9a3e50604398..c5e0c73cc000ed25 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -41,8 +41,6 @@
  */
 #define efi_call_virt(f, args...)   \
 	efi_call_virt_pointer(efi.runtime, f, args)
-#define __efi_call_virt(f, args...) \
-	__efi_call_virt_pointer(efi.runtime, f, args)
 
 union efi_rts_args {
 	struct {
@@ -491,8 +489,13 @@ static void virt_efi_reset_system(int reset_type,
 			"could not get exclusive access to the firmware\n");
 		return;
 	}
+
+	arch_efi_call_virt_setup();
 	efi_rts_work.efi_rts_id = EFI_RESET_SYSTEM;
-	__efi_call_virt(reset_system, reset_type, status, data_size, data);
+	arch_efi_call_virt(efi.runtime, reset_system, reset_type, status,
+			   data_size, data);
+	arch_efi_call_virt_teardown();
+
 	up(&efi_runtime_lock);
 }
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index b8c446da608371d2..ebb9671ed15ee3cb 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1171,8 +1171,7 @@ static inline void efi_check_for_embedded_firmwares(void) { }
 #define arch_efi_call_virt(p, f, args...)	((p)->f(args))
 
 /*
- * Arch code can implement the following three template macros, avoiding
- * reptition for the void/non-void return cases of {__,}efi_call_virt():
+ * Arch code must implement the following three routines:
  *
  *  * arch_efi_call_virt_setup()
  *
@@ -1181,9 +1180,8 @@ static inline void efi_check_for_embedded_firmwares(void) { }
  *
  *  * arch_efi_call_virt()
  *
- *    Performs the call. The last expression in the macro must be the call
- *    itself, allowing the logic to be shared by the void and non-void
- *    cases.
+ *    Performs the call. This routine takes a variable number of arguments so
+ *    it must be implemented as a variadic preprocessor macro.
  *
  *  * arch_efi_call_virt_teardown()
  *
@@ -1192,7 +1190,7 @@ static inline void efi_check_for_embedded_firmwares(void) { }
 
 #define efi_call_virt_pointer(p, f, args...)				\
 ({									\
-	efi_status_t __s;						\
+	typeof((p)->f(args)) __s;					\
 	unsigned long __flags;						\
 									\
 	arch_efi_call_virt_setup();					\
@@ -1206,19 +1204,6 @@ static inline void efi_check_for_embedded_firmwares(void) { }
 	__s;								\
 })
 
-#define __efi_call_virt_pointer(p, f, args...)				\
-({									\
-	unsigned long __flags;						\
-									\
-	arch_efi_call_virt_setup();					\
-									\
-	__flags = efi_call_virt_save_flags();				\
-	arch_efi_call_virt(p, f, args);					\
-	efi_call_virt_check_flags(__flags, __stringify(f));		\
-									\
-	arch_efi_call_virt_teardown();					\
-})
-
 #define EFI_RANDOM_SEED_SIZE		32U // BLAKE2S_HASH_SIZE
 
 struct linux_efi_random_seed {
-- 
2.39.2

[PATCH v2 07/11] efi/runtime-wrappers: Don't duplicate setup/teardown code

[Ard Biesheuvel]

Avoid duplicating the EFI arch setup and teardown routine calls numerous
times in efi_call_rts(). Instead, expand the efi_call_virt_pointer()
macro into efi_call_rts(), taking the pre and post parts out of the
switch.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 26 ++++++++++++++------
 include/linux/efi.h                     |  6 +++--
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index c5e0c73cc000ed25..afe9248cc5bc61ba 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -40,7 +40,7 @@
  * code doesn't get too cluttered:
  */
 #define efi_call_virt(f, args...)   \
-	efi_call_virt_pointer(efi.runtime, f, args)
+	arch_efi_call_virt(efi.runtime, f, args)
 
 union efi_rts_args {
 	struct {
@@ -139,7 +139,7 @@ unsigned long efi_call_virt_save_flags(void)
 	return flags;
 }
 
-void efi_call_virt_check_flags(unsigned long flags, const char *call)
+void efi_call_virt_check_flags(unsigned long flags, const void *caller)
 {
 	unsigned long cur_flags, mismatch;
 
@@ -150,8 +150,8 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call)
 		return;
 
 	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
-	pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
-			   flags, cur_flags, call);
+	pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI call from %pS\n",
+			   flags, cur_flags, caller ?: __builtin_return_address(0));
 	arch_efi_restore_flags(flags);
 }
 
@@ -211,6 +211,10 @@ static void efi_call_rts(struct work_struct *work)
 {
 	const union efi_rts_args *args = efi_rts_work.args;
 	efi_status_t status = EFI_NOT_FOUND;
+	unsigned long flags;
+
+	arch_efi_call_virt_setup();
+	flags = efi_call_virt_save_flags();
 
 	switch (efi_rts_work.efi_rts_id) {
 	case EFI_GET_TIME:
@@ -287,6 +291,10 @@ static void efi_call_rts(struct work_struct *work)
 		 */
 		pr_err("Requested executing invalid EFI Runtime Service.\n");
 	}
+
+	efi_call_virt_check_flags(flags, efi_rts_work.caller);
+	arch_efi_call_virt_teardown();
+
 	efi_rts_work.status = status;
 	complete(&efi_rts_work.efi_rts_comp);
 }
@@ -296,6 +304,7 @@ static efi_status_t __efi_queue_work(enum efi_rts_ids id,
 {
 	efi_rts_work.efi_rts_id = id;
 	efi_rts_work.args = args;
+	efi_rts_work.caller = __builtin_return_address(0);
 	efi_rts_work.status = EFI_ABORTED;
 
 	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {
@@ -423,8 +432,8 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 	if (down_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
-	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
-			       data);
+	status = efi_call_virt_pointer(efi.runtime, set_variable, name, vendor,
+				       attr, data_size, data);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -462,8 +471,9 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 	if (down_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
-	status = efi_call_virt(query_variable_info, attr, storage_space,
-			       remaining_space, max_variable_size);
+	status = efi_call_virt_pointer(efi.runtime, query_variable_info, attr,
+				       storage_space, remaining_space,
+				       max_variable_size);
 	up(&efi_runtime_lock);
 	return status;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ebb9671ed15ee3cb..cf450b6fbfd20aa5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1130,7 +1130,7 @@ extern bool efi_runtime_disabled(void);
 static inline bool efi_runtime_disabled(void) { return true; }
 #endif
 
-extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
+extern void efi_call_virt_check_flags(unsigned long flags, const void *caller);
 extern unsigned long efi_call_virt_save_flags(void);
 
 enum efi_secureboot_mode {
@@ -1197,7 +1197,7 @@ static inline void efi_check_for_embedded_firmwares(void) { }
 									\
 	__flags = efi_call_virt_save_flags();				\
 	__s = arch_efi_call_virt(p, f, args);				\
-	efi_call_virt_check_flags(__flags, __stringify(f));		\
+	efi_call_virt_check_flags(__flags, NULL);			\
 									\
 	arch_efi_call_virt_teardown();					\
 									\
@@ -1258,6 +1258,7 @@ union efi_rts_args;
  * @status:		Status of executing EFI Runtime Service
  * @efi_rts_id:		EFI Runtime Service function identifier
  * @efi_rts_comp:	Struct used for handling completions
+ * @caller:		The caller of the runtime service
  */
 struct efi_runtime_work {
 	union efi_rts_args	*args;
@@ -1265,6 +1266,7 @@ struct efi_runtime_work {
 	struct work_struct	work;
 	enum efi_rts_ids	efi_rts_id;
 	struct completion	efi_rts_comp;
+	const void		*caller;
 };
 
 extern struct efi_runtime_work efi_rts_work;
-- 
2.39.2

[PATCH v2 08/11] acpi/prmt: Use EFI runtime sandbox to invoke PRM handlers

[Ard Biesheuvel]

Instead of bypassing the kernel's adaptation layer for performing EFI
runtime calls, wire up ACPI PRM handling into it. This means these calls
can no longer occur concurrently with EFI runtime calls, and will be
made from the EFI runtime workqueue. It also means any page faults
occurring during PRM handling will be identified correctly as
originating in firmware code.

While at it, give the function pointer struct member a more descriptive
name so it will stand out in diagnostic messages if any issues do occur.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/acpi/Kconfig                    |  2 +-
 drivers/acpi/prmt.c                     |  6 ++--
 drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++++++++++++++
 include/linux/efi.h                     |  5 +++
 4 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 00dd309b66828182..cee82b473dc50921 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -581,7 +581,7 @@ config ACPI_VIOT
 
 config ACPI_PRMT
 	bool "Platform Runtime Mechanism Support"
-	depends on EFI && (X86_64 || ARM64)
+	depends on EFI_RUNTIME_WRAPPERS && (X86_64 || ARM64)
 	default y
 	help
 	  Platform Runtime Mechanism (PRM) is a firmware interface exposing a
diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
index 71b9adaaf33b93cf..7020584096bfaaaf 100644
--- a/drivers/acpi/prmt.c
+++ b/drivers/acpi/prmt.c
@@ -260,9 +260,9 @@ static acpi_status acpi_platformrt_space_handler(u32 function,
 		context.static_data_buffer = handler->static_data_buffer_addr;
 		context.mmio_ranges = module->mmio_info;
 
-		status = efi_call_virt_pointer(handler, handler_addr,
-					       handler->acpi_param_buffer_addr,
-					       &context);
+		status = efi_call_acpi_prm_handler(handler->handler_addr,
+						   handler->acpi_param_buffer_addr,
+						   &context);
 		if (status == EFI_SUCCESS) {
 			buffer->prm_status = PRM_HANDLER_SUCCESS;
 		} else {
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index afe9248cc5bc61ba..71d3c70f0705e1b9 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -108,6 +108,12 @@ union efi_rts_args {
 		u64		*max_size;
 		int		*reset_type;
 	} QUERY_CAPSULE_CAPS;
+
+	struct {
+		efi_status_t	(__efiapi *acpi_prm_handler)(u64, void *);
+		u64		param_buffer_addr;
+		void		*context;
+	} ACPI_PRM_HANDLER;
 };
 
 struct efi_runtime_work efi_rts_work;
@@ -283,6 +289,14 @@ static void efi_call_rts(struct work_struct *work)
 				       args->QUERY_CAPSULE_CAPS.max_size,
 				       args->QUERY_CAPSULE_CAPS.reset_type);
 		break;
+	case EFI_ACPI_PRM_HANDLER:
+#ifdef CONFIG_ACPI_PRMT
+		status = arch_efi_call_virt(&args->ACPI_PRM_HANDLER,
+					    acpi_prm_handler,
+					    args->ACPI_PRM_HANDLER.param_buffer_addr,
+					    args->ACPI_PRM_HANDLER.context);
+		break;
+#endif
 	default:
 		/*
 		 * Ideally, we should never reach here because a caller of this
@@ -560,3 +574,21 @@ void efi_native_runtime_setup(void)
 	efi.update_capsule = virt_efi_update_capsule;
 	efi.query_capsule_caps = virt_efi_query_capsule_caps;
 }
+
+#ifdef CONFIG_ACPI_PRMT
+
+efi_status_t
+efi_call_acpi_prm_handler(efi_status_t (__efiapi *handler_addr)(u64, void *),
+			  u64 param_buffer_addr, void *context)
+{
+	efi_status_t status;
+
+	if (down_interruptible(&efi_runtime_lock))
+		return EFI_ABORTED;
+	status = efi_queue_work(ACPI_PRM_HANDLER, handler_addr,
+				param_buffer_addr, context);
+	up(&efi_runtime_lock);
+	return status;
+}
+
+#endif
diff --git a/include/linux/efi.h b/include/linux/efi.h
index cf450b6fbfd20aa5..15b94dad5091b406 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1229,6 +1229,10 @@ extern int efi_tpm_final_log_size;
 
 extern unsigned long rci2_table_phys;
 
+efi_status_t
+efi_call_acpi_prm_handler(efi_status_t (__efiapi *handler_addr)(u64, void *),
+			  u64 param_buffer_addr, void *context);
+
 /*
  * efi_runtime_service() function identifiers.
  * "NONE" is used by efi_recover_from_page_fault() to check if the page
@@ -1248,6 +1252,7 @@ enum efi_rts_ids {
 	EFI_RESET_SYSTEM,
 	EFI_UPDATE_CAPSULE,
 	EFI_QUERY_CAPSULE_CAPS,
+	EFI_ACPI_PRM_HANDLER,
 };
 
 union efi_rts_args;
-- 
2.39.2

[PATCH v2 09/11] efi/runtime-wrappers: Clean up white space and add __init annotation

[Ard Biesheuvel]

Some cosmetic changes as well as a missing __init annotation.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 41 +++++++++-----------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 71d3c70f0705e1b9..dfec5969dbaba417 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -437,9 +437,8 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 }
 
 static efi_status_t
-virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
-				  u32 attr, unsigned long data_size,
-				  void *data)
+virt_efi_set_variable_nb(efi_char16_t *name, efi_guid_t *vendor, u32 attr,
+			 unsigned long data_size, void *data)
 {
 	efi_status_t status;
 
@@ -472,10 +471,8 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 }
 
 static efi_status_t
-virt_efi_query_variable_info_nonblocking(u32 attr,
-					 u64 *storage_space,
-					 u64 *remaining_space,
-					 u64 *max_variable_size)
+virt_efi_query_variable_info_nb(u32 attr, u64 *storage_space,
+				u64 *remaining_space, u64 *max_variable_size)
 {
 	efi_status_t status;
 
@@ -557,22 +554,22 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 	return status;
 }
 
-void efi_native_runtime_setup(void)
+void __init efi_native_runtime_setup(void)
 {
-	efi.get_time = virt_efi_get_time;
-	efi.set_time = virt_efi_set_time;
-	efi.get_wakeup_time = virt_efi_get_wakeup_time;
-	efi.set_wakeup_time = virt_efi_set_wakeup_time;
-	efi.get_variable = virt_efi_get_variable;
-	efi.get_next_variable = virt_efi_get_next_variable;
-	efi.set_variable = virt_efi_set_variable;
-	efi.set_variable_nonblocking = virt_efi_set_variable_nonblocking;
-	efi.get_next_high_mono_count = virt_efi_get_next_high_mono_count;
-	efi.reset_system = virt_efi_reset_system;
-	efi.query_variable_info = virt_efi_query_variable_info;
-	efi.query_variable_info_nonblocking = virt_efi_query_variable_info_nonblocking;
-	efi.update_capsule = virt_efi_update_capsule;
-	efi.query_capsule_caps = virt_efi_query_capsule_caps;
+	efi.get_time			    = virt_efi_get_time;
+	efi.set_time			    = virt_efi_set_time;
+	efi.get_wakeup_time		    = virt_efi_get_wakeup_time;
+	efi.set_wakeup_time		    = virt_efi_set_wakeup_time;
+	efi.get_variable		    = virt_efi_get_variable;
+	efi.get_next_variable		    = virt_efi_get_next_variable;
+	efi.set_variable		    = virt_efi_set_variable;
+	efi.set_variable_nonblocking	    = virt_efi_set_variable_nb;
+	efi.get_next_high_mono_count	    = virt_efi_get_next_high_mono_count;
+	efi.reset_system 		    = virt_efi_reset_system;
+	efi.query_variable_info		    = virt_efi_query_variable_info;
+	efi.query_variable_info_nonblocking = virt_efi_query_variable_info_nb;
+	efi.update_capsule		    = virt_efi_update_capsule;
+	efi.query_capsule_caps		    = virt_efi_query_capsule_caps;
 }
 
 #ifdef CONFIG_ACPI_PRMT
-- 
2.39.2

[RFC PATCH v2 10/11] efi/x86: Realign EFI runtime stack

[Ard Biesheuvel]

Instruct the compiler to realign the stack to 16 bytes, as required by
the UEFI specification for x86_64. This will allow us to drop the asm
helper that converts between MS and SysV calling conventions in a future
patch.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/Makefile                       |  3 +++
 arch/x86/include/asm/efi.h              |  2 ++
 arch/x86/platform/efi/Makefile          |  2 ++
 arch/x86/platform/efi/efi_64.c          |  2 +-
 arch/x86/platform/uv/Makefile           |  1 +
 arch/x86/platform/uv/bios_uv.c          |  4 ++--
 drivers/firmware/efi/Makefile           |  1 +
 drivers/firmware/efi/runtime-wrappers.c | 17 ++++++++++-------
 8 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index fdc2e3abd6152f53..debc43fb9a2e6d36 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -36,10 +36,13 @@ export RETPOLINE_VDSO_CFLAGS
 ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
       cc_stack_align4 := -mpreferred-stack-boundary=2
       cc_stack_align8 := -mpreferred-stack-boundary=3
+      EFI_STACK_ALIGN := -mpreferred-stack-boundary=4
 else ifneq ($(call cc-option, -mstack-alignment=16),)
       cc_stack_align4 := -mstack-alignment=4
       cc_stack_align8 := -mstack-alignment=8
+      EFI_STACK_ALIGN := -mstack-alignment=16
 endif
+export EFI_STACK_ALIGN
 
 # How to compile the 16-bit code.  Note we always compile for -march=i386;
 # that way we can complain to the user if the CPU is insufficient.
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index c4555b269a1b2474..c47f2d49e6bc6a09 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -111,6 +111,8 @@ extern bool efi_disable_ibt_for_runtime;
 	ret;								\
 })
 
+#define __efi_realign_stack	__attribute__((force_align_arg_pointer))
+
 #ifdef CONFIG_KASAN
 /*
  * CONFIG_KASAN may redefine memset to __memset.  __memset function is present
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 543df9a1379d121c..684ac27cdf7cabfe 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -7,3 +7,5 @@ obj-$(CONFIG_EFI) 		+= memmap.o quirks.o efi.o efi_$(BITS).o \
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
 obj-$(CONFIG_EFI_FAKE_MEMMAP)	+= fake_mem.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)	+= runtime-map.o
+
+CFLAGS_efi_64.o			:= $(EFI_STACK_ALIGN)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 91d31ac422d6cde7..8033b4a338c4b431 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -846,7 +846,7 @@ void __init efi_thunk_runtime_setup(void)
 	efi.query_capsule_caps = efi_thunk_query_capsule_caps;
 }
 
-efi_status_t __init __no_sanitize_address
+efi_status_t __init __no_sanitize_address __efi_realign_stack
 efi_set_virtual_address_map(unsigned long memory_map_size,
 			    unsigned long descriptor_size,
 			    u32 descriptor_version,
diff --git a/arch/x86/platform/uv/Makefile b/arch/x86/platform/uv/Makefile
index 1441dda8edf76879..cb4807ba4ef50f02 100644
--- a/arch/x86/platform/uv/Makefile
+++ b/arch/x86/platform/uv/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_X86_UV)		+= bios_uv.o uv_irq.o uv_time.o uv_nmi.o
+CFLAGS_bios_uv.o		:= $(EFI_STACK_ALIGN)
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index bf31af3d32d69cbc..d6b69f363aa1f2a9 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -20,8 +20,8 @@ unsigned long uv_systab_phys __ro_after_init = EFI_INVALID_TABLE_ADDR;
 
 struct uv_systab *uv_systab;
 
-static s64 __uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3,
-			u64 a4, u64 a5)
+static s64 __efi_realign_stack
+__uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
 {
 	struct uv_systab *tab = uv_systab;
 	s64 ret;
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index e489fefd23dae0bc..db3f8c1fbc1d8a54 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -9,6 +9,7 @@
 # in efi_call_virt() will cause crash if this code instrumented.
 #
 KASAN_SANITIZE_runtime-wrappers.o	:= n
+CFLAGS_runtime-wrappers.o		:= $(EFI_STACK_ALIGN)
 
 obj-$(CONFIG_ACPI_BGRT) 		+= efi-bgrt.o
 obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o memattr.o tpm.o
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index dfec5969dbaba417..2db9e0b3c2842e50 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -35,6 +35,10 @@
 
 #include <asm/efi.h>
 
+#ifndef __efi_realign_stack
+#define __efi_realign_stack
+#endif
+
 /*
  * Wrap around the new efi_call_virt_generic() macros so that the
  * code doesn't get too cluttered:
@@ -213,7 +217,7 @@ extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock);
  * Calls the appropriate efi_runtime_service() with the appropriate
  * arguments.
  */
-static void efi_call_rts(struct work_struct *work)
+static void __efi_realign_stack efi_call_rts(struct work_struct *work)
 {
 	const union efi_rts_args *args = efi_rts_work.args;
 	efi_status_t status = EFI_NOT_FOUND;
@@ -436,7 +440,7 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 	return status;
 }
 
-static efi_status_t
+static efi_status_t __efi_realign_stack
 virt_efi_set_variable_nb(efi_char16_t *name, efi_guid_t *vendor, u32 attr,
 			 unsigned long data_size, void *data)
 {
@@ -470,7 +474,7 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 	return status;
 }
 
-static efi_status_t
+static efi_status_t __efi_realign_stack
 virt_efi_query_variable_info_nb(u32 attr, u64 *storage_space,
 				u64 *remaining_space, u64 *max_variable_size)
 {
@@ -500,10 +504,9 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 	return status;
 }
 
-static void virt_efi_reset_system(int reset_type,
-				  efi_status_t status,
-				  unsigned long data_size,
-				  efi_char16_t *data)
+static void __efi_realign_stack
+virt_efi_reset_system(int reset_type, efi_status_t status,
+		      unsigned long data_size, efi_char16_t *data)
 {
 	if (down_trylock(&efi_runtime_lock)) {
 		pr_warn("failed to invoke the reset_system() runtime service:\n"
-- 
2.39.2

[RFC PATCH v2 11/11] efi/x86: Rely on compiler to emit MS ABI calls

[Ard Biesheuvel]

When EFI support was added to the x86_64 port originally, GCC did not
implement support for emitting procedure calls using the MS calling
convention, which deviates from the SysV one used in Linux.

However, this support has been added a long time ago, and the EFI stub
(which runs in a different execution context) has already been updated
to simply rely on the __attribute__((ms_abi)) annotations of the EFI
function pointers, resulting in both GCC and Clang doing the right
thing, as long as the correct stack alignment is being used.

The EFI stub runs on the stack provided by the firmware, and maintains
16 byte alignment internally, but the core x86_64 code does not, so
explicit stack realignment is needed, and this has been dealt with in
a previous patch. So the asm wrapper has become redundant, and can be
dropped.

Note that one of the EFI runtime services returns void so a trick is
needed to make the compiler ignore the return value in that case, or an
error will be triggered.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h          | 11 +++-----
 arch/x86/platform/efi/Makefile      |  4 +--
 arch/x86/platform/efi/efi_stub_64.S | 27 --------------------
 3 files changed, 5 insertions(+), 37 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index c47f2d49e6bc6a09..f5ac4dbecafe7f37 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -94,19 +94,14 @@ static inline void efi_fpu_end(void)
 #else /* !CONFIG_X86_32 */
 #define EFI_X86_KERNEL_ALLOC_LIMIT		EFI_ALLOC_LIMIT
 
-extern asmlinkage u64 __efi_call(void *fp, ...);
-
 extern bool efi_disable_ibt_for_runtime;
 
-#define efi_call(...) ({						\
-	__efi_nargs_check(efi_call, 7, __VA_ARGS__);			\
-	__efi_call(__VA_ARGS__);					\
-})
-
 #undef arch_efi_call_virt
 #define arch_efi_call_virt(p, f, args...) ({				\
 	u64 ret, ibt = ibt_save(efi_disable_ibt_for_runtime);		\
-	ret = efi_call((void *)p->f, args);				\
+	ret = __builtin_choose_expr(					\
+		__builtin_types_compatible_p(typeof((p)->f(args)),void),\
+			((p)->f(args), EFI_ABORTED), (p)->f(args));	\
 	ibt_restore(ibt);						\
 	ret;								\
 })
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 684ac27cdf7cabfe..6dbd9533c4ce088c 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,8 +2,8 @@
 KASAN_SANITIZE := n
 GCOV_PROFILE := n
 
-obj-$(CONFIG_EFI) 		+= memmap.o quirks.o efi.o efi_$(BITS).o \
-				   efi_stub_$(BITS).o
+obj-$(CONFIG_EFI) 		+= memmap.o quirks.o efi.o efi_$(BITS).o
+obj-$(CONFIG_X86_32)		+= efi_stub_32.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
 obj-$(CONFIG_EFI_FAKE_MEMMAP)	+= fake_mem.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)	+= runtime-map.o
diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
deleted file mode 100644
index 2206b8bc47b8a757..0000000000000000
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ /dev/null
@@ -1,27 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Function calling ABI conversion from Linux to EFI for x86_64
- *
- * Copyright (C) 2007 Intel Corp
- *	Bibo Mao <bibo.mao@intel.com>
- *	Huang Ying <ying.huang@intel.com>
- */
-
-#include <linux/linkage.h>
-#include <asm/nospec-branch.h>
-
-SYM_FUNC_START(__efi_call)
-	pushq %rbp
-	movq %rsp, %rbp
-	and $~0xf, %rsp
-	mov 16(%rbp), %rax
-	subq $48, %rsp
-	mov %r9, 32(%rsp)
-	mov %rax, 40(%rsp)
-	mov %r8, %r9
-	mov %rcx, %r8
-	mov %rsi, %rcx
-	CALL_NOSPEC rdi
-	leave
-	RET
-SYM_FUNC_END(__efi_call)
-- 
2.39.2

Re: [PATCH v2 02/11] efi/arm64: Move EFI runtime call setup/teardown helpers out of line

[Catalin Marinas]

On Fri, Aug 18, 2023 at 01:37:15PM +0200, Ard Biesheuvel wrote:
> Only the arch_efi_call_virt() macro that some architectures override
> needs to be a macro, given that it is variadic and encapsulates calls
> via function pointers that have different prototypes.
> 
> The associated setup and teardown code are not special in this regard,
> and don't need to be instantiated at each call site. So turn them into
> ordinary C functions and move them out of line.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 00/11] efi: Clean up runtime wrapper and wire it up for PRM

[Nathan Chancellor]

Hi Ard,

On Fri, Aug 18, 2023 at 01:37:13PM +0200, Ard Biesheuvel wrote:

<snip>

> Ard Biesheuvel (11):
>   efi/x86: Move EFI runtime call setup/teardown helpers out of line
>   efi/arm64: Move EFI runtime call setup/teardown helpers out of line
>   efi/riscv: Move EFI runtime call setup/teardown helpers out of line
>   efi/runtime-wrappers: Use type safe encapsulation of call arguments
>   efi/runtime-wrapper: Move workqueue manipulation out of line
>   efi/runtime-wrappers: Remove duplicated macro for service returning
>     void
>   efi/runtime-wrappers: Don't duplicate setup/teardown code
>   acpi/prmt: Use EFI runtime sandbox to invoke PRM handlers
>   efi/runtime-wrappers: Clean up white space and add __init annotation
>   efi/x86: Realign EFI runtime stack
>   efi/x86: Rely on compiler to emit MS ABI calls

I took this series for a spin on my arm64 and x86_64 systems that boot
under EFI. I noticed two issues for x86_64, none for arm64. I was hoping
to have a little more information by this point but I had some personal
stuff to deal with today but I figured I would report this initially
and if you want to continue on the ClangBuiltLinux issue tracker, we
certainly can.

1. I see some kCFI failures with this series on x86_64. The fact that
the target is not a symbol makes me think that a function defined in a
.S file is being called indirectly? The following stacktrace is repeated
over and over on all my machines.

  [ 3520.654794] CFI failure at efi_call_rts+0x314/0x3f0 (target: 0xfffffffeee9db7a4; expected type: 0x747b9986)
  [ 3520.654797] WARNING: CPU: 5 PID: 1870 at efi_call_rts+0x314/0x3f0
  [ 3520.654798] Modules linked in: ...
  [ 3520.654828] CPU: 5 PID: 1870 Comm: kworker/u32:0 Tainted: P                   6.5.0-rc6-next-20230818-debug-11225-g78f833901b02 #1 ab5bcd9a29b4227ebb5977a738a805cb7c6c7fff
  [ 3520.654829] Hardware name: ASUS System Product Name/PRIME Z590M-PLUS, BIOS 1203 10/27/2021
  [ 3520.654829] Workqueue: efi_rts_wq efi_call_rts
  [ 3520.654830] RIP: 0010:efi_call_rts+0x314/0x3f0
  [ 3520.654831] Code: 4e 01 4c 8b 58 48 49 8b 0e 49 8b 56 08 4d 8b 46 10 4d 8b 4e 18 49 8b 46 20 48 89 44 24 20 41 ba 7a 66 84 8b 45 03 53 f1 74 02 <0f> 0b 41 ff d3 0f 1f 00 eb 78 0f b6 3d 13 c3 d5 00 e8 36 6f 54 ff
  [ 3520.654832] RSP: 0018:ffffc90004e37e00 EFLAGS: 00010296
  [ 3520.654832] RAX: ffff88814a618004 RBX: 0000000000000206 RCX: ffff8881017da000
  [ 3520.654833] RDX: ffff8881017da400 RSI: ffffffff94d865c0 RDI: 0000000000000000
  [ 3520.654833] RBP: ffffc90004e37e48 R08: ffffc90004eafe04 R09: ffffc90004eafe08
  [ 3520.654834] R10: 000000008b846759 R11: fffffffeee9db7a4 R12: ffffffff95679990
  [ 3520.654834] R13: ffff888100059000 R14: ffffc90004eafd70 R15: ffff8881024c7800
  [ 3520.654835] FS:  0000000000000000(0000) GS:ffff88883f540000(0000) knlGS:0000000000000000
  [ 3520.654835] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 3520.654836] CR2: 00007f3f5814e008 CR3: 000000010020c001 CR4: 0000000000770ee0
  [ 3520.654836] PKRU: 55555554
  [ 3520.654837] Call Trace:
  [ 3520.654837]  <TASK>
  [ 3520.654837]  ? __warn+0xc8/0x1c0
  [ 3520.654838]  ? efi_call_rts+0x314/0x3f0
  [ 3520.654839]  ? report_cfi_failure+0x4e/0x60
  [ 3520.654841]  ? handle_cfi_failure+0x14c/0x1e0
  [ 3520.654842]  ? handle_bug+0x4f/0x90
  [ 3520.654843]  ? exc_invalid_op+0x1a/0x60
  [ 3520.654844]  ? asm_exc_invalid_op+0x1a/0x20
  [ 3520.654845]  ? efi_call_rts+0x314/0x3f0
  [ 3520.654847]  process_scheduled_works+0x25f/0x470
  [ 3520.654848]  worker_thread+0x21c/0x2e0
  [ 3520.654849]  ? __cfi_worker_thread+0x10/0x10
  [ 3520.654851]  kthread+0xf6/0x110
  [ 3520.654852]  ? __cfi_kthread+0x10/0x10
  [ 3520.654853]  ret_from_fork+0x45/0x60
  [ 3520.654854]  ? __cfi_kthread+0x10/0x10
  [ 3520.654856]  ret_from_fork_asm+0x1b/0x30
  [ 3520.654858]  </TASK>
  [ 3520.654858] ---[ end trace 0000000000000000 ]---

2. I see a link failure when building with LTO, presumably due to the
final two patches of the series. I see it with my distribution's
configuration [1] and allmodconfig with CONFIG_LTO_CLANG_THIN=y, I did
not try full LTO. Unfortunately, the message does not seem to make it
obvious what is going on here, I will try to debug this more unless you
beat me to it.

  ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi.o at 1059858)' and 'vmlinux.a(efi_64.o at 1059918)'
  ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi_64.o at 1059918)' and 'vmlinux.a(efi.o at 1059858)'
  ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(runtime-wrappers.o at 1181838)' and 'vmlinux.a(quirks.o at 1059798)'
  ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi.o at 1181178)' and 'vmlinux.a(runtime-wrappers.o at 1181838)'
  ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi_64.o at 1059918)' and 'vmlinux.a(setup.o at 1048218)'

[1]: https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config

Cheers,
Nathan

Re: [PATCH v2 00/11] efi: Clean up runtime wrapper and wire it up for PRM

[Ard Biesheuvel]

On Tue, 22 Aug 2023 at 04:01, Nathan Chancellor <nathan@kernel.org> wrote:
>
> Hi Ard,
>
> On Fri, Aug 18, 2023 at 01:37:13PM +0200, Ard Biesheuvel wrote:
>
> <snip>
>
> > Ard Biesheuvel (11):
> >   efi/x86: Move EFI runtime call setup/teardown helpers out of line
> >   efi/arm64: Move EFI runtime call setup/teardown helpers out of line
> >   efi/riscv: Move EFI runtime call setup/teardown helpers out of line
> >   efi/runtime-wrappers: Use type safe encapsulation of call arguments
> >   efi/runtime-wrapper: Move workqueue manipulation out of line
> >   efi/runtime-wrappers: Remove duplicated macro for service returning
> >     void
> >   efi/runtime-wrappers: Don't duplicate setup/teardown code
> >   acpi/prmt: Use EFI runtime sandbox to invoke PRM handlers
> >   efi/runtime-wrappers: Clean up white space and add __init annotation
> >   efi/x86: Realign EFI runtime stack
> >   efi/x86: Rely on compiler to emit MS ABI calls
>
> I took this series for a spin on my arm64 and x86_64 systems that boot
> under EFI. I noticed two issues for x86_64, none for arm64. I was hoping
> to have a little more information by this point but I had some personal
> stuff to deal with today but I figured I would report this initially
> and if you want to continue on the ClangBuiltLinux issue tracker, we
> certainly can.
>
> 1. I see some kCFI failures with this series on x86_64. The fact that
> the target is not a symbol makes me think that a function defined in a
> .S file is being called indirectly? The following stacktrace is repeated
> over and over on all my machines.
>

This has to do with the indirect calls being made by the EFI code to
the firmware services, which are not part of the kernel build.

Before, those indirect calls were hidden from the compiler, as they
were made from assembler, but now they are generated by the compiler,
and so we have to inform it that those functions do not have kCFI
metadata.

The below should have fixed it, but I am getting lots of build errors
along the lines of

error: '__no_sanitize__' attribute only applies to functions,
Objective-C methods, and global variables

when I add this. Suggestions welcome on how to inform the compiler
that calls via those function pointers should have __nocfi semantics.

--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -51,7 +51,7 @@ typedef u64 efi_physical_addr_t;
 typedef void *efi_handle_t;

 #if defined(CONFIG_X86_64)
-#define __efiapi __attribute__((ms_abi))
+#define __efiapi __attribute__((ms_abi)) __nocfi
 #elif defined(CONFIG_X86_32)
 #define __efiapi __attribute__((regparm(0)))
 #else

>   [ 3520.654794] CFI failure at efi_call_rts+0x314/0x3f0 (target: 0xfffffffeee9db7a4; expected type: 0x747b9986)
>   [ 3520.654797] WARNING: CPU: 5 PID: 1870 at efi_call_rts+0x314/0x3f0
>   [ 3520.654798] Modules linked in: ...
>   [ 3520.654828] CPU: 5 PID: 1870 Comm: kworker/u32:0 Tainted: P                   6.5.0-rc6-next-20230818-debug-11225-g78f833901b02 #1 ab5bcd9a29b4227ebb5977a738a805cb7c6c7fff
>   [ 3520.654829] Hardware name: ASUS System Product Name/PRIME Z590M-PLUS, BIOS 1203 10/27/2021
>   [ 3520.654829] Workqueue: efi_rts_wq efi_call_rts
>   [ 3520.654830] RIP: 0010:efi_call_rts+0x314/0x3f0
>   [ 3520.654831] Code: 4e 01 4c 8b 58 48 49 8b 0e 49 8b 56 08 4d 8b 46 10 4d 8b 4e 18 49 8b 46 20 48 89 44 24 20 41 ba 7a 66 84 8b 45 03 53 f1 74 02 <0f> 0b 41 ff d3 0f 1f 00 eb 78 0f b6 3d 13 c3 d5 00 e8 36 6f 54 ff
>   [ 3520.654832] RSP: 0018:ffffc90004e37e00 EFLAGS: 00010296
>   [ 3520.654832] RAX: ffff88814a618004 RBX: 0000000000000206 RCX: ffff8881017da000
>   [ 3520.654833] RDX: ffff8881017da400 RSI: ffffffff94d865c0 RDI: 0000000000000000
>   [ 3520.654833] RBP: ffffc90004e37e48 R08: ffffc90004eafe04 R09: ffffc90004eafe08
>   [ 3520.654834] R10: 000000008b846759 R11: fffffffeee9db7a4 R12: ffffffff95679990
>   [ 3520.654834] R13: ffff888100059000 R14: ffffc90004eafd70 R15: ffff8881024c7800
>   [ 3520.654835] FS:  0000000000000000(0000) GS:ffff88883f540000(0000) knlGS:0000000000000000
>   [ 3520.654835] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   [ 3520.654836] CR2: 00007f3f5814e008 CR3: 000000010020c001 CR4: 0000000000770ee0
>   [ 3520.654836] PKRU: 55555554
>   [ 3520.654837] Call Trace:
>   [ 3520.654837]  <TASK>
>   [ 3520.654837]  ? __warn+0xc8/0x1c0
>   [ 3520.654838]  ? efi_call_rts+0x314/0x3f0
>   [ 3520.654839]  ? report_cfi_failure+0x4e/0x60
>   [ 3520.654841]  ? handle_cfi_failure+0x14c/0x1e0
>   [ 3520.654842]  ? handle_bug+0x4f/0x90
>   [ 3520.654843]  ? exc_invalid_op+0x1a/0x60
>   [ 3520.654844]  ? asm_exc_invalid_op+0x1a/0x20
>   [ 3520.654845]  ? efi_call_rts+0x314/0x3f0
>   [ 3520.654847]  process_scheduled_works+0x25f/0x470
>   [ 3520.654848]  worker_thread+0x21c/0x2e0
>   [ 3520.654849]  ? __cfi_worker_thread+0x10/0x10
>   [ 3520.654851]  kthread+0xf6/0x110
>   [ 3520.654852]  ? __cfi_kthread+0x10/0x10
>   [ 3520.654853]  ret_from_fork+0x45/0x60
>   [ 3520.654854]  ? __cfi_kthread+0x10/0x10
>   [ 3520.654856]  ret_from_fork_asm+0x1b/0x30
>   [ 3520.654858]  </TASK>
>   [ 3520.654858] ---[ end trace 0000000000000000 ]---
>
> 2. I see a link failure when building with LTO, presumably due to the
> final two patches of the series. I see it with my distribution's
> configuration [1] and allmodconfig with CONFIG_LTO_CLANG_THIN=y, I did
> not try full LTO. Unfortunately, the message does not seem to make it
> obvious what is going on here, I will try to debug this more unless you
> beat me to it.
>
>   ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi.o at 1059858)' and 'vmlinux.a(efi_64.o at 1059918)'
>   ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi_64.o at 1059918)' and 'vmlinux.a(efi.o at 1059858)'
>   ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(runtime-wrappers.o at 1181838)' and 'vmlinux.a(quirks.o at 1059798)'
>   ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi.o at 1181178)' and 'vmlinux.a(runtime-wrappers.o at 1181838)'
>   ld.lld: error: Function Import: link error: linking module flags 'override-stack-alignment': IDs have conflicting values in 'vmlinux.a(efi_64.o at 1059918)' and 'vmlinux.a(setup.o at 1048218)'
>
> [1]: https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config
>

OK, this is probably a nasty one. The x86 kernel uses 8 byte stack
alignment only, instead of the 16 bytes mandated by the calling
convention. The reason is that function calls made from inline asm
will misalign the stack without the compiler knowing about this, and
so the only way to deal with this is to simply use 8 byte alignment
throughout.

EFI runtime services require 16 byte stack alignment, and so the call
needs to originate from a compilation unit that a) uses 16 byte stack
alignment, and b) realigns the stack if its routines are invoked with
a stack that is misaligned. This ensures that the outgoing stack
alignment when actually entering the firmware is correct.

Apparently, LTO does not like it if you use different stack alignment
setting for different object files, which is understandable. I don't
see any way around this, so I will probably have to drop the last two
patches.

Re: [PATCH v2 00/11] efi: Clean up runtime wrapper and wire it up for PRM

[Ard Biesheuvel]

On Tue, 22 Aug 2023 at 09:53, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 22 Aug 2023 at 04:01, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > Hi Ard,
> >
> > On Fri, Aug 18, 2023 at 01:37:13PM +0200, Ard Biesheuvel wrote:
> >
> > <snip>
> >
> > > Ard Biesheuvel (11):
> > >   efi/x86: Move EFI runtime call setup/teardown helpers out of line
> > >   efi/arm64: Move EFI runtime call setup/teardown helpers out of line
> > >   efi/riscv: Move EFI runtime call setup/teardown helpers out of line
> > >   efi/runtime-wrappers: Use type safe encapsulation of call arguments
> > >   efi/runtime-wrapper: Move workqueue manipulation out of line
> > >   efi/runtime-wrappers: Remove duplicated macro for service returning
> > >     void
> > >   efi/runtime-wrappers: Don't duplicate setup/teardown code
> > >   acpi/prmt: Use EFI runtime sandbox to invoke PRM handlers
> > >   efi/runtime-wrappers: Clean up white space and add __init annotation
> > >   efi/x86: Realign EFI runtime stack
> > >   efi/x86: Rely on compiler to emit MS ABI calls
> >
> > I took this series for a spin on my arm64 and x86_64 systems that boot
> > under EFI. I noticed two issues for x86_64, none for arm64. I was hoping
> > to have a little more information by this point but I had some personal
> > stuff to deal with today but I figured I would report this initially
> > and if you want to continue on the ClangBuiltLinux issue tracker, we
> > certainly can.
> >
> > 1. I see some kCFI failures with this series on x86_64. The fact that
> > the target is not a symbol makes me think that a function defined in a
> > .S file is being called indirectly? The following stacktrace is repeated
> > over and over on all my machines.
> >
>
> This has to do with the indirect calls being made by the EFI code to
> the firmware services, which are not part of the kernel build.
>
> Before, those indirect calls were hidden from the compiler, as they
> were made from assembler, but now they are generated by the compiler,
> and so we have to inform it that those functions do not have kCFI
> metadata.
>
> The below should have fixed it, but I am getting lots of build errors
> along the lines of
>
> error: '__no_sanitize__' attribute only applies to functions,
> Objective-C methods, and global variables
>
> when I add this. Suggestions welcome on how to inform the compiler
> that calls via those function pointers should have __nocfi semantics.
>

OK, so it is a property of the caller, not the callee. So it is simply
a matter of marking every user of the EFI runtime services with
__nocfi. So basically, every routine marked __efi_realign_stack should
be marked __nocfi as well.