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

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

[Alex Thorlton]

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

[PATCH 1/3] Convert efi_call_virt to efi_call_virt_generic

[Alex Thorlton]

This commit makes a few slight modifications to the efi_call_virt macro
to get it to work with function pointers that are stored in locations
other than efi.systab->runtime, and renames the macro to
efi_call_virt_generic.  The majority of the changes here are to pull
these macros up into header files so that they can be accessed from
outside of drivers/firmware/efi/runtime-wrappers.c.

The most significant change not directly related to the code move is to
add an extra "p" argument into the appropriate efi_call macros, and use
that new argument in place of the, formerly hard-coded,
efi.systab->runtime pointer.

The last piece of the puzzle was to add an efi_call_virt macro back into
drivers/firmware/efi/runtime-wrappers.c to wrap around the new
efi_call_virt_generic macro - this was mainly to keep the code from
looking too cluttered by adding a bunch of extra references to
efi.systab->runtime everywhere.

Note that I also broke up the code in the efi_call_virt_generic macro a
bit in the process of moving it.

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              |  4 +--
 drivers/firmware/efi/runtime-wrappers.c | 53 +++++++--------------------------
 include/linux/efi.h                     | 51 +++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 45 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 78d1e74..f310f0b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -81,8 +81,8 @@ struct efi_scratch {
 	}								\
 })
 
