[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[Ard Biesheuvel]

The current UEFI implementation for arm64 fails to preserve/restore the contents
of the NEON register file, which may result in data corruption, especially now
that those contents are lazily restored for user processes.

This series proposes to fix this by wrapping all runtime services calls, and
adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers.

The first patch moves the existing x86 versions of those wrappers to generic
code, so that the second patch can easily enable them by supplying a definition
for efi_call_virt and adding a call to efi_native_runtime_setup().

Changes since v1:
- rename runtime.c -> runtime-wrappers.c
- make build depend on new Kconfig symbol EFI_RUNTIME_WRAPPERS to fix ia64
  breakage
- remove default #defines for efi_call_virt()/__efi_call_virt(), they are not
  needed anymore now that it is built conditionally
- add references to applicable UEFI/AAPCS spec sections

Ard Biesheuvel (2):
  efi/x86: move UEFI Runtime Services wrappers to generic code
  efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

 arch/arm64/Kconfig                      |   1 +
 arch/arm64/include/asm/efi.h            |  21 +++++
 arch/arm64/kernel/efi.c                 |  14 +--
 arch/x86/Kconfig                        |   1 +
 arch/x86/platform/efi/efi.c             | 144 +---------------------------
 drivers/firmware/efi/Kconfig            |   7 ++
 drivers/firmware/efi/Makefile           |   1 +
 drivers/firmware/efi/runtime-wrappers.c | 161 ++++++++++++++++++++++++++++++++
 include/linux/efi.h                     |   2 +
 9 files changed, 197 insertions(+), 155 deletions(-)
 create mode 100644 drivers/firmware/efi/runtime-wrappers.c

-- 
1.8.3.2

[PATCH v2 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code

[Ard Biesheuvel]

In order for other archs (such as arm64) to be able to reuse the virtual mode
function call wrappers, move them to drivers/firmware/efi/runtime-wrappers.c.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/Kconfig                        |   1 +
 arch/x86/platform/efi/efi.c             | 144 +---------------------------
 drivers/firmware/efi/Kconfig            |   7 ++
 drivers/firmware/efi/Makefile           |   1 +
 drivers/firmware/efi/runtime-wrappers.c | 161 ++++++++++++++++++++++++++++++++
 include/linux/efi.h                     |   2 +
 6 files changed, 174 insertions(+), 142 deletions(-)
 create mode 100644 drivers/firmware/efi/runtime-wrappers.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a8f749ef0fdc..4c3f026aa5ce 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1522,6 +1522,7 @@ config EFI
 	bool "EFI runtime service support"
 	depends on ACPI
 	select UCS2_STRING
+	select EFI_RUNTIME_WRAPPERS
 	---help---
 	  This enables the kernel to use EFI runtime services that are
 	  available (such as the EFI variable services).
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 87fc96bcc13c..36d0835210c3 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -104,130 +104,6 @@ static int __init setup_storage_paranoia(char *arg)
 }
 early_param("efi_no_storage_paranoia", setup_storage_paranoia);
 
-static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_set_time(efi_time_t *tm)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt(set_time, tm);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
-					     efi_bool_t *pending,
-					     efi_time_t *tm)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_get_variable(efi_char16_t *name,
-					  efi_guid_t *vendor,
-					  u32 *attr,
-					  unsigned long *data_size,
-					  void *data)
-{
-	return efi_call_virt(get_variable,
-			     name, vendor, attr,
-			     data_size, data);
-}
-
-static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
-					       efi_char16_t *name,
-					       efi_guid_t *vendor)
-{
-	return efi_call_virt(get_next_variable,
-			     name_size, name, vendor);
-}
-
-static efi_status_t virt_efi_set_variable(efi_char16_t *name,
-					  efi_guid_t *vendor,
-					  u32 attr,
-					  unsigned long data_size,
-					  void *data)
-{
-	return efi_call_virt(set_variable,
-			     name, vendor, attr,
-			     data_size, data);
-}
-
-static efi_status_t virt_efi_query_variable_info(u32 attr,
-						 u64 *storage_space,
-						 u64 *remaining_space,
-						 u64 *max_variable_size)
-{
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
-		return EFI_UNSUPPORTED;
-
-	return efi_call_virt(query_variable_info, attr, storage_space,
-			     remaining_space, max_variable_size);
-}
-
-static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
-{
-	return efi_call_virt(get_next_high_mono_count, count);
-}
-
-static void virt_efi_reset_system(int reset_type,
-				  efi_status_t status,
-				  unsigned long data_size,
-				  efi_char16_t *data)
-{
-	__efi_call_virt(reset_system, reset_type, status,
-			data_size, data);
-}
-
-static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
-					    unsigned long count,
-					    unsigned long sg_list)
-{
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
-		return EFI_UNSUPPORTED;
-
-	return efi_call_virt(update_capsule, capsules, count, sg_list);
-}
-
-static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
-						unsigned long count,
-						u64 *max_size,
-						int *reset_type)
-{
-	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
-		return EFI_UNSUPPORTED;
-
-	return efi_call_virt(query_capsule_caps, capsules, count, max_size,
-			     reset_type);
-}
-
 static efi_status_t __init phys_efi_set_virtual_address_map(
 	unsigned long memory_map_size,
 	unsigned long descriptor_size,
@@ -847,22 +723,6 @@ void __init old_map_region(efi_memory_desc_t *md)
 		       (unsigned long long)md->phys_addr);
 }
 
