[PATCH V1 0/3] Use efi_rts_workqueue to invoke EFI Runtime Services

[PATCH V1 0/3] Use efi_rts_workqueue to invoke EFI Runtime Services

[Sai Praneeth Prakhya]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

This patch set is an outcome of the discussion at
https://lkml.org/lkml/2017/8/21/607

Presently, efi_runtime_services() are executed by firmware in process
context. To execute efi_runtime_service(), kernel switches the page
directory from swapper_pgd to efi_pgd. However, efi_pgd doesn't have any
user space mappings. A potential issue could be, for instance, an NMI
interrupt (like perf) trying to profile some user data while in efi_pgd.

A solution for this issue could be to use kthread to run
efi_runtime_service(). When a user/kernel thread requests to execute
efi_runtime_service(), kernel off-loads this work to kthread which in
turn uses efi_pgd. Anything that tries to touch user space addresses
while in kthread is terminally broken. This patch set adds support to
the efi subsystem to handle all calls to efi_runtime_services() using a
work queue (which in turn uses kthread).

Implementation summary:
-----------------------
1. When a user/kernel thread requests to execute efi_runtime_service(),
enqueue work to a work queue, efi_rts_workqueue.
2. The caller thread waits until the work is finished because it's
dependent on the return status of efi_runtime_service() and, in specific
cases, the arguments populated by efi_runtime_service(). Some
efi_runtime_services() takes a pointer to buffer as an argument and
fills up the buffer with requested data. For instance, efi_get_variable()
and efi_get_next_variable(). Hence, the caller process cannot just post
the work and get going, it has to wait for results from firmware.

Caveat: efi_rts_workqueue to run efi_runtime_services() shouldn't be used
while in atomic, because caller thread might sleep. Presently, pstore
code doesn't use efi_rts_workqueue.

Tested using LUV (Linux UEFI Validation) for x86_64 and x86_32. Builds
fine for arm and arm64. Will appreciate the effort if someone could test
the patches on ARM (although I was able to boot with LUV for ARM).
LUV: https://01.org/linux-uefi-validation

Thanks to Ricardo and Dan for initial reviews and suggestions. Please
feel free to pour in your comments and concerns.
Note: Patches are based on Linus's kernel v4.16-rc2

Sai Praneeth (3):
  x86/efi: Call efi_delete_dummy_variable() during efi subsystem    
    initialization
  efi: Introduce efi_rts_workqueue and necessary infrastructure to
    invoke     all efi_runtime_services()
  efi: Use efi_rts_workqueue to invoke EFI Runtime Services

 arch/x86/include/asm/efi.h              |   1 -
 arch/x86/platform/efi/efi.c             |   6 -
 drivers/firmware/efi/efi.c              |  18 +++
 drivers/firmware/efi/runtime-wrappers.c | 229 +++++++++++++++++++++++++++++---
 include/linux/efi.h                     |  26 ++++
 5 files changed, 253 insertions(+), 27 deletions(-)

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peter.zijlstra@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Dan Williams <dan.j.williams@intel.com>

-- 
2.1.4

[PATCH V1 1/3] x86/efi: Call efi_delete_dummy_variable() during efi subsystem initialization

[Sai Praneeth Prakhya]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Invoking efi_runtime_services() through efi_workqueue means all accesses
to efi_runtime_services() should be done after efi_rts_wq has been
created. efi_delete_dummy_variable() calls set_variable(), hence
efi_delete_dummy_variable() should be called after efi_rts_wq has been
created.

efi_delete_dummy_variable() is called from efi_enter_virtual_mode()
which is early in the boot phase (efi_rts_wq isn't created yet), so call
efi_delete_dummy_variable() later in the boot phase i.e. while
initializing efi subsystem. In the next patch, this is the place where
we create efi_rts_wq and all the efi_runtime_services() will be called
using efi_rts_wq.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peter.zijlstra@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/include/asm/efi.h  | 1 -
 arch/x86/platform/efi/efi.c | 6 ------
 drivers/firmware/efi/efi.c  | 7 +++++++
 include/linux/efi.h         | 3 +++
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 85f6ccb80b91..34b03440a80f 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -130,7 +130,6 @@ extern void __init efi_runtime_update_mappings(void);
 extern void __init efi_dump_pagetable(void);
 extern void __init efi_apply_memmap_quirks(void);
 extern int __init efi_reuse_config(u64 tables, int nr_tables);
-extern void efi_delete_dummy_variable(void);
 
 struct efi_setup_data {
 	u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 9061babfbc83..a3169d14583f 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -893,9 +893,6 @@ static void __init kexec_enter_virtual_mode(void)
 
 	if (efi_enabled(EFI_OLD_MEMMAP) && (__supported_pte_mask & _PAGE_NX))
 		runtime_code_page_mkexec();
-
-	/* clean DUMMY object */
-	efi_delete_dummy_variable();
 #endif
 }
 
@@ -1015,9 +1012,6 @@ static void __init __efi_enter_virtual_mode(void)
 	 * necessary relocation fixups for the new virtual addresses.
 	 */
 	efi_runtime_update_mappings();
-
-	/* clean DUMMY object */
-	efi_delete_dummy_variable();
 }
 
 void __init efi_enter_virtual_mode(void)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd42f66a7c85..ac5db5f8dbbf 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -33,6 +33,7 @@
 #include <linux/memblock.h>
 
 #include <asm/early_ioremap.h>