-#define arch_efi_call_virt(f, args...)					\
-	efi_call((void *)efi.systab->runtime->f, args)			\
+#define arch_efi_call_virt(p, f, args...)					\
+	efi_call((void *)p->f, args)			\
 
 #define arch_efi_call_virt_teardown()					\
 ({									\
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 23bef6b..e8bc493 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -22,7 +22,16 @@
 #include <linux/stringify.h>
 #include <asm/efi.h>
 
-static void efi_call_virt_check_flags(unsigned long flags, const char *call)
+/*
+ * Wrap around the new efi_call_virt_generic macros so that the
+ * code doesn't get too cluttered
+ */
+#define efi_call_virt(f, args...)   \
+	efi_call_virt_generic(efi.systab->runtime, f, args)
+#define __efi_call_virt(f, args...) \
+	__efi_call_virt_generic(efi.systab->runtime, f, args)
+
+void efi_call_virt_check_flags(unsigned long flags, const char *call)
 {
 	unsigned long cur_flags, mismatch;
 
@@ -39,48 +48,6 @@ static void efi_call_virt_check_flags(unsigned long flags, const char *call)
 }
 
 /*
- * Arch code can implement the following three template macros, avoiding
- * reptition for the void/non-void return cases of {__,}efi_call_virt:
- *
- *  * arch_efi_call_virt_setup
- *
- *    Sets up the environment for the call (e.g. switching page tables,
- *    allowing kernel-mode use of floating point, if required).
- *
- *  * arch_efi_call_virt
- *
- *    Performs the call. The last expression in the macro must be the call
- *    itself, allowing the logic to be shared by the void and non-void
- *    cases.
- *
- *  * arch_efi_call_virt_teardown
- *
- *    Restores the usual kernel environment once the call has returned.
- */
-
-#define efi_call_virt(f, args...)					\
-({									\
-	efi_status_t __s;						\
-	unsigned long flags;						\
-	arch_efi_call_virt_setup();					\
-	local_save_flags(flags);					\
-	__s = arch_efi_call_virt(f, args);				\
-	efi_call_virt_check_flags(flags, __stringify(f));		\
-	arch_efi_call_virt_teardown();					\
-	__s;								\
-})
-
-#define __efi_call_virt(f, args...)					\
-({									\
-	unsigned long flags;						\
-	arch_efi_call_virt_setup();					\
-	local_save_flags(flags);					\
-	arch_efi_call_virt(f, args);					\
-	efi_call_virt_check_flags(flags, __stringify(f));		\
-	arch_efi_call_virt_teardown();					\
-})
-
-/*
  * According to section 7.1 of the UEFI spec, Runtime Services are not fully
  * reentrant, and there are particular combinations of calls that need to be
  * serialized. (source: UEFI Specification v2.4A)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index df7acb5..8d1117ae 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1471,4 +1471,55 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
 			   unsigned long size);
 
 bool efi_runtime_disabled(void);
+extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
+
+/*
+ * Arch code can implement the following three template macros, avoiding
+ * reptition for the void/non-void return cases of {__,}efi_call_virt:
+ *
+ *  * arch_efi_call_virt_setup
+ *
+ *    Sets up the environment for the call (e.g. switching page tables,
+ *    allowing kernel-mode use of floating point, if required).
+ *
+ *  * arch_efi_call_virt
+ *
+ *    Performs the call. The last expression in the macro must be the call
+ *    itself, allowing the logic to be shared by the void and non-void
+ *    cases.
+ *
+ *  * arch_efi_call_virt_teardown
+ *
+ *    Restores the usual kernel environment once the call has returned.
+ */
+
+#define efi_call_virt_generic(p, f, args...)					\
+({									\
+	efi_status_t __s;						\
+	unsigned long flags;						\
+									\
+	arch_efi_call_virt_setup();					\
+									\
+	local_save_flags(flags);					\
+	__s = arch_efi_call_virt(p, f, args);				\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
+									\
+	arch_efi_call_virt_teardown();					\
+									\
+	__s;								\
+})
+
+#define __efi_call_virt_generic(p, f, args...)					\
+({									\
+	unsigned long flags;						\
+									\
+	arch_efi_call_virt_setup();					\
+									\
+	local_save_flags(flags);					\
+	arch_efi_call_virt(p, f, args);					\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
+									\
+	arch_efi_call_virt_teardown();					\
+})
+
 #endif /* _LINUX_EFI_H */
-- 
1.8.5.6

Re: [PATCH 1/3] Convert efi_call_virt to efi_call_virt_generic

[Matt Fleming]

On Wed, 18 May, at 02:11:39PM, Alex Thorlton wrote:
> This commit makes a few slight modifications to the efi_call_virt macro
> to get it to work with function pointers that are stored in locations
> other than efi.systab->runtime, and renames the macro to
> efi_call_virt_generic.  The majority of the changes here are to pull
> these macros up into header files so that they can be accessed from
> outside of drivers/firmware/efi/runtime-wrappers.c.
> 
> The most significant change not directly related to the code move is to
> add an extra "p" argument into the appropriate efi_call macros, and use
> that new argument in place of the, formerly hard-coded,
> efi.systab->runtime pointer.
> 
> The last piece of the puzzle was to add an efi_call_virt macro back into
> drivers/firmware/efi/runtime-wrappers.c to wrap around the new
> efi_call_virt_generic macro - this was mainly to keep the code from
> looking too cluttered by adding a bunch of extra references to
> efi.systab->runtime everywhere.
> 
> Note that I also broke up the code in the efi_call_virt_generic macro a
> bit in the process of moving it.
> 
> 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              |  4 +--
>  drivers/firmware/efi/runtime-wrappers.c | 53 +++++++--------------------------
>  include/linux/efi.h                     | 51 +++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 78d1e74..f310f0b 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -81,8 +81,8 @@ struct efi_scratch {
>  	}								\
>  })
>  
> -#define arch_efi_call_virt(f, args...)					\
> -	efi_call((void *)efi.systab->runtime->f, args)			\
> +#define arch_efi_call_virt(p, f, args...)					\
> +	efi_call((void *)p->f, args)			\
>  
>  #define arch_efi_call_virt_teardown()					\
>  ({									\

Oops, you're missing updates to the 32-bit version and ARM/arm64,
which results in this,

drivers/firmware/efi/runtime-wrappers.c: In function ‘virt_efi_get_time’:
arch/x86/include/asm/efi.h:46:4: error: ‘efi_efi’ undeclared (first use in this function)
  ((efi_##f##_t __attribute__((regparm(0)))*)   \
    ^

but that's easily fixed up.

> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index 23bef6b..e8bc493 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -22,7 +22,16 @@
>  #include <linux/stringify.h>
>  #include <asm/efi.h>
>  
> -static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> +/*
> + * Wrap around the new efi_call_virt_generic macros so that the
> + * code doesn't get too cluttered
> + */
> +#define efi_call_virt(f, args...)   \
> +	efi_call_virt_generic(efi.systab->runtime, f, args)
> +#define __efi_call_virt(f, args...) \
> +	__efi_call_virt_generic(efi.systab->runtime, f, args)
> +
> +void efi_call_virt_check_flags(unsigned long flags, const char *call)
>  {
>  	unsigned long cur_flags, mismatch;
>  

I'm not crazy about using "generic" in the name. How about
efi_call_virt_function() or efi_call_virt_pointer()?

Re: [PATCH 1/3] Convert efi_call_virt to efi_call_virt_generic

[Alex Thorlton]

On Thu, Jun 02, 2016 at 04:41:14PM +0100, Matt Fleming wrote:
> Oops, you're missing updates to the 32-bit version and ARM/arm64,
> which results in this,
> 
> drivers/firmware/efi/runtime-wrappers.c: In function ‘virt_efi_get_time’:
> arch/x86/include/asm/efi.h:46:4: error: ‘efi_efi’ undeclared (first use in this function)
>   ((efi_##f##_t __attribute__((regparm(0)))*)   \
>     ^
> 
> but that's easily fixed up.

Oops!  Sorry about that.  I had done a pretty recent pull before I did
my last build, but it was probably before these updates came in.  I'll
rebase my patches onto the latest tip kernel and fix this.

> > +/*
> > + * Wrap around the new efi_call_virt_generic macros so that the
> > + * code doesn't get too cluttered
> > + */
> > +#define efi_call_virt(f, args...)   \
> > +	efi_call_virt_generic(efi.systab->runtime, f, args)
> > +#define __efi_call_virt(f, args...) \
> > +	__efi_call_virt_generic(efi.systab->runtime, f, args)
> > +
> > +void efi_call_virt_check_flags(unsigned long flags, const char *call)
> >  {
> >  	unsigned long cur_flags, mismatch;
> >  
> 
> I'm not crazy about using "generic" in the name. How about
> efi_call_virt_function() or efi_call_virt_pointer()?

I'm not married to "generic."  I'll change it to efi_call_virt_pointer.
I think efi_call_virt_function is a little confusing/redundant, since a
call is always to a function, right?

I will fix this stuff up and send a v2.

Thanks for looking this over, Matt!

- Alex

[PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic

[Alex Thorlton]

Now that the efi_call_virt macro has been generalized to be able to
use EFI system tables besides efi.systab, we are able to convert our
uv_bios_call wrapper to use this standard EFI callback mechanism.

This simple change is part of a much larger effort to recover from some
issues with the way we were mapping in some of our MMRs, and the way
that we were doing our BIOS callbacks, which were uncovered by commit
67a9108ed431 ("x86/efi: Build our own page table structures").

The first issue that this uncovered was that we were relying on the EFI
memory mapping mechanism to map in our MMR space for us, which, while
reliable, was technically a bug, as it relied on "undefined" behavior in
the mapping code.

The reason we were able to piggyback on the EFI memory mapping code to
map in our MMRs was because, previously, EFI code used the
trampoline_pgd, which shares a few entries with the main kernel pgd.  It
just so happened, that the memory range containing our MMRs was inside
one of those shared regions, which kept our code working without issue
for quite a while.

Anyways, once we discovered this problem, we brought back our original
code to map in the MMRs with commit 08914f436bdd ("x86/platform/UV:
Bring back the call to map_low_mmrs in uv_system_init").  This got our
systems a little further along, but we were still running into trouble
with our EFI callbacks, which prevented us from booting all the way up.

Our first step towards fixing the BIOS callbacks was to get our
uv_bios_call wrapper updated to use efi_call_virt instead of the plain
efi_call.  The previous patch took care of the effort needed to make
that possible.  Along the way, we hit a major issue with some confusion
about how to properly pull arguments higher than number 6 off the stack
in the efi_call code, which resulted in Linus's commit 683ad8092cd2
("x86/efi: Fix 7-parameter efi_call()s").

Now that all of those issues are out of the way, we're able to make this
simple change to use the new efi_call_virt_generic in uv_bios_call which
gets our machines booting, running properly, and able to execute our
callbacks with 6+ arguments.

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/platform/uv/bios_uv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 815fec6..0ae0826 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -40,8 +40,7 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
 		 */
 		return BIOS_STATUS_UNIMPLEMENTED;
 
-	ret = efi_call((void *)__va(tab->function), (u64)which,
-			a1, a2, a3, a4, a5);
+	ret = efi_call_virt_generic(tab, function, (u64)which, a1, a2, a3, a4, a5);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(uv_bios_call);
-- 
1.8.5.6

Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic

[Matt Fleming]

On Wed, 18 May, at 02:11:40PM, Alex Thorlton wrote:
> Now that the efi_call_virt macro has been generalized to be able to
> use EFI system tables besides efi.systab, we are able to convert our
> uv_bios_call wrapper to use this standard EFI callback mechanism.
> 
> This simple change is part of a much larger effort to recover from some
> issues with the way we were mapping in some of our MMRs, and the way
> that we were doing our BIOS callbacks, which were uncovered by commit
> 67a9108ed431 ("x86/efi: Build our own page table structures").
> 
> The first issue that this uncovered was that we were relying on the EFI
> memory mapping mechanism to map in our MMR space for us, which, while
> reliable, was technically a bug, as it relied on "undefined" behavior in
> the mapping code.
> 
> The reason we were able to piggyback on the EFI memory mapping code to
> map in our MMRs was because, previously, EFI code used the
> trampoline_pgd, which shares a few entries with the main kernel pgd.  It
> just so happened, that the memory range containing our MMRs was inside
> one of those shared regions, which kept our code working without issue
> for quite a while.
> 
> Anyways, once we discovered this problem, we brought back our original
> code to map in the MMRs with commit 08914f436bdd ("x86/platform/UV:
> Bring back the call to map_low_mmrs in uv_system_init").  This got our
> systems a little further along, but we were still running into trouble
> with our EFI callbacks, which prevented us from booting all the way up.
> 
> Our first step towards fixing the BIOS callbacks was to get our
> uv_bios_call wrapper updated to use efi_call_virt instead of the plain
> efi_call.  The previous patch took care of the effort needed to make
> that possible.  Along the way, we hit a major issue with some confusion
> about how to properly pull arguments higher than number 6 off the stack
> in the efi_call code, which resulted in Linus's commit 683ad8092cd2
> ("x86/efi: Fix 7-parameter efi_call()s").
> 
> Now that all of those issues are out of the way, we're able to make this
> simple change to use the new efi_call_virt_generic in uv_bios_call which
> gets our machines booting, running properly, and able to execute our
> callbacks with 6+ arguments.
> 
> 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/platform/uv/bios_uv.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 815fec6..0ae0826 100644
> --- a/arch/x86/platform/uv/bios_uv.c
> +++ b/arch/x86/platform/uv/bios_uv.c
> @@ -40,8 +40,7 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
>  		 */
>  		return BIOS_STATUS_UNIMPLEMENTED;
>  
> -	ret = efi_call((void *)__va(tab->function), (u64)which,
> -			a1, a2, a3, a4, a5);
> +	ret = efi_call_virt_generic(tab, function, (u64)which, a1, a2, a3, a4, a5);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(uv_bios_call);

Unless I've missed it, I didn't see an explanation in the changelog of
why it's OK to switch from using __va(tab->function) to tab->function
directly, which presumably is a physical address.

Was that intended?

Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic

[Alex Thorlton]

On Thu, Jun 02, 2016 at 08:45:47PM +0100, Matt Fleming wrote:
> Unless I've missed it, I didn't see an explanation in the changelog of
> why it's OK to switch from using __va(tab->function) to tab->function
> directly, which presumably is a physical address.
> 
> Was that intended?

It was intended.  The motivation is so that we can use the same
"dereference the pointer inside the macro" stuff that we do with the
efi.systab->runtime pointer.  IINM, the reason it works is because we do

/*
 * Make sure the 1:1 mappings are present as a catch-all for b0rked
 * firmware which doesn't update all internal pointers after switching
 * to virtual mode and would otherwise crap on us.
 */
__map_region(md, md->phys_addr);

Inside of efi_map_region, so we know we'll have that physical address
mapped into the EFI page table.

Upon review, I'm wondering if the correct thing to do  here is to
update that pointer during the switch to virtual mode, to avoid the
b0rkage mentioned in the above comment.

Either way, the straigh tab->function dereference should work while
using the EFI page table, but I do wonder if that tab->function address
should have been updated to the __va() version of itself before we reach
this point.

Maybe you can comment on that?

- Alex

Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic

[Alex Thorlton]

On Thu, Jun 02, 2016 at 04:14:03PM -0500, Alex Thorlton wrote:
> On Thu, Jun 02, 2016 at 08:45:47PM +0100, Matt Fleming wrote:
> > Unless I've missed it, I didn't see an explanation in the changelog of
> > why it's OK to switch from using __va(tab->function) to tab->function
> > directly, which presumably is a physical address.
> > 
> > Was that intended?
> 
> It was intended.  The motivation is so that we can use the same
> "dereference the pointer inside the macro" stuff that we do with the
> efi.systab->runtime pointer.  IINM, the reason it works is because we do
> 
> /*
>  * Make sure the 1:1 mappings are present as a catch-all for b0rked
>  * firmware which doesn't update all internal pointers after switching
>  * to virtual mode and would otherwise crap on us.
>  */
> __map_region(md, md->phys_addr);
> 
> Inside of efi_map_region, so we know we'll have that physical address
> mapped into the EFI page table.
> 
> Upon review, I'm wondering if the correct thing to do  here is to
> update that pointer during the switch to virtual mode, to avoid the
> b0rkage mentioned in the above comment.

Realizing after I typed this that the b0rkage in question is firmware
b0rkage, so the comment there doesn't really apply to this situation.

> Either way, the straigh tab->function dereference should work while
> using the EFI page table, but I do wonder if that tab->function address
> should have been updated to the __va() version of itself before we reach
> this point.

I played with this some and it's not working the way I expected.  In
thinking about it, I believe the reason is fairly obvious.  We have
already switched to the EFI page table when we try to dereference the
pointer, so if we have some ffff88.... address, it won't be mapped
(this is somewhat related to the situation that got us here in the first
place).

I think, in order for this to work, tab->function has to be either the
physical address, or the address in the ffffffef....  range that also
gets mapped in during efi_map_region.

My brain is getting close to fried for the day, so this might be utter
nonsense, but it's sounding pretty reasonable to me right now.  We'll
see how I feel about it tomorrow :)

Let me know what you think!

- Alex

[PATCH 3/3] Update efi_thunk to use efi_call_virt_generic

[Alex Thorlton]

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

Re: [PATCH 3/3] Update efi_thunk to use efi_call_virt_generic

[Matt Fleming]

On Wed, 18 May, at 02:11:41PM, Alex Thorlton wrote:
> 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__);					\
> +})
> +

This isn't correct because you're turning the runtime decision of
whether we're executing the thunking code into a build time one.

Users can enable CONFIG_EFI_MIXED in their builds but never actually
run that kernel on a mixed mode machine. One of the original design
intentions behind CONFIG_EFI_MIXED was that you can (and should!) turn
it on because it has no effect unless you run it on a machine with
32-bit EFI.

The switch to the thunk layer is done in efi_thunk_runtime_setup().

As a real world example of this, the openSUSE x86_64 kernel config has
CONFIG_EFI_MIXED enabled out of the box.

The thunk code should be able to reuse the regular x86_64
arch_efi_call_virt_setup() and arch_efi_call_virt_teardown(), since,

  a. We can also disable preemption without issue
  b. We can disable/reenable interrupts around those existing wrappers
  c. The "if (efi_scratch.use_pgd)" check is missing because we
     *always* use the EFI pgtables for mixed mode, it's a requirement

Would something like this work instead? It's not as neat as your
suggestion but it's a damn sight better than what we have today.

---

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 6e7242be1c87..b976084e56ef 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -469,18 +469,13 @@ extern efi_status_t efi64_thunk(u32, ...);
 	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();						\
+	arch_efi_call_virt_setup();					\
 									\
 	func = runtime_service32(f);					\
 	__s = efi64_thunk(func, __VA_ARGS__);			\
 									\
-	write_cr3(efi_scratch.prev_cr3);				\
-	__flush_tlb_all();						\
+	arch_efi_call_virt_teardown();					\
 	local_irq_restore(flags);					\
 									\
 	__s;								\

Re: [PATCH 3/3] Update efi_thunk to use efi_call_virt_generic

[Alex Thorlton]

On Thu, Jun 02, 2016 at 09:19:49PM +0100, Matt Fleming wrote:
> On Wed, 18 May, at 02:11:41PM, Alex Thorlton wrote:
> > +#define arch_efi_call_virt(p, f, ...)					\
> > +({									\
> > +	u32 func = runtime_service32(f);				\
> > +	efi64_thunk(func, __VA_ARGS__);					\
> > +})
> > +
> 
> This isn't correct because you're turning the runtime decision of
> whether we're executing the thunking code into a build time one.

Ahh, yep, you're absolutely correct.  That's not what I intended to do,
but that's definitely the effect that this change has.

> Would something like this work instead? It's not as neat as your
> suggestion but it's a damn sight better than what we have today.
> 
> ---
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 6e7242be1c87..b976084e56ef 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -469,18 +469,13 @@ extern efi_status_t efi64_thunk(u32, ...);
>  	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();						\
> +	arch_efi_call_virt_setup();					\
>  									\
>  	func = runtime_service32(f);					\
>  	__s = efi64_thunk(func, __VA_ARGS__);			\
>  									\
> -	write_cr3(efi_scratch.prev_cr3);				\
> -	__flush_tlb_all();						\
> +	arch_efi_call_virt_teardown();					\
>  	local_irq_restore(flags);					\
>  									\
>  	__s;								\

This looks good to me.  We're at least making use of the
arch_efi_call_virt_* stuff where possible, and only using the special
thunk code where necessary.  I think it's a good middle ground between
the two approaches (especially considering the fact that mine won't
work :) 

I will re-work that last patch to include this change instead of my
original, broken one.

Thanks, Matt!

- Alex

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

[Alex Thorlton]

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