-static void 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.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.update_capsule = virt_efi_update_capsule;
-	efi.query_capsule_caps = virt_efi_query_capsule_caps;
-}
-
 /* Merge contiguous regions of the same type and attribute */
 static void __init efi_merge_regions(void)
 {
@@ -1049,7 +909,7 @@ static void __init kexec_enter_virtual_mode(void)
 	 */
 	efi.runtime_version = efi_systab.hdr.revision;
 
-	native_runtime_setup();
+	efi_native_runtime_setup();
 
 	efi.set_virtual_address_map = NULL;
 
@@ -1142,7 +1002,7 @@ static void __init __efi_enter_virtual_mode(void)
 	efi.runtime_version = efi_systab.hdr.revision;
 
 	if (efi_is_native())
-		native_runtime_setup();
+		efi_native_runtime_setup();
 	else
 		efi_thunk_runtime_setup();
 
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index d420ae2d3413..04a7af46736a 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -54,6 +54,13 @@ config EFI_PARAMS_FROM_FDT
 	  the EFI runtime support gets system table address, memory
           map address, and other parameters from the device tree.
 
+config EFI_RUNTIME_WRAPPERS
+	bool
+	help
+	  Selected by the arch if it needs to wrap UEFI Runtime Services calls,
+	  in which case it needs to provide #definitions of efi_call_virt and
+	  __efi_call_virt in <asm/efi.h>
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 9553496b0f43..e1096539eedb 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_EFI_VARS)			+= efivars.o
 obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
+obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
new file mode 100644
index 000000000000..10daa4bbb258
--- /dev/null
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -0,0 +1,161 @@
+/*
+ * runtime-wrappers.c - Runtime Services function call wrappers
+ *
+ * Copyright (C) 2014 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * Split off from arch/x86/platform/efi/efi.c
+ *
+ * Copyright (C) 1999 VA Linux Systems
+ * Copyright (C) 1999 Walt Drummond <drummond@valinux.com>
+ * Copyright (C) 1999-2002 Hewlett-Packard Co.
+ * Copyright (C) 2005-2008 Intel Co.
+ * Copyright (C) 2013 SuSE Labs
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/efi.h>
+#include <linux/spinlock.h>             /* spinlock_t */
+#include <asm/efi.h>
+
+/*
+ * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
+ * the EFI specification requires that callers of the time related runtime
+ * functions serialize with other CMOS accesses in the kernel, as the EFI time
+ * functions may choose to also use the legacy CMOS RTC.
+ */
+__weak DEFINE_SPINLOCK(rtc_lock);
+
+static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+	status = efi_call_virt(get_time, tm, tc);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return status;
+}
+
+static efi_status_t virt_efi_set_time(efi_time_t *tm)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+	status = efi_call_virt(set_time, tm);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return status;
+}
+
+static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
+					     efi_bool_t *pending,
+					     efi_time_t *tm)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return status;
+}
+
+static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+	status = efi_call_virt(set_wakeup_time, enabled, tm);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return status;
+}
+
+static efi_status_t virt_efi_get_variable(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 *attr,
+					  unsigned long *data_size,
+					  void *data)
+{
+	return efi_call_virt(get_variable, name, vendor, attr, data_size, data);
+}
+
+static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
+					       efi_char16_t *name,
+					       efi_guid_t *vendor)
+{
+	return efi_call_virt(get_next_variable, name_size, name, vendor);
+}
+
+static efi_status_t virt_efi_set_variable(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 attr,
+					  unsigned long data_size,
+					  void *data)
+{
+	return efi_call_virt(set_variable, name, vendor, attr, data_size, data);
+}
+
+static efi_status_t virt_efi_query_variable_info(u32 attr,
+						 u64 *storage_space,
+						 u64 *remaining_space,
+						 u64 *max_variable_size)
+{
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	return efi_call_virt(query_variable_info, attr, storage_space,
+			     remaining_space, max_variable_size);
+}
+
+static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
+{
+	return efi_call_virt(get_next_high_mono_count, count);
+}
+
+static void virt_efi_reset_system(int reset_type,
+				  efi_status_t status,
+				  unsigned long data_size,
+				  efi_char16_t *data)
+{
+	__efi_call_virt(reset_system, reset_type, status, data_size, data);
+}
+
+static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
+					    unsigned long count,
+					    unsigned long sg_list)
+{
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	return efi_call_virt(update_capsule, capsules, count, sg_list);
+}
+
+static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
+						unsigned long count,
+						u64 *max_size,
+						int *reset_type)
+{
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	return efi_call_virt(query_capsule_caps, capsules, count, max_size,
+			     reset_type);
+}
+
+void 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.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.update_capsule = virt_efi_update_capsule;
+	efi.query_capsule_caps = virt_efi_query_capsule_caps;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 41bbf8ba4ba8..0ceb816bdfc2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -521,6 +521,8 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
 					      int *reset_type);
 typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long size);
 
+void efi_native_runtime_setup(void);
+
 /*
  *  EFI Configuration Table and GUID definitions
  */
-- 
1.8.3.2