+#include <asm/efi.h>
 
 struct efi __read_mostly efi = {
 	.mps			= EFI_INVALID_TABLE_ADDR,
@@ -328,6 +329,12 @@ static int __init efisubsys_init(void)
 	if (!efi_enabled(EFI_BOOT))
 		return 0;
 
+	/*
+	 * Clean DUMMY object calls EFI Runtime Service, set_variable(), so
+	 * it should be invoked only after efi_rts_workqueue is ready.
+	 */
+	efi_delete_dummy_variable();
+
 	/* We register the efi directory at /sys/firmware/efi */
 	efi_kobj = kobject_create_and_add("efi", firmware_kobj);
 	if (!efi_kobj) {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f5083aa72eae..c4efb3ef0dfa 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -992,6 +992,7 @@ extern efi_status_t efi_query_variable_store(u32 attributes,
 					     unsigned long size,
 					     bool nonblocking);
 extern void efi_find_mirror(void);
+extern void efi_delete_dummy_variable(void);
 #else
 static inline void efi_late_init(void) {}
 static inline void efi_free_boot_services(void) {}
@@ -1002,6 +1003,8 @@ static inline efi_status_t efi_query_variable_store(u32 attributes,
 {
 	return EFI_SUCCESS;
 }
+
+static inline void efi_delete_dummy_variable(void) {}
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 
-- 
2.1.4

[PATCH V1 2/3] efi: Introduce efi_rts_workqueue and necessary infrastructure to invoke all efi_runtime_services()

[Sai Praneeth Prakhya]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

When a process requests the kernel to execute any efi_runtime_service(),
the requested efi_runtime_service (represented as an identifier) and its
arguments are packed into a struct named efi_runtime_work and queued
onto work queue named efi_rts_wq. The caller then waits until the work
is completed.

Introduce necessary infrastructure:
1. Creating workqueue named efi_rts_wq
2. A macro (efi_queue_work()) that
	a. populates efi_runtime_work
	b. queues work onto efi_rts_wq and
	c. waits until worker thread returns
3. A handler function that
	a. understands efi_runtime_work and
	b. invokes the appropriate efi_runtime_service() with the
	appropriate arguments

The caller thread has to wait until the worker thread returns, because
it's dependent on the return status of efi_runtime_service() and, in
specific cases, the arguments populated by efi_runtime_service(). Some
efi_runtime_services() takes a pointer to buffer as an argument and
fills up the buffer with requested data. For instance,
efi_get_variable() and efi_get_next_variable(). Hence, caller process
cannot just post the work and get going.

Some facts about efi_runtime_services():
1. A quick look at all the efi_runtime_services() shows that any
efi_runtime_service() has five or less arguments.
2. An argument of efi_runtime_service() can be a value (of any type)
or a pointer (of any type).
Hence, efi_runtime_work has five void pointers to store these 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.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peter.zijlstra@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/firmware/efi/efi.c              |  11 +++
 drivers/firmware/efi/runtime-wrappers.c | 143 ++++++++++++++++++++++++++++++++
 include/linux/efi.h                     |  23 +++++
 3 files changed, 177 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index ac5db5f8dbbf..4714b305ca90 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -76,6 +76,8 @@ static unsigned long *efi_tables[] = {
 	&efi.mem_attr_table,
 };
 
+struct workqueue_struct *efi_rts_wq;
+
 static bool disable_runtime;
 static int __init setup_noefi(char *arg)
 {
@@ -329,6 +331,15 @@ static int __init efisubsys_init(void)
 	if (!efi_enabled(EFI_BOOT))
 		return 0;
 
+	/* Create a work queue to run EFI Runtime Services */
+	efi_rts_wq = create_workqueue("efi_rts_workqueue");
+	if (!efi_rts_wq) {
+		pr_err("Failed to create efi_rts_workqueue, EFI runtime services "
+		       "disabled.\n");
+		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+		return 0;
+	}
+
 	/*
 	 * Clean DUMMY object calls EFI Runtime Service, set_variable(), so
 	 * it should be invoked only after efi_rts_workqueue is ready.
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index ae54870b2788..5cdb787da5d3 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -1,6 +1,14 @@
 /*
  * runtime-wrappers.c - Runtime Services function call wrappers
  *
+ * Implementation summary:
+ * -----------------------
+ * 1. When user/kernel thread requests to execute efi_runtime_service(),
+ * enqueue work to efi_rts_workqueue.
+ * 2. Caller thread waits until the work is finished because it's
+ * dependent on the return status and execution of efi_runtime_service().
+ * For instance, get_variable() and get_next_variable().
+ *
  * Copyright (C) 2014 Linaro Ltd. <ard.biesheuvel@linaro.org>
  *
  * Split off from arch/x86/platform/efi/efi.c
@@ -22,6 +30,8 @@
 #include <linux/mutex.h>
 #include <linux/semaphore.h>
 #include <linux/stringify.h>
+#include <linux/workqueue.h>
+
 #include <asm/efi.h>
 
 /*
@@ -33,6 +43,50 @@
 #define __efi_call_virt(f, args...) \
 	__efi_call_virt_pointer(efi.systab->runtime, f, args)
 
+/* Each EFI Runtime Service is represented with a unique number */
+#define GET_TIME					0
+#define SET_TIME					1
+#define GET_WAKEUP_TIME					2
+#define SET_WAKEUP_TIME					3
+#define GET_VARIABLE					4
+#define GET_NEXT_VARIABLE				5
+#define SET_VARIABLE					6
+#define SET_VARIABLE_NONBLOCKING			7
+#define QUERY_VARIABLE_INFO				8
+#define QUERY_VARIABLE_INFO_NONBLOCKING			9
+#define GET_NEXT_HIGH_MONO_COUNT			10
+#define RESET_SYSTEM					11
+#define UPDATE_CAPSULE					12
+#define QUERY_CAPSULE_CAPS				13
+
+/*
+ * 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
+ *
+ * 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. So, queue_work()
+ * should never fail.
+ */
+#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)		\
+({									\
+	struct efi_runtime_work efi_rts_work;				\
+	efi_rts_work.status = EFI_ABORTED;				\
+									\
+	INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts);		\
+	efi_rts_work.func = _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;					\
+	if (queue_work(efi_rts_wq, &efi_rts_work.work))			\
+		flush_work(&efi_rts_work.work);				\
+									\
+	efi_rts_work.status;						\
+})
+
 void efi_call_virt_check_flags(unsigned long flags, const char *call)
 {
 	unsigned long cur_flags, mismatch;
@@ -312,3 +366,92 @@ void efi_native_runtime_setup(void)
 	efi.update_capsule = virt_efi_update_capsule;
 	efi.query_capsule_caps = virt_efi_query_capsule_caps;
 }
+
+/*
+ * 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.
+ */
+void efi_call_rts(struct work_struct *work)
+{
+	struct efi_runtime_work *efi_rts_work;
+	void *arg1, *arg2, *arg3, *arg4, *arg5;
+	efi_status_t status = EFI_NOT_FOUND;
+
+	efi_rts_work = container_of(work, struct efi_runtime_work, work);
+	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->func) {
+	case GET_TIME:
+		status = efi_call_virt(get_time, (efi_time_t *)arg1,
+				       (efi_time_cap_t *)arg2);
+		break;
+	case SET_TIME:
+		status = efi_call_virt(set_time, (efi_time_t *)arg1);
+		break;
+	case GET_WAKEUP_TIME:
+		status = efi_call_virt(get_wakeup_time, (efi_bool_t *)arg1,
+				       (efi_bool_t *)arg2, (efi_time_t *)arg3);
+		break;
+	case SET_WAKEUP_TIME:
+		status = efi_call_virt(set_wakeup_time, *(efi_bool_t *)arg1,
+				       (efi_time_t *)arg2);
+		break;
+	case GET_VARIABLE:
+		status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
+				       (efi_guid_t *)arg2, (u32 *)arg3,
+				       (unsigned long *)arg4, (void *)arg5);
+		break;
+	case GET_NEXT_VARIABLE:
+		status = efi_call_virt(get_next_variable, (unsigned long *)arg1,
+				       (efi_char16_t *)arg2,
+				       (efi_guid_t *)arg3);
+		break;
+	case SET_VARIABLE:
+	case SET_VARIABLE_NONBLOCKING:
+		status = efi_call_virt(set_variable, (efi_char16_t *)arg1,
+				       (efi_guid_t *)arg2, *(u32 *)arg3,
+				       *(unsigned long *)arg4, (void *)arg5);
+		break;
+	case QUERY_VARIABLE_INFO:
+	case QUERY_VARIABLE_INFO_NONBLOCKING:
+		status = efi_call_virt(query_variable_info, *(u32 *)arg1,
+				       (u64 *)arg2, (u64 *)arg3, (u64 *)arg4);
+		break;
+	case GET_NEXT_HIGH_MONO_COUNT:
+		status = efi_call_virt(get_next_high_mono_count, (u32 *)arg1);
+		break;
+	case RESET_SYSTEM:
+		__efi_call_virt(reset_system, *(int *)arg1,
+				*(efi_status_t *)arg2,
+				*(unsigned long *)arg3,
+				(efi_char16_t *)arg4);
+		break;
+	case UPDATE_CAPSULE:
+		status = efi_call_virt(update_capsule,
+				       (efi_capsule_header_t **)arg1,
+				       *(unsigned long *)arg2,
+				       *(unsigned long *)arg3);
+		break;
+	case QUERY_CAPSULE_CAPS:
+		status = efi_call_virt(query_capsule_caps,
+				       (efi_capsule_header_t **)arg1,
+				       *(unsigned long *)arg2, (u64 *)arg3,
+				       (int *)arg4);
+		break;
+	default:
+		pr_err("Not a valid EFI_RT_SERVICE?");
+		status = EFI_NOT_FOUND;
+		break;
+	}
+	efi_rts_work->status = status;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c4efb3ef0dfa..4abb401d40f5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1652,4 +1652,27 @@ struct linux_efi_tpm_eventlog {
 
 extern int efi_tpm_eventlog_init(void);
 
+/*
+ * efi_runtime_work:	Details of EFI Runtime Service work
+ * @func:		EFI Runtime Service function identifier
+ * @arg<1-5>:		EFI Runtime Service function arguments
+ * @status:		Status of executing EFI Runtime Service
+ */
+struct efi_runtime_work {
+	u8 func;
+	void *arg1;
+	void *arg2;
+	void *arg3;
+	void *arg4;
+	void *arg5;
+	efi_status_t status;
+	struct work_struct work;
+};
+
+/* Workqueue to queue EFI Runtime Services */
+extern struct workqueue_struct *efi_rts_wq;
+
+/* Call back function that calls EFI Runtime Services */
+extern void efi_call_rts(struct work_struct *work);
+
 #endif /* _LINUX_EFI_H */
-- 
2.1.4

[PATCH V1 3/3] efi: Use efi_rts_workqueue to invoke EFI Runtime Services

[Sai Praneeth Prakhya]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Presently, efi_runtime_services() are executed by firmware in process
context. To execute efi_runtime_service(), kernel switches the page
directory from swapper_pgd to efi_pgd. However, efi_pgd doesn't have any
user space mappings. A potential issue could be, for instance, an NMI
interrupt (like perf) trying to profile some user data while in efi_pgd.

A solution for this issue could be to use kthread to run
efi_runtime_service(). When a user/kernel thread requests to execute
efi_runtime_service(), kernel off-loads this work to kthread which in
turn uses efi_pgd. Anything that tries to touch user space addresses
while in kthread is terminally broken. This patch adds support to efi
subsystem to handle all calls to efi_runtime_services() using a work
queue (which in turn uses kthread).

Implementation summary:
-----------------------
1. When user/kernel thread requests to execute efi_runtime_service(),
enqueue work to efi_rts_workqueue.
2. Caller thread waits until the work is finished because it's dependent
on the return status of efi_runtime_service().

pstore writes could potentially be invoked in interrupt context and it
uses set_variable<>() and query_variable_info<>() to store logs. If we
invoke efi_runtime_services() through efi_rts_wq while in atomic()
kernel issues a warning ("scheduling wile in atomic") and prints stack
trace. One way to overcome this is to not make the caller process wait
for the worker thread to finish. This approach breaks pstore i.e. the
log messages aren't written to efi variables. Hence, pstore calls
efi_runtime_services() without using efi_rts_wq or in other words
efi_rts_wq will be used unconditionally for all the
efi_runtime_services() except set_variable<>() and
query_variable_info<>()

Semantics to pack arguments in efi_runtime_work (has void pointers):
1. If argument is a pointer (of any type), pass it as is.
2. If argument is a value (of any type), address of the value is
passed.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peter.zijlstra@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/firmware/efi/runtime-wrappers.c | 86 +++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 5cdb787da5d3..531d077aac70 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -68,6 +68,16 @@
  * semaphore (efi_runtime_lock) and caller waits until the work is
  * finished, hence _only_ one work is queued at a time. So, queue_work()
  * should never fail.
+ *
+ * efi_rts_workqueue to run efi_runtime_services() shouldn't be used
+ * while in atomic, because caller thread might sleep. pstore writes
+ * could potentially be invoked in interrupt context and it uses
+ * set_variable<>() and query_variable_info<>(), so pstore code doesn't
+ * use efi_rts_workqueue.
+ *
+ * Semantics that caller function should follow while passing arguments:
+ * 1. If argument is a pointer (of any type), pass it as is.
+ * 2. If argument is a value (of any type), address of the value is passed.
  */
 #define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)		\
 ({									\
@@ -150,7 +160,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_call_virt(get_time, tm, tc);
+	status = efi_queue_work(GET_TIME, tm, tc, NULL, NULL, NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -161,7 +171,7 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm)
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(set_time, tm);
+	status = efi_queue_work(SET_TIME, tm, NULL, NULL, NULL, NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -174,7 +184,8 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
+	status = efi_queue_work(GET_WAKEUP_TIME, enabled, pending, tm, NULL,
+				NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -185,7 +196,8 @@ 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_call_virt(set_wakeup_time, enabled, tm);
+	status = efi_queue_work(SET_WAKEUP_TIME, &enabled, tm, NULL, NULL,
+				NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -200,8 +212,8 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
-			       data);
+	status = efi_queue_work(GET_VARIABLE, name, vendor, attr, data_size,
+				data);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -214,7 +226,8 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(get_next_variable, name_size, name, vendor);
+	status = efi_queue_work(GET_NEXT_VARIABLE, name_size, name, vendor,
+				NULL, NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -229,8 +242,15 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
-			       data);
+
+	/* pstore shouldn't use efi_rts_wq while in atomic */
+	if (!in_atomic())
+		status = efi_queue_work(SET_VARIABLE, name, vendor, &attr,
+					&data_size, data);
+	else
+		status = efi_call_virt(set_variable, name, vendor, attr,
+				       data_size, data);
+
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -245,8 +265,14 @@ 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);
+	/* pstore shouldn't use efi_rts_wq while in atomic */
+	if (!in_atomic())
+		status = efi_queue_work(SET_VARIABLE_NONBLOCKING, &name, vendor,
+					&attr,	&data_size, data);
+	else
+		status = efi_call_virt(set_variable, name, vendor, attr,
+				       data_size, data);
+
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -264,8 +290,17 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(query_variable_info, attr, storage_space,
-			       remaining_space, max_variable_size);
+
+	/* pstore shouldn't use efi_rts_wq while in atomic */
+	if (!in_atomic())
+		status = efi_queue_work(QUERY_VARIABLE_INFO, &attr,
+					storage_space, remaining_space,
+					max_variable_size, NULL);
+	else
+		status = efi_call_virt(query_variable_info, attr,
+				       storage_space, remaining_space,
+				       max_variable_size);
+
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -284,8 +319,16 @@ 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);
+	/* pstore shouldn't use efi_rts_wq while in atomic */
+	if (!in_atomic())
+		status = efi_queue_work(QUERY_VARIABLE_INFO_NONBLOCKING, &attr,
+					storage_space, remaining_space,
+					max_variable_size, NULL);
+	else
+		status = efi_call_virt(query_variable_info, attr,
+				       storage_space, remaining_space,
+				       max_variable_size);
+
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -296,7 +339,8 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(get_next_high_mono_count, count);
+	status = efi_queue_work(GET_NEXT_HIGH_MONO_COUNT, count, NULL, NULL,
+				NULL, NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -311,7 +355,8 @@ static void virt_efi_reset_system(int reset_type,
 			"could not get exclusive access to the firmware\n");
 		return;
 	}
-	__efi_call_virt(reset_system, reset_type, status, data_size, data);
+	efi_queue_work(RESET_SYSTEM, &reset_type, &status, &data_size, data,
+		       NULL);
 	up(&efi_runtime_lock);
 }
 
@@ -326,7 +371,8 @@ static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 
 	if (down_interruptible(&efi_runtime_lock))
 		return EFI_ABORTED;
-	status = efi_call_virt(update_capsule, capsules, count, sg_list);
+	status = efi_queue_work(UPDATE_CAPSULE, capsules, &count, &sg_list,
+				NULL, NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
@@ -343,8 +389,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_call_virt(query_capsule_caps, capsules, count, max_size,
-			       reset_type);
+	status = efi_queue_work(QUERY_CAPSULE_CAPS, capsules, &count,
+				max_size, reset_type, NULL);
 	up(&efi_runtime_lock);
 	return status;
 }
-- 
2.1.4

Re: [PATCH V1 2/3] efi: Introduce efi_rts_workqueue and necessary infrastructure to invoke all efi_runtime_services()

[Miguel Ojeda]

On Sat, Feb 24, 2018 at 11:10 PM, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.com> wrote:
> From: Sai Praneeth <sai.praneeth.prakhya@intel.com>
>
> When a process requests the kernel to execute any efi_runtime_service(),
> the requested efi_runtime_service (represented as an identifier) and its
> arguments are packed into a struct named efi_runtime_work and queued
> onto work queue named efi_rts_wq. The caller then waits until the work
> is completed.
>
> Introduce necessary infrastructure:
> 1. Creating workqueue named efi_rts_wq
> 2. A macro (efi_queue_work()) that
>         a. populates efi_runtime_work
>         b. queues work onto efi_rts_wq and
>         c. waits until worker thread returns
> 3. A handler function that
>         a. understands efi_runtime_work and
>         b. invokes the appropriate efi_runtime_service() with the
>         appropriate arguments
>
> The caller thread has to wait until the worker thread returns, because
> it's dependent on the return status of efi_runtime_service() and, in
> specific cases, the arguments populated by efi_runtime_service(). Some
> efi_runtime_services() takes a pointer to buffer as an argument and
> fills up the buffer with requested data. For instance,
> efi_get_variable() and efi_get_next_variable(). Hence, caller process
> cannot just post the work and get going.
>
> Some facts about efi_runtime_services():
> 1. A quick look at all the efi_runtime_services() shows that any
> efi_runtime_service() has five or less arguments.
> 2. An argument of efi_runtime_service() can be a value (of any type)
> or a pointer (of any type).
> Hence, efi_runtime_work has five void pointers to store these 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.
>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Cc: Lee, Chun-Yi <jlee@suse.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Bhupesh Sharma <bhsharma@redhat.com>
> Cc: Ricardo Neri <ricardo.neri@intel.com>
> Cc: Ravi Shankar <ravi.v.shankar@intel.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Peter Zijlstra <peter.zijlstra@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/firmware/efi/efi.c              |  11 +++
>  drivers/firmware/efi/runtime-wrappers.c | 143 ++++++++++++++++++++++++++++++++
>  include/linux/efi.h                     |  23 +++++
>  3 files changed, 177 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index ac5db5f8dbbf..4714b305ca90 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -76,6 +76,8 @@ static unsigned long *efi_tables[] = {
>         &efi.mem_attr_table,
>  };
>
> +struct workqueue_struct *efi_rts_wq;
> +
>  static bool disable_runtime;
>  static int __init setup_noefi(char *arg)
>  {
> @@ -329,6 +331,15 @@ static int __init efisubsys_init(void)
>         if (!efi_enabled(EFI_BOOT))
>                 return 0;
>
> +       /* Create a work queue to run EFI Runtime Services */
> +       efi_rts_wq = create_workqueue("efi_rts_workqueue");

Please consider alloc_workqueue() instead with the appropriate flags,
since create_workqueue() and friends are deprecated.

> +       if (!efi_rts_wq) {
> +               pr_err("Failed to create efi_rts_workqueue, EFI runtime services "
> +                      "disabled.\n");
> +               clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +               return 0;
> +       }
> +
>         /*
>          * Clean DUMMY object calls EFI Runtime Service, set_variable(), so
>          * it should be invoked only after efi_rts_workqueue is ready.
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index ae54870b2788..5cdb787da5d3 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -1,6 +1,14 @@
>  /*
>   * runtime-wrappers.c - Runtime Services function call wrappers
>   *
> + * Implementation summary:
> + * -----------------------
> + * 1. When user/kernel thread requests to execute efi_runtime_service(),
> + * enqueue work to efi_rts_workqueue.
> + * 2. Caller thread waits until the work is finished because it's
> + * dependent on the return status and execution of efi_runtime_service().
> + * For instance, get_variable() and get_next_variable().
> + *
>   * Copyright (C) 2014 Linaro Ltd. <ard.biesheuvel@linaro.org>
>   *
>   * Split off from arch/x86/platform/efi/efi.c
> @@ -22,6 +30,8 @@
>  #include <linux/mutex.h>
>  #include <linux/semaphore.h>
>  #include <linux/stringify.h>
> +#include <linux/workqueue.h>
> +
>  #include <asm/efi.h>
>
>  /*
> @@ -33,6 +43,50 @@
>  #define __efi_call_virt(f, args...) \
>         __efi_call_virt_pointer(efi.systab->runtime, f, args)
>
> +/* Each EFI Runtime Service is represented with a unique number */
> +#define GET_TIME                                       0
> +#define SET_TIME                                       1
> +#define GET_WAKEUP_TIME                                        2
> +#define SET_WAKEUP_TIME                                        3
> +#define GET_VARIABLE                                   4
> +#define GET_NEXT_VARIABLE                              5
> +#define SET_VARIABLE                                   6
> +#define SET_VARIABLE_NONBLOCKING                       7
> +#define QUERY_VARIABLE_INFO                            8
> +#define QUERY_VARIABLE_INFO_NONBLOCKING                        9
> +#define GET_NEXT_HIGH_MONO_COUNT                       10
> +#define RESET_SYSTEM                                   11
> +#define UPDATE_CAPSULE                                 12
> +#define QUERY_CAPSULE_CAPS                             13

An enum would be better, given these are just internal, contiguous IDs.

> +
> +/*
> + * 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
> + *
> + * 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. So, queue_work()
> + * should never fail.
> + */
> +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)                \
> +({                                                                     \
> +       struct efi_runtime_work efi_rts_work;                           \
> +       efi_rts_work.status = EFI_ABORTED;                              \
> +                                                                       \
> +       INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts);            \
> +       efi_rts_work.func = _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;                                      \
> +       if (queue_work(efi_rts_wq, &efi_rts_work.work))                 \
> +               flush_work(&efi_rts_work.work);                         \

If "So, queue_work() should never fail.", shouldn't this be a BUG() or similar?

> +                                                                       \
> +       efi_rts_work.status;                                            \
> +})
> +
>  void efi_call_virt_check_flags(unsigned long flags, const char *call)
>  {
>         unsigned long cur_flags, mismatch;
> @@ -312,3 +366,92 @@ void efi_native_runtime_setup(void)
>         efi.update_capsule = virt_efi_update_capsule;
>         efi.query_capsule_caps = virt_efi_query_capsule_caps;
>  }
> +
> +/*
> + * 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.
> + */
> +void efi_call_rts(struct work_struct *work)
> +{
> +       struct efi_runtime_work *efi_rts_work;
> +       void *arg1, *arg2, *arg3, *arg4, *arg5;
> +       efi_status_t status = EFI_NOT_FOUND;
> +
> +       efi_rts_work = container_of(work, struct efi_runtime_work, work);
> +       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->func) {
> +       case GET_TIME:
> +               status = efi_call_virt(get_time, (efi_time_t *)arg1,
> +                                      (efi_time_cap_t *)arg2);
> +               break;
> +       case SET_TIME:
> +               status = efi_call_virt(set_time, (efi_time_t *)arg1);
> +               break;
> +       case GET_WAKEUP_TIME:
> +               status = efi_call_virt(get_wakeup_time, (efi_bool_t *)arg1,
> +                                      (efi_bool_t *)arg2, (efi_time_t *)arg3);
> +               break;
> +       case SET_WAKEUP_TIME:
> +               status = efi_call_virt(set_wakeup_time, *(efi_bool_t *)arg1,
> +                                      (efi_time_t *)arg2);
> +               break;
> +       case GET_VARIABLE:
> +               status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
> +                                      (efi_guid_t *)arg2, (u32 *)arg3,
> +                                      (unsigned long *)arg4, (void *)arg5);
> +               break;
> +       case GET_NEXT_VARIABLE:
> +               status = efi_call_virt(get_next_variable, (unsigned long *)arg1,
> +                                      (efi_char16_t *)arg2,
> +                                      (efi_guid_t *)arg3);
> +               break;
> +       case SET_VARIABLE:
> +       case SET_VARIABLE_NONBLOCKING:
> +               status = efi_call_virt(set_variable, (efi_char16_t *)arg1,
> +                                      (efi_guid_t *)arg2, *(u32 *)arg3,
> +                                      *(unsigned long *)arg4, (void *)arg5);
> +               break;
> +       case QUERY_VARIABLE_INFO:
> +       case QUERY_VARIABLE_INFO_NONBLOCKING:
> +               status = efi_call_virt(query_variable_info, *(u32 *)arg1,
> +                                      (u64 *)arg2, (u64 *)arg3, (u64 *)arg4);
> +               break;
> +       case GET_NEXT_HIGH_MONO_COUNT:
> +               status = efi_call_virt(get_next_high_mono_count, (u32 *)arg1);
> +               break;
> +       case RESET_SYSTEM:
> +               __efi_call_virt(reset_system, *(int *)arg1,
> +                               *(efi_status_t *)arg2,
> +                               *(unsigned long *)arg3,
> +                               (efi_char16_t *)arg4);
> +               break;
> +       case UPDATE_CAPSULE:
> +               status = efi_call_virt(update_capsule,
> +                                      (efi_capsule_header_t **)arg1,
> +                                      *(unsigned long *)arg2,
> +                                      *(unsigned long *)arg3);
> +               break;
> +       case QUERY_CAPSULE_CAPS:
> +               status = efi_call_virt(query_capsule_caps,
> +                                      (efi_capsule_header_t **)arg1,
> +                                      *(unsigned long *)arg2, (u64 *)arg3,
> +                                      (int *)arg4);
> +               break;
> +       default:
> +               pr_err("Not a valid EFI_RT_SERVICE?");
> +               status = EFI_NOT_FOUND;
> +               break;

Given that efi_call_rts() is only called from the virt_efi_*()
funtions in the same file and that the IDs are internal as well,
shouldn't this be a hard error? i.e. no one should be calling this
with an unknown ID at all. But please see below.

> +       }
> +       efi_rts_work->status = status;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index c4efb3ef0dfa..4abb401d40f5 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1652,4 +1652,27 @@ struct linux_efi_tpm_eventlog {
>
>  extern int efi_tpm_eventlog_init(void);
>
> +/*
> + * efi_runtime_work:   Details of EFI Runtime Service work
> + * @func:              EFI Runtime Service function identifier
> + * @arg<1-5>:          EFI Runtime Service function arguments
> + * @status:            Status of executing EFI Runtime Service
> + */
> +struct efi_runtime_work {
> +       u8 func;
> +       void *arg1;
> +       void *arg2;
> +       void *arg3;
> +       void *arg4;
> +       void *arg5;
> +       efi_status_t status;
> +       struct work_struct work;
> +};
> +
> +/* Workqueue to queue EFI Runtime Services */
> +extern struct workqueue_struct *efi_rts_wq;
> +
> +/* Call back function that calls EFI Runtime Services */
> +extern void efi_call_rts(struct work_struct *work);

So this seems to indicate there will be external users calling
efi_call_rts() (even outside EFI itself, given it is in include/linux)
-- but then, how they will know what to put in "u8 func"?

Note that I have no clue on EFI, so I am just commenting on how the
patch looks. :)

Cheers,
Miguel

> +
>  #endif /* _LINUX_EFI_H */
> --
> 2.1.4
>

RE: [PATCH V1 2/3] efi: Introduce efi_rts_workqueue and necessary infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index ac5db5f8dbbf..4714b305ca90 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -76,6 +76,8 @@ static unsigned long *efi_tables[] = {
> >         &efi.mem_attr_table,
> >  };
> >
> > +struct workqueue_struct *efi_rts_wq;
> > +
> >  static bool disable_runtime;
> >  static int __init setup_noefi(char *arg)  { @@ -329,6 +331,15 @@
> > static int __init efisubsys_init(void)
> >         if (!efi_enabled(EFI_BOOT))
> >                 return 0;
> >
> > +       /* Create a work queue to run EFI Runtime Services */
> > +       efi_rts_wq = create_workqueue("efi_rts_workqueue");
> 
> Please consider alloc_workqueue() instead with the appropriate flags, since
> create_workqueue() and friends are deprecated.

Sure! Will change in V2.

> 
> > +       if (!efi_rts_wq) {
> > +               pr_err("Failed to create efi_rts_workqueue, EFI runtime services "
> > +                      "disabled.\n");
> > +               clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> > +               return 0;
> > +       }
> > +
> >         /*
> >          * Clean DUMMY object calls EFI Runtime Service, set_variable(), so
> >          * it should be invoked only after efi_rts_workqueue is ready.
> > diff --git a/drivers/firmware/efi/runtime-wrappers.c
> > b/drivers/firmware/efi/runtime-wrappers.c
> > index ae54870b2788..5cdb787da5d3 100644
> > --- a/drivers/firmware/efi/runtime-wrappers.c
> > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > @@ -1,6 +1,14 @@
> >  /*
> >   * runtime-wrappers.c - Runtime Services function call wrappers
> >   *
> > + * Implementation summary:
> > + * -----------------------
> > + * 1. When user/kernel thread requests to execute
> > + efi_runtime_service(),
> > + * enqueue work to efi_rts_workqueue.
> > + * 2. Caller thread waits until the work is finished because it's
> > + * dependent on the return status and execution of efi_runtime_service().
> > + * For instance, get_variable() and get_next_variable().
> > + *
> >   * Copyright (C) 2014 Linaro Ltd. <ard.biesheuvel@linaro.org>
> >   *
> >   * Split off from arch/x86/platform/efi/efi.c @@ -22,6 +30,8 @@
> > #include <linux/mutex.h>  #include <linux/semaphore.h>  #include
> > <linux/stringify.h>
> > +#include <linux/workqueue.h>
> > +
> >  #include <asm/efi.h>
> >
> >  /*
> > @@ -33,6 +43,50 @@
> >  #define __efi_call_virt(f, args...) \
> >         __efi_call_virt_pointer(efi.systab->runtime, f, args)
> >
> > +/* Each EFI Runtime Service is represented with a unique number */
> > +#define GET_TIME                                       0
> > +#define SET_TIME                                       1
> > +#define GET_WAKEUP_TIME                                        2
> > +#define SET_WAKEUP_TIME                                        3
> > +#define GET_VARIABLE                                   4
> > +#define GET_NEXT_VARIABLE                              5
> > +#define SET_VARIABLE                                   6
> > +#define SET_VARIABLE_NONBLOCKING                       7
> > +#define QUERY_VARIABLE_INFO                            8
> > +#define QUERY_VARIABLE_INFO_NONBLOCKING                        9
> > +#define GET_NEXT_HIGH_MONO_COUNT                       10
> > +#define RESET_SYSTEM                                   11
> > +#define UPDATE_CAPSULE                                 12
> > +#define QUERY_CAPSULE_CAPS                             13
> 
> An enum would be better, given these are just internal, contiguous IDs.
> 

Makes sense.

> > +
> > +/*
> > + * 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
> > + *
> > + * 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. So,
> > +queue_work()
> > + * should never fail.
> > + */
> > +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)                \
> > +({                                                                     \
> > +       struct efi_runtime_work efi_rts_work;                           \
> > +       efi_rts_work.status = EFI_ABORTED;                              \
> > +                                                                       \
> > +       INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts);            \
> > +       efi_rts_work.func = _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;                                      \
> > +       if (queue_work(efi_rts_wq, &efi_rts_work.work))                 \
> > +               flush_work(&efi_rts_work.work);                         \
> 
> If "So, queue_work() should never fail.", shouldn't this be a BUG() or similar?
> 

BUG() sounds appropriate. Will roll it in V2.

> > +                                                                       \
> > +       efi_rts_work.status;                                            \
> > +})
> > +
> >  void efi_call_virt_check_flags(unsigned long flags, const char *call)
> > {
> >         unsigned long cur_flags, mismatch; @@ -312,3 +366,92 @@ void
> > efi_native_runtime_setup(void)
> >         efi.update_capsule = virt_efi_update_capsule;
> >         efi.query_capsule_caps = virt_efi_query_capsule_caps;  }
> > +
> > +/*
> > + * 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.
> > + */
> > +void efi_call_rts(struct work_struct *work) {
> > +       struct efi_runtime_work *efi_rts_work;
> > +       void *arg1, *arg2, *arg3, *arg4, *arg5;
> > +       efi_status_t status = EFI_NOT_FOUND;
> > +
> > +       efi_rts_work = container_of(work, struct efi_runtime_work, work);
> > +       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->func) {
> > +       case GET_TIME:
> > +               status = efi_call_virt(get_time, (efi_time_t *)arg1,
> > +                                      (efi_time_cap_t *)arg2);
> > +               break;
> > +       case SET_TIME:
> > +               status = efi_call_virt(set_time, (efi_time_t *)arg1);
> > +               break;
> > +       case GET_WAKEUP_TIME:
> > +               status = efi_call_virt(get_wakeup_time, (efi_bool_t *)arg1,
> > +                                      (efi_bool_t *)arg2, (efi_time_t *)arg3);
> > +               break;
> > +       case SET_WAKEUP_TIME:
> > +               status = efi_call_virt(set_wakeup_time, *(efi_bool_t *)arg1,
> > +                                      (efi_time_t *)arg2);
> > +               break;
> > +       case GET_VARIABLE:
> > +               status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
> > +                                      (efi_guid_t *)arg2, (u32 *)arg3,
> > +                                      (unsigned long *)arg4, (void *)arg5);
> > +               break;
> > +       case GET_NEXT_VARIABLE:
> > +               status = efi_call_virt(get_next_variable, (unsigned long *)arg1,
> > +                                      (efi_char16_t *)arg2,
> > +                                      (efi_guid_t *)arg3);
> > +               break;
> > +       case SET_VARIABLE:
> > +       case SET_VARIABLE_NONBLOCKING:
> > +               status = efi_call_virt(set_variable, (efi_char16_t *)arg1,
> > +                                      (efi_guid_t *)arg2, *(u32 *)arg3,
> > +                                      *(unsigned long *)arg4, (void *)arg5);
> > +               break;
> > +       case QUERY_VARIABLE_INFO:
> > +       case QUERY_VARIABLE_INFO_NONBLOCKING:
> > +               status = efi_call_virt(query_variable_info, *(u32 *)arg1,
> > +                                      (u64 *)arg2, (u64 *)arg3, (u64 *)arg4);
> > +               break;
> > +       case GET_NEXT_HIGH_MONO_COUNT:
> > +               status = efi_call_virt(get_next_high_mono_count, (u32 *)arg1);
> > +               break;
> > +       case RESET_SYSTEM:
> > +               __efi_call_virt(reset_system, *(int *)arg1,
> > +                               *(efi_status_t *)arg2,
> > +                               *(unsigned long *)arg3,
> > +                               (efi_char16_t *)arg4);
> > +               break;
> > +       case UPDATE_CAPSULE:
> > +               status = efi_call_virt(update_capsule,
> > +                                      (efi_capsule_header_t **)arg1,
> > +                                      *(unsigned long *)arg2,
> > +                                      *(unsigned long *)arg3);
> > +               break;
> > +       case QUERY_CAPSULE_CAPS:
> > +               status = efi_call_virt(query_capsule_caps,
> > +                                      (efi_capsule_header_t **)arg1,
> > +                                      *(unsigned long *)arg2, (u64 *)arg3,
> > +                                      (int *)arg4);
> > +               break;
> > +       default:
> > +               pr_err("Not a valid EFI_RT_SERVICE?");
> > +               status = EFI_NOT_FOUND;
> > +               break;
> 
> Given that efi_call_rts() is only called from the virt_efi_*() funtions in the same
> file and that the IDs are internal as well, shouldn't this be a hard error? i.e. no
> one should be calling this with an unknown ID at all. But please see below.

That's true! No one should be calling with unknown ID.
Will make it a BUG().

> 
> > +       }
> > +       efi_rts_work->status = status; }
> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
> > c4efb3ef0dfa..4abb401d40f5 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1652,4 +1652,27 @@ struct linux_efi_tpm_eventlog {
> >
> >  extern int efi_tpm_eventlog_init(void);
> >
> > +/*
> > + * efi_runtime_work:   Details of EFI Runtime Service work
> > + * @func:              EFI Runtime Service function identifier
> > + * @arg<1-5>:          EFI Runtime Service function arguments
> > + * @status:            Status of executing EFI Runtime Service
> > + */
> > +struct efi_runtime_work {
> > +       u8 func;
> > +       void *arg1;
> > +       void *arg2;
> > +       void *arg3;
> > +       void *arg4;
> > +       void *arg5;
> > +       efi_status_t status;
> > +       struct work_struct work;
> > +};
> > +
> > +/* Workqueue to queue EFI Runtime Services */ extern struct
> > +workqueue_struct *efi_rts_wq;
> > +
> > +/* Call back function that calls EFI Runtime Services */ extern void
> > +efi_call_rts(struct work_struct *work);
> 
> So this seems to indicate there will be external users calling
> efi_call_rts() (even outside EFI itself, given it is in include/linux)
> -- but then, how they will know what to put in "u8 func"?
> 

My bad! efi_call_rts() should be static. There will not be any external users.

> Note that I have no clue on EFI, so I am just commenting on how the patch
> looks. :)
>

Thanks a lot! for the review. They are helpful :)
Will make changes in V2.

Regards,
Sai

Re: [PATCH V1 1/3] x86/efi: Call efi_delete_dummy_variable() during efi subsystem initialization

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

Hi Sai,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc3 next-20180227]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sai-Praneeth-Prakhya/Use-efi_rts_workqueue-to-invoke-EFI-Runtime-Services/20180227-075905
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

>> drivers/firmware/efi/efi.c:36:10: fatal error: asm/efi.h: No such file or directory
    #include <asm/efi.h>
             ^~~~~~~~~~~
   compilation terminated.

vim +36 drivers/firmware/efi/efi.c

    34	
    35	#include <asm/early_ioremap.h>
  > 36	#include <asm/efi.h>
    37	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5974 bytes --]