Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1403242 > unrolled thread

[RFC PATCH 0/3] x86/UV, x86/efi: Re-factor efi_call_virt for general use

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/3] x86/UV, x86/efi: Re-factor efi_call_virt for general use Alex Thorlton <athorlton@sgi.com> - 2016-05-18 21:20 +0200
    Re: [RFC PATCH 0/3] x86/UV, x86/efi: Re-factor efi_call_virt for  general use Alex Thorlton <athorlton@sgi.com> - 2016-05-18 21:20 +0200
    [PATCH 3/3] Update efi_thunk to use efi_call_virt_generic Alex Thorlton <athorlton@sgi.com> - 2016-05-18 21:20 +0200

#1403242 — [RFC PATCH 0/3] x86/UV, x86/efi: Re-factor efi_call_virt for general use

Hey guys,

This patchset creates a general purpose version of the efi_call_virt
macro that does not assume that the function pointer being passed in is
inside of efi.systab->runtime.  It also fixes up a few potentional users
of that new functionality, namely the SGI UV, and the CONFIG_EFI_MIXED
code paths.

Quick breakdown of the patches:

Patch 1) Move necessary macros to locations where we can access them.
	 Remove hard-coded efi.systab reference from efi_call_virt.
	 Rename/create new macros as needed.
Patch 2) Simple change to allow UV code to utilize the new
	 functionality.  Included a detailed explanation of how we got
	 here.
Patch 3) This is the one I'm most looking for input on.  I merge the
	 efi_thunk code in with the new efi_call_virt scheme (giving it
	 it's own arch_efi_call_* macros, conditionally defined for
	 EFI_MIXED) and then use efi_thunk as a wrapper for
	 efi_call_virt_generic.

The first two have been tested on simulators and hardware, but the third
has only been compile-tested.  I don't have any hardware to test that
on.  I'm sure I could set up a VM with OVMF, but I haven't taken the
time yet :)

A few notes/concerns that I had about the patches:

1) I could have created more specific names for the individual uses of
   efi_call_virt instead of using the originals as wrappers for
   efi_call_virt_generic.
   * Would be easy enough to do, but would need to update all the
     original callers of the function.  Not difficult, just different.
2) I'm not sure if each macro really needs to have the same args
   implemented.  Could be simplified a bit if we didn't use "p" on the
   EFI_MIXED side.
   * I did this for consistency, but I suppose it's not explicitly
     necessary.
3) It wouldn't be too hard to add an efi_thunk_generic function that
   would just expect a 32-bit pointer, and then have an
   efi_thunk_runtime to wrap that and handle the call to
   runtime_service32 for us, so that the efi.systab pointer doesn't have
   to be hard-coded into the EFI_MIXED version of efi_call_virt_generic.
   * This would only be to cover a hypothetical situation where there
     was code that needed to use a function pointer outside of
     efi.systab->runtime, running with CONFIG_EFI_MIXED enabled.

I'm still playing around with some of this to see how it could be
cleaned up, but wanted to get something out there so people could see
how I'm thinking about handling this.

Let me know what everybody thinks!

Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Mike Travis <travis@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: x86@kernel.org
Cc: linux-efi@vger.kernel.org

Alex Thorlton (3):
  Convert efi_call_virt to efi_call_virt_generic
  Update uv_bios_call to use efi_call_virt_generic
  Update efi_thunk to use efi_call_virt_generic

 arch/x86/include/asm/efi.h              | 51 +++++++++++++++++++++++++++++--
 arch/x86/platform/efi/efi_64.c          | 49 +++++++-----------------------
 arch/x86/platform/uv/bios_uv.c          |  3 +-
 drivers/firmware/efi/runtime-wrappers.c | 53 +++++++--------------------------
 include/linux/efi.h                     | 51 +++++++++++++++++++++++++++++++
 5 files changed, 122 insertions(+), 85 deletions(-)

-- 
1.8.5.6

[toc] | [next] | [standalone]

#1403244 — Re: [RFC PATCH 0/3] x86/UV, x86/efi: Re-factor efi_call_virt for general use

On Wed, May 18, 2016 at 02:11:38PM -0500, Alex Thorlton wrote:
> Let me know what everybody thinks!

I realized right as I sent these that I should've included prefixes on
the individual patches.  I have a feeling we'll need a v2 anyways, so
I'll clean that up then.

- Alex

[toc] | [prev] | [next] | [standalone]

#1403245 — [PATCH 3/3] Update efi_thunk to use efi_call_virt_generic

Now that we have efi_call_virt_generic, we no longer need to have an
entirely separate efi_thunk macro to handle the CONFIG_EFI_MIXED
scenario, where the function pointers cannot be read directly out of
efi.systab->runtime.

This commit creates a new set of arch_efi_call_virt* macros to mimic the
behavior of the old efi_thunk macro.  In the end, the code should be the
same, functionally, but we'll have eliminated a good chunk of code
duplication by splitting the efi_thunk macro up into the appropriate
arch_efi_call_virt bits and then just calling efi_call_virt_generic,
instead of efi_thunk.  I do go ahead and create a new efi_thunk macro in
arch/x86/platform/efi/efi_64.c, but this is mainly to keep the existing
code clean.