[PATCH v2 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code

[Matt Fleming]

On Thu, 26 Jun, at 12:09:05PM, Ard Biesheuvel wrote:
> In order for other archs (such as arm64) to be able to reuse the virtual mode
> function call wrappers, move them to drivers/firmware/efi/runtime-wrappers.c.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

[...]

> @@ -54,6 +54,13 @@ config EFI_PARAMS_FROM_FDT
>  	  the EFI runtime support gets system table address, memory
>            map address, and other parameters from the device tree.
>  
> +config EFI_RUNTIME_WRAPPERS
> +	bool
> +	help
> +	  Selected by the arch if it needs to wrap UEFI Runtime Services calls,
> +	  in which case it needs to provide #definitions of efi_call_virt and
> +	  __efi_call_virt in <asm/efi.h>
> +
>  endmenu

Actually, could we drop this help text?

That may seem like a backwards step, but I have concerns that we'll fail
to keep this help text in sync with the code. Furthermore, by providing
help text that kinda says, "casual users need the help text to
understand when to enable this feature". Clearly that's not what this
Kconfig symbol is for.

Most of the other guard Kconfig symbols don't provide this kind of text,
and I think there's good reason to follow suit.

What do you think?

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v2 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code

[Ard Biesheuvel]

On 1 July 2014 15:30, Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, 26 Jun, at 12:09:05PM, Ard Biesheuvel wrote:
>> In order for other archs (such as arm64) to be able to reuse the virtual mode
>> function call wrappers, move them to drivers/firmware/efi/runtime-wrappers.c.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> [...]
>
>> @@ -54,6 +54,13 @@ config EFI_PARAMS_FROM_FDT
>>         the EFI runtime support gets system table address, memory
>>            map address, and other parameters from the device tree.
>>
>> +config EFI_RUNTIME_WRAPPERS
>> +     bool
>> +     help
>> +       Selected by the arch if it needs to wrap UEFI Runtime Services calls,
>> +       in which case it needs to provide #definitions of efi_call_virt and
>> +       __efi_call_virt in <asm/efi.h>
>> +
>>  endmenu
>
> Actually, could we drop this help text?
>
> That may seem like a backwards step, but I have concerns that we'll fail
> to keep this help text in sync with the code. Furthermore, by providing
> help text that kinda says, "casual users need the help text to
> understand when to enable this feature". Clearly that's not what this
> Kconfig symbol is for.
>
> Most of the other guard Kconfig symbols don't provide this kind of text,
> and I think there's good reason to follow suit.
>
> What do you think?
>

I agree, but I kind of followed the example of the Kconfig symbols in
the vicinity.
So sure, rip it out. Or would you like me to do a v(n+1)?

-- 
Ard.

[PATCH v2 1/2] efi/x86: move UEFI Runtime Services wrappers to generic code

[Matt Fleming]

On Tue, 01 Jul, at 03:35:09PM, Ard Biesheuvel wrote:
> 
> I agree, but I kind of followed the example of the Kconfig symbols in
> the vicinity.
> So sure, rip it out. Or would you like me to do a v(n+1)?

Thanks. Nah, no need for another version, I'ved fixed this up in 'next',
but please do check the commits to make sure nothing untoward has
occurred.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

[Ard Biesheuvel]

According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is
allowed, and should adhere to the AAPCS64 calling convention, which states that
'only the bottom 64 bits of each value stored in registers v8-v15 need to be
preserved' (section 5.1.2).

This applies equally to UEFI Runtime Services called by the kernel, so make sure
the FP/SIMD register file is preserved in this case.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/Kconfig           |  1 +
 arch/arm64/include/asm/efi.h | 21 +++++++++++++++++++++
 arch/arm64/kernel/efi.c      | 14 +-------------
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a474de346be6..93e11f4d9513 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -299,6 +299,7 @@ config EFI
 	select LIBFDT
 	select UCS2_STRING
 	select EFI_PARAMS_FROM_FDT
+	select EFI_RUNTIME_WRAPPERS
 	default y
 	help
 	  This option provides support for runtime services provided
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 5a46c4e7f539..375ba342dca6 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -2,6 +2,7 @@
 #define _ASM_EFI_H
 
 #include <asm/io.h>
+#include <asm/neon.h>
 
 #ifdef CONFIG_EFI
 extern void efi_init(void);
@@ -11,4 +12,24 @@ extern void efi_idmap_init(void);
 #define efi_idmap_init()
 #endif
 
+#define efi_call_virt(f, ...)						\
+({									\
+	efi_##f##_t *__f = efi.systab->runtime->f;			\
+	efi_status_t __s;						\
+									\
+	kernel_neon_begin();						\
+	__s = __f(__VA_ARGS__);						\
+	kernel_neon_end();						\
+	__s;								\
+})
+
+#define __efi_call_virt(f, ...)						\
+({									\
+	efi_##f##_t *__f = efi.systab->runtime->f;			\
+									\
+	kernel_neon_begin();						\
+	__f(__VA_ARGS__);						\
+	kernel_neon_end();						\
+})
+
 #endif /* _ASM_EFI_H */
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 14db1f6e8d7f..56c3327bbf79 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -449,19 +449,7 @@ static int __init arm64_enter_virtual_mode(void)
 
 	/* Set up runtime services function pointers */
 	runtime = efi.systab->runtime;
-	efi.get_time = runtime->get_time;
-	efi.set_time = runtime->set_time;
-	efi.get_wakeup_time = runtime->get_wakeup_time;
-	efi.set_wakeup_time = runtime->set_wakeup_time;
-	efi.get_variable = runtime->get_variable;
-	efi.get_next_variable = runtime->get_next_variable;
-	efi.set_variable = runtime->set_variable;
-	efi.query_variable_info = runtime->query_variable_info;
-	efi.update_capsule = runtime->update_capsule;
-	efi.query_capsule_caps = runtime->query_capsule_caps;
-	efi.get_next_high_mono_count = runtime->get_next_high_mono_count;
-	efi.reset_system = runtime->reset_system;
-
+	efi_native_runtime_setup();
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
 	return 0;
-- 
1.8.3.2

[PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

[Catalin Marinas]

On Thu, Jun 26, 2014 at 11:09:06AM +0100, Ard Biesheuvel wrote:
> According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is
> allowed, and should adhere to the AAPCS64 calling convention, which states that
> 'only the bottom 64 bits of each value stored in registers v8-v15 need to be
> preserved' (section 5.1.2).
> 
> This applies equally to UEFI Runtime Services called by the kernel, so make sure
> the FP/SIMD register file is preserved in this case.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

While the code looks fine, I think there is a mismatch between what the
subject says and what the patch does (enabling EFI_RUNTIME_WRAPPERS).

-- 
Catalin

[PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

[Ard Biesheuvel]

On 4 July 2014 17:45, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Thu, Jun 26, 2014 at 11:09:06AM +0100, Ard Biesheuvel wrote:
>> According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is
>> allowed, and should adhere to the AAPCS64 calling convention, which states that
>> 'only the bottom 64 bits of each value stored in registers v8-v15 need to be
>> preserved' (section 5.1.2).
>>
>> This applies equally to UEFI Runtime Services called by the kernel, so make sure
>> the FP/SIMD register file is preserved in this case.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> While the code looks fine, I think there is a mismatch between what the
> subject says and what the patch does (enabling EFI_RUNTIME_WRAPPERS).
>

Not entirely. In order to be able to insert calls to
kernel_neon_begin()/end() into the runtime services calls, we need
a) to supply definitions for efi_call_virt() and __efi_call_virt()
that contain those calls to kernel_neon_begin()/end()
b) to enable runtime wrappers (which is what uses those definitions)

Would you prefer those to be split in 2 patches?

--
Ard.

[PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

[Catalin Marinas]

On Fri, Jul 04, 2014 at 04:51:31PM +0100, Ard Biesheuvel wrote:
> On 4 July 2014 17:45, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Thu, Jun 26, 2014 at 11:09:06AM +0100, Ard Biesheuvel wrote:
> >> According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is
> >> allowed, and should adhere to the AAPCS64 calling convention, which states that
> >> 'only the bottom 64 bits of each value stored in registers v8-v15 need to be
> >> preserved' (section 5.1.2).
> >>
> >> This applies equally to UEFI Runtime Services called by the kernel, so make sure
> >> the FP/SIMD register file is preserved in this case.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > While the code looks fine, I think there is a mismatch between what the
> > subject says and what the patch does (enabling EFI_RUNTIME_WRAPPERS).
> >
> 
> Not entirely. In order to be able to insert calls to
> kernel_neon_begin()/end() into the runtime services calls, we need
> a) to supply definitions for efi_call_virt() and __efi_call_virt()
> that contain those calls to kernel_neon_begin()/end()
> b) to enable runtime wrappers (which is what uses those definitions)
> 
> Would you prefer those to be split in 2 patches?

No, that's fine. You could just add the above explanation to the commit
log. Otherwise:

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

[PATCH v2 2/2] efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

[Ard Biesheuvel]

On 4 July 2014 18:59, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Jul 04, 2014 at 04:51:31PM +0100, Ard Biesheuvel wrote:
>> On 4 July 2014 17:45, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > On Thu, Jun 26, 2014 at 11:09:06AM +0100, Ard Biesheuvel wrote:
>> >> According to the UEFI spec section 2.3.6.4, the use of FP/SIMD instructions is
>> >> allowed, and should adhere to the AAPCS64 calling convention, which states that
>> >> 'only the bottom 64 bits of each value stored in registers v8-v15 need to be
>> >> preserved' (section 5.1.2).
>> >>
>> >> This applies equally to UEFI Runtime Services called by the kernel, so make sure
>> >> the FP/SIMD register file is preserved in this case.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > While the code looks fine, I think there is a mismatch between what the
>> > subject says and what the patch does (enabling EFI_RUNTIME_WRAPPERS).
>> >
>>
>> Not entirely. In order to be able to insert calls to
>> kernel_neon_begin()/end() into the runtime services calls, we need
>> a) to supply definitions for efi_call_virt() and __efi_call_virt()
>> that contain those calls to kernel_neon_begin()/end()
>> b) to enable runtime wrappers (which is what uses those definitions)
>>
>> Would you prefer those to be split in 2 patches?
>
> No, that's fine. You could just add the above explanation to the commit
> log. Otherwise:
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

OK, thanks. I will add your ack and ask Matt to take it.

-- 
Ard.

[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[Mark Salter]

On Thu, 2014-06-26 at 12:09 +0200, Ard Biesheuvel wrote:
> The current UEFI implementation for arm64 fails to preserve/restore the contents
> of the NEON register file,

Does the current implementation actually use NEON registers?
I know there are some at least, which build with -mgeneral-regs-only
to keep gcc from using NEON registers.

Not that that means we don't need this. Just curious.

[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[Ard Biesheuvel]

On 26 June 2014 15:58, Mark Salter <msalter@redhat.com> wrote:
> On Thu, 2014-06-26 at 12:09 +0200, Ard Biesheuvel wrote:
>> The current UEFI implementation for arm64 fails to preserve/restore the contents
>> of the NEON register file,
>
> Does the current implementation actually use NEON registers?
> I know there are some at least, which build with -mgeneral-regs-only
> to keep gcc from using NEON registers.
>

Tianocore/EDK2 does not use -mgeneral-regs-only when building for
AArch64, and my current build shows that, for instance, GCC starts
spilling to FP registers when compiling the LZMA decoder, and it is
likely to do the same for the crypto bits once I start enabling them
(although, interestingly enough, the OpenSSL SHA-1 C implementation
runs 60% faster with -mgeneral-regs-only set)

> Not that that means we don't need this. Just curious.
>

The spec does not forbid it, so we will need it anyhow ...

-- 
Ard.

[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[Matt Fleming]

On Thu, 26 Jun, at 12:09:04PM, Ard Biesheuvel wrote:
> The current UEFI implementation for arm64 fails to preserve/restore the contents
> of the NEON register file, which may result in data corruption, especially now
> that those contents are lazily restored for user processes.
> 
> This series proposes to fix this by wrapping all runtime services calls, and
> adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers.
> 
> The first patch moves the existing x86 versions of those wrappers to generic
> code, so that the second patch can easily enable them by supplying a definition
> for efi_call_virt and adding a call to efi_native_runtime_setup().
> 
> Changes since v1:
> - rename runtime.c -> runtime-wrappers.c
> - make build depend on new Kconfig symbol EFI_RUNTIME_WRAPPERS to fix ia64
>   breakage
> - remove default #defines for efi_call_virt()/__efi_call_virt(), they are not
>   needed anymore now that it is built conditionally
> - add references to applicable UEFI/AAPCS spec sections
> 
> Ard Biesheuvel (2):
>   efi/x86: move UEFI Runtime Services wrappers to generic code
>   efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls

Thanks Ard. These patches look OK to me, and I've now pulled them into
my 'next' branch.

It'd be nice if we could get some ACKs from other people since this is
an ABI issue.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[Ard Biesheuvel]

On 1 July 2014 15:26, Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, 26 Jun, at 12:09:04PM, Ard Biesheuvel wrote:
>> The current UEFI implementation for arm64 fails to preserve/restore the contents
>> of the NEON register file, which may result in data corruption, especially now
>> that those contents are lazily restored for user processes.
>>
>> This series proposes to fix this by wrapping all runtime services calls, and
>> adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers.
>>
>> The first patch moves the existing x86 versions of those wrappers to generic
>> code, so that the second patch can easily enable them by supplying a definition
>> for efi_call_virt and adding a call to efi_native_runtime_setup().
>>
>> Changes since v1:
>> - rename runtime.c -> runtime-wrappers.c
>> - make build depend on new Kconfig symbol EFI_RUNTIME_WRAPPERS to fix ia64
>>   breakage
>> - remove default #defines for efi_call_virt()/__efi_call_virt(), they are not
>>   needed anymore now that it is built conditionally
>> - add references to applicable UEFI/AAPCS spec sections
>>
>> Ard Biesheuvel (2):
>>   efi/x86: move UEFI Runtime Services wrappers to generic code
>>   efi/arm64: preserve FP/SIMD registers on UEFI runtime services calls
>
> Thanks Ard. These patches look OK to me, and I've now pulled them into
> my 'next' branch.
>
> It'd be nice if we could get some ACKs from other people since this is
> an ABI issue.

Yes, they have been awfully quiet, haven't they?

At least Roy has volunteered to get involved in the USWG to get the
spec clarified.

@Leif, Catalin, Olivier: care to chime in?

-- 
Ard.

[PATCH v2 0/2] efi: preserve NEON registers on UEFI services calls

[Olivier Martin]

It is correct the UEFI 2.4 spec says 'Floating point and SIMD instructions may be used'. And it is also correct these registers are not preserved in the current UEFI EDK2 implementation.
I will keep track in ARM UEFI todo list.

Acked-by: Olivier Martin <olivier.martin@arm.com>

Thanks Ard for your patch,
Olivier

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel at linaro.org]
> Sent: 01 July 2014 14:46
> To: Matt Fleming
> Cc: Matt Fleming; x86 at kernel.org; Catalin Marinas; linux-
> efi at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> hpa at zytor.com; Leif Lindholm; Roy Franz; msalter at redhat.com; Olivier
> Martin
> Subject: Re: [PATCH v2 0/2] efi: preserve NEON registers on UEFI
> services calls
> 
> On 1 July 2014 15:26, Matt Fleming <matt@console-pimps.org> wrote:
> > On Thu, 26 Jun, at 12:09:04PM, Ard Biesheuvel wrote:
> >> The current UEFI implementation for arm64 fails to preserve/restore
> the contents
> >> of the NEON register file, which may result in data corruption,
> especially now
> >> that those contents are lazily restored for user processes.
> >>
> >> This series proposes to fix this by wrapping all runtime services
> calls, and
> >> adding kernel_neon_begin()/kernel_neon_end() pairs to the wrappers.
> >>
> >> The first patch moves the existing x86 versions of those wrappers to
> generic
> >> code, so that the second patch can easily enable them by supplying a
> definition
> >> for efi_call_virt and adding a call to efi_native_runtime_setup().
> >>
> >> Changes since v1:
> >> - rename runtime.c -> runtime-wrappers.c
> >> - make build depend on new Kconfig symbol EFI_RUNTIME_WRAPPERS to
> fix ia64
> >>   breakage
> >> - remove default #defines for efi_call_virt()/__efi_call_virt(),
> they are not
> >>   needed anymore now that it is built conditionally
> >> - add references to applicable UEFI/AAPCS spec sections
> >>
> >> Ard Biesheuvel (2):
> >>   efi/x86: move UEFI Runtime Services wrappers to generic code
> >>   efi/arm64: preserve FP/SIMD registers on UEFI runtime services
> calls
> >
> > Thanks Ard. These patches look OK to me, and I've now pulled them
> into
> > my 'next' branch.
> >
> > It'd be nice if we could get some ACKs from other people since this
> is
> > an ABI issue.
> 
> Yes, they have been awfully quiet, haven't they?
> 
> At least Roy has volunteered to get involved in the USWG to get the
> spec clarified.
> 
> @Leif, Catalin, Olivier: care to chime in?
> 
> --
> Ard.