One thing to note here, is that this is not and absolutely *perfect* use
of the efi_call_virt_generic macro, in that it still has the
efi.systab->runtime pointer hard-coded into runtime_service32.  For now,
I think this should be fine, as the only users of the function already
assumed that this was where their function pointer would come from.  If
another CONFIG_EFI_MIXED user comes along and needs to use
efi_call_virt_generic, then we will need to re-think things a bit.

Signed-off-by: Alex Thorlton <athorlton@sgi.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Mike Travis <travis@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: x86@kernel.org
Cc: linux-efi@vger.kernel.org
---
 arch/x86/include/asm/efi.h     | 47 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/platform/efi/efi_64.c | 49 ++++++++++--------------------------------
 2 files changed, 58 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index f310f0b..6643f9b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -68,6 +68,52 @@ struct efi_scratch {
 	u64	phys_stack;
 } __packed;
 
+#ifdef CONFIG_EFI_MIXED
+extern efi_status_t efi64_thunk(u32, ...);
+
+#define runtime_service32(func)						 \
+({									 \
+	u32 table = (u32)(unsigned long)efi.systab;			 \
+	u32 *rt, *___f;							 \
+									 \
+	rt = (u32 *)(table + offsetof(efi_system_table_32_t, runtime));	 \
+	___f = (u32 *)(*rt + offsetof(efi_runtime_services_32_t, func)); \
+	*___f;								 \
+})
+
+/*
+ * Switch to the EFI page tables early so that we can access the 1:1
+ * runtime services mappings which are not mapped in any other page
+ * tables. This function must be called before runtime_service32().
+ *
+ * Also, disable interrupts because the IDT points to 64-bit handlers,
+ * which aren't going to function correctly when we switch to 32-bit.
+ */
+#define arch_efi_call_virt_setup()					\
+({									\
+	efi_sync_low_kernel_mappings();					\
+	local_irq_save(flags);						\
+									\
+	efi_scratch.prev_cr3 = read_cr3();				\
+	write_cr3((unsigned long)efi_scratch.efi_pgt);			\
+	__flush_tlb_all();						\
+})
+
+#define arch_efi_call_virt(p, f, ...)					\
+({									\
+	u32 func = runtime_service32(f);				\
+	efi64_thunk(func, __VA_ARGS__);					\
+})
+
+#define arch_efi_call_virt_teardown()					\
+({									\
+	write_cr3(efi_scratch.prev_cr3);				\
+	__flush_tlb_all();						\
+	local_irq_restore(flags);					\
+})
+
+#else /* !CONFIG_EFI_MIXED */
+
 #define arch_efi_call_virt_setup()					\
 ({									\
 	efi_sync_low_kernel_mappings();					\
@@ -94,6 +140,7 @@ struct efi_scratch {
 	__kernel_fpu_end();						\
 	preempt_enable();						\
 })
+#endif
 
 extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
 					u32 type, u64 attribute);
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 6e7242b..747bbc3 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -443,48 +443,21 @@ void __init efi_dump_pagetable(void)
 }
 
 #ifdef CONFIG_EFI_MIXED
-extern efi_status_t efi64_thunk(u32, ...);
-
-#define runtime_service32(func)						 \
-({									 \
-	u32 table = (u32)(unsigned long)efi.systab;			 \
-	u32 *rt, *___f;							 \
-									 \
-	rt = (u32 *)(table + offsetof(efi_system_table_32_t, runtime));	 \
-	___f = (u32 *)(*rt + offsetof(efi_runtime_services_32_t, func)); \
-	*___f;								 \
-})
 
 /*
- * Switch to the EFI page tables early so that we can access the 1:1
- * runtime services mappings which are not mapped in any other page
- * tables. This function must be called before runtime_service32().
+ * Note that we pass in NULL here instead of the systab/function name combo
+ * that usually gets us what we need.  This is because the EFI_MIXED code
+ * still assumes that all EFI callback function pointers will be in
+ * efi.systab->runtime.  Currently there are no users in the EFI_MIXED code
+ * that require efi_thunk to behave otherwise.
  *
- * Also, disable interrupts because the IDT points to 64-bit handlers,
- * which aren't going to function correctly when we switch to 32-bit.
+ * If EFI_MIXED users want to call runtime functions that *don't* live in
+ * efi.systab->runtime, runtime_service32 and some of the related macros
+ * will need to be updated to handle this.  Not a major headache, but
+ * something to keep in mind.
  */
-#define efi_thunk(f, ...)						\
-({									\
-	efi_status_t __s;						\
-	unsigned long flags;						\
-	u32 func;							\
-									\
-	efi_sync_low_kernel_mappings();					\
-	local_irq_save(flags);						\
-									\
-	efi_scratch.prev_cr3 = read_cr3();				\
-	write_cr3((unsigned long)efi_scratch.efi_pgt);			\
-	__flush_tlb_all();						\
-									\
-	func = runtime_service32(f);					\
-	__s = efi64_thunk(func, __VA_ARGS__);			\
-									\
-	write_cr3(efi_scratch.prev_cr3);				\
-	__flush_tlb_all();						\
-	local_irq_restore(flags);					\
-									\
-	__s;								\
-})
+#define efi_thunk(f, args...)   \
+	efi_call_virt_generic(NULL, f, args)
 
 efi_status_t efi_thunk_set_virtual_address_map(
 	void *phys_set_virtual_address_map,
-- 
1.8.5.6

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web