[RFC PATCH 0/2] Fix EFI runtime calls on SGI UV

[RFC PATCH 0/2] Fix EFI runtime calls on SGI UV

[Alex Thorlton]

These patches make the necessary changes to get SGI UVs working with the
latest EFI memory mapping code.  The motivation behind the changes is
fairly simple:

Patch 1: The current efi_call_virt macro will not work with function
	 pointers that don't live in efi.systab->runtime
Patch 2: The efi_call assembly code incorrectly puts the return address
	 from the current stack frame into the space reserved for the
	 arguments in the stack frame that we're setting up for our EFI
	 runtime call, instead of the 7th argument to efi_call.

I'm pretty sure that the second patch should be fine in its current
state, but there will likely need to be some discussion about how to
properly handle the stuff I'm doing in the first patch.  I know we need
to do something kind of like what I did, but I know my copied/pasted
UV-specific macros are not how we'll want to implement this in the end.

Please note that, as requested, these patches apply to the current
tip/master branch, but they will not apply out-of-the-box to
linus/master.

Let me know what everybody thinks!

Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Mike Travis <travis@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: x86@kernel.org
Cc: linux-efi@vger.kernel.org

Alex Thorlton (2):
  Create UV efi_call macros
  Fix efi_call

 arch/x86/include/asm/efi.h              |  3 ++
 arch/x86/platform/efi/efi_stub_64.S     |  2 +-
 arch/x86/platform/uv/bios_uv.c          |  3 +-
 drivers/firmware/efi/runtime-wrappers.c | 44 +-------------------------
 include/linux/efi.h                     | 55 +++++++++++++++++++++++++++++++++
 5 files changed, 61 insertions(+), 46 deletions(-)

-- 
1.8.5.6

[PATCH 1/2] Create UV efi_call macros

[Alex Thorlton]

We need a slightly different macro than the standard efi_call_virt,
since those macros all assume that the function pointer, f, that gets
passed in will live somewhere in efi.systab->runtime.  Our EFI function
pointer lives in efi.uv_systab, so we can't use the standard macros out
of the box.

This commit creates some new uv_* macros that are functionally
equivalent to the standard ones, with the exception of allowing us to
use a different function pointer.  I figure that we won't want to call
these uv_* in the end (we'll probably want something more generic), but
I thought I would get everyone's thoughts on how we might best reach
that goal instead of just trying to come up with a new implementation on
my own.

By itself, this commit does get our machines booting, but it needs the
small fix to the efi_call assembly code for our modules to work.

Signed-off-by: Alex Thorlton <athorlton@sgi.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Mike Travis <travis@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: x86@kernel.org
Cc: linux-efi@vger.kernel.org
---
 arch/x86/include/asm/efi.h              |  3 ++
 arch/x86/platform/uv/bios_uv.c          |  3 +-
 drivers/firmware/efi/runtime-wrappers.c | 44 +-------------------------
 include/linux/efi.h                     | 55 +++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 45 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 78d1e74..f384047 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -84,6 +84,9 @@ struct efi_scratch {
 #define arch_efi_call_virt(f, args...)					\
 	efi_call((void *)efi.systab->runtime->f, args)			\
 
+#define uv_efi_call_virt(f, args...)					\
+	efi_call((void *)f, args)			\
+
 #define arch_efi_call_virt_teardown()					\
 ({									\
 	if (efi_scratch.use_pgd) {					\
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index 815fec6..62a46cb 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 = uv_call_virt(tab->function, (u64)which, a1, a2, a3, a4, a5);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(uv_bios_call);
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 23bef6b..008a1a3 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -22,7 +22,7 @@
 #include <linux/stringify.h>
 #include <asm/efi.h>
 
-static void efi_call_virt_check_flags(unsigned long flags, const char *call)
+void efi_call_virt_check_flags(unsigned long flags, const char *call)
 {
 	unsigned long cur_flags, mismatch;
 
@@ -39,48 +39,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..f429269 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1471,4 +1471,59 @@ 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(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();					\
+})
+
+#define uv_call_virt(f, args...)					\
+({									\
+	efi_status_t __s;						\
+	unsigned long flags;						\
+	arch_efi_call_virt_setup();					\
+	local_save_flags(flags);					\
+	__s = uv_efi_call_virt(f, args);				\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
+	arch_efi_call_virt_teardown();					\
+	__s;								\
+})
 #endif /* _LINUX_EFI_H */
-- 
1.8.5.6

[PATCH 2/2] Fix efi_call

[Alex Thorlton]

The efi_call assembly code has a slight error that prevents us from
using arguments 7 and higher, which will be passed in on the stack.

        mov (%rsp), %rax
        mov 8(%rax), %rax
	...
        mov %rax, 40(%rsp)

This code goes and grabs the return address for the current stack frame,
and puts it on the stack, next the 5th argument for the EFI runtime
call.  Considering the fact that having the return address in that
position on the stack makes no sense, I'm guessing that the intent of
this code was actually to grab an argument off the stack frame for this
call and place it into the frame for the next one.

The small change to that offset (i.e. 8(%rax) to 16(%rax)) ensures that
we grab the 7th argument off the stack, and pass it as the 6th argument
to the EFI runtime function that we're about to call.  This change gets
our EFI runtime calls that need to pass more than 6 arguments working
again.

Signed-off-by: Alex Thorlton <athorlton@sgi.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Mike Travis <travis@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: x86@kernel.org
Cc: linux-efi@vger.kernel.org
---
 arch/x86/platform/efi/efi_stub_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
index 92723ae..62938ff 100644
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -43,7 +43,7 @@ ENTRY(efi_call)
 	FRAME_BEGIN
 	SAVE_XMM
 	mov (%rsp), %rax
-	mov 8(%rax), %rax
+	mov 16(%rax), %rax
 	subq $48, %rsp
 	mov %r9, 32(%rsp)
 	mov %rax, 40(%rsp)
-- 
1.8.5.6

Re: [PATCH 1/2] Create UV efi_call macros

[Ingo Molnar]

* Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org> wrote:

> +#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();					\
> +})
> +
> +#define uv_call_virt(f, args...)					\
> +({									\
> +	efi_status_t __s;						\
> +	unsigned long flags;						\
> +	arch_efi_call_virt_setup();					\
> +	local_save_flags(flags);					\
> +	__s = uv_efi_call_virt(f, args);				\
> +	efi_call_virt_check_flags(flags, __stringify(f));		\
> +	arch_efi_call_virt_teardown();					\
> +	__s;								\
> +})

Btw., a very (very!) small stylistic nit that caught my eyes, and I realize that 
you just moved code, but could you please improve these macros a bit and make it 
look like regular kernel code? I.e. something like:

#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;								\
})

This delineates the various blocks of code: variables, setup, the saving/calling 
block plus the return code.

(Assuming the EFI folks like the whole approach.)

Thanks,

	Ingo

Re: [PATCH 2/2] Fix efi_call

[Ingo Molnar]

* Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org> wrote:

> The efi_call assembly code has a slight error that prevents us from
> using arguments 7 and higher, which will be passed in on the stack.
> 
>         mov (%rsp), %rax
>         mov 8(%rax), %rax
> 	...
>         mov %rax, 40(%rsp)
> 
> This code goes and grabs the return address for the current stack frame,
> and puts it on the stack, next the 5th argument for the EFI runtime
> call.  Considering the fact that having the return address in that
> position on the stack makes no sense, I'm guessing that the intent of
> this code was actually to grab an argument off the stack frame for this
> call and place it into the frame for the next one.
> 
> The small change to that offset (i.e. 8(%rax) to 16(%rax)) ensures that
> we grab the 7th argument off the stack, and pass it as the 6th argument
> to the EFI runtime function that we're about to call.  This change gets
> our EFI runtime calls that need to pass more than 6 arguments working
> again.

I suppose the SGI/UV code is the only one using 7 arguments or more? Might make 
sense to point that out in the changelog.

> 
> Signed-off-by: Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org>
> Cc: Dimitri Sivanich <sivanich-sJ/iWh9BUns@public.gmane.org>
> Cc: Russ Anderson <rja-sJ/iWh9BUns@public.gmane.org>
> Cc: Mike Travis <travis-sJ/iWh9BUns@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> Cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/x86/platform/efi/efi_stub_64.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
> index 92723ae..62938ff 100644
> --- a/arch/x86/platform/efi/efi_stub_64.S
> +++ b/arch/x86/platform/efi/efi_stub_64.S
> @@ -43,7 +43,7 @@ ENTRY(efi_call)
>  	FRAME_BEGIN
>  	SAVE_XMM
>  	mov (%rsp), %rax
> -	mov 8(%rax), %rax
> +	mov 16(%rax), %rax
>  	subq $48, %rsp
>  	mov %r9, 32(%rsp)
>  	mov %rax, 40(%rsp)

Just curious, how did you find this bug? It's a pretty obscure one, of the 
'developer tears out hairs from frustruation' type ...

Thanks,

	Ingo

Re: [PATCH 1/2] Create UV efi_call macros

[Ard Biesheuvel]

On 12 May 2016 at 08:46, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org> wrote:
>
>> +#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();                                  \
>> +})
>> +
>> +#define uv_call_virt(f, args...)                                     \
>> +({                                                                   \
>> +     efi_status_t __s;                                               \
>> +     unsigned long flags;                                            \
>> +     arch_efi_call_virt_setup();                                     \
>> +     local_save_flags(flags);                                        \
>> +     __s = uv_efi_call_virt(f, args);                                \
>> +     efi_call_virt_check_flags(flags, __stringify(f));               \
>> +     arch_efi_call_virt_teardown();                                  \
>> +     __s;                                                            \
>> +})
>
> Btw., a very (very!) small stylistic nit that caught my eyes, and I realize that
> you just moved code, but could you please improve these macros a bit and make it
> look like regular kernel code? I.e. something like:
>
> #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;                                                            \
> })
>
> This delineates the various blocks of code: variables, setup, the saving/calling
> block plus the return code.
>
> (Assuming the EFI folks like the whole approach.)
>

Fine by me, although having a newline after arch_efi_call_virt_setup()
but not before arch_efi_call_virt_teardown() seems rather arbitrary

-- 
Ard.

Re: [PATCH 1/2] Create UV efi_call macros

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 12 May 2016 at 08:46, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > * Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org> wrote:
> >
> >> +#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();                                  \
> >> +})
> >> +
> >> +#define uv_call_virt(f, args...)                                     \
> >> +({                                                                   \
> >> +     efi_status_t __s;                                               \
> >> +     unsigned long flags;                                            \
> >> +     arch_efi_call_virt_setup();                                     \
> >> +     local_save_flags(flags);                                        \
> >> +     __s = uv_efi_call_virt(f, args);                                \
> >> +     efi_call_virt_check_flags(flags, __stringify(f));               \
> >> +     arch_efi_call_virt_teardown();                                  \
> >> +     __s;                                                            \
> >> +})
> >
> > Btw., a very (very!) small stylistic nit that caught my eyes, and I realize that
> > you just moved code, but could you please improve these macros a bit and make it
> > look like regular kernel code? I.e. something like:
> >
> > #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;                                                            \
> > })
> >
> > This delineates the various blocks of code: variables, setup, the saving/calling
> > block plus the return code.
> >
> > (Assuming the EFI folks like the whole approach.)
> >
> 
> Fine by me, although having a newline after arch_efi_call_virt_setup()
> but not before arch_efi_call_virt_teardown() seems rather arbitrary

It's an oversight! :-)

#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;								\
})

But if it's too segmented this is fine too:

#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;								\
})

Thanks,

	Ingo

Re: [PATCH 2/2] Fix efi_call

[Matt Fleming]

On Wed, 11 May, at 02:55:45PM, Alex Thorlton wrote:
> The efi_call assembly code has a slight error that prevents us from
> using arguments 7 and higher, which will be passed in on the stack.
> 
>         mov (%rsp), %rax
>         mov 8(%rax), %rax
> 	...
>         mov %rax, 40(%rsp)
> 
> This code goes and grabs the return address for the current stack frame,
> and puts it on the stack, next the 5th argument for the EFI runtime
> call.  Considering the fact that having the return address in that
> position on the stack makes no sense, I'm guessing that the intent of
> this code was actually to grab an argument off the stack frame for this
> call and place it into the frame for the next one.
> 
> The small change to that offset (i.e. 8(%rax) to 16(%rax)) ensures that
> we grab the 7th argument off the stack, and pass it as the 6th argument
> to the EFI runtime function that we're about to call.  This change gets
> our EFI runtime calls that need to pass more than 6 arguments working
> again.
> 
> Signed-off-by: Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org>
> Cc: Dimitri Sivanich <sivanich-sJ/iWh9BUns@public.gmane.org>
> Cc: Russ Anderson <rja-sJ/iWh9BUns@public.gmane.org>
> Cc: Mike Travis <travis-sJ/iWh9BUns@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> Cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/x86/platform/efi/efi_stub_64.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
> index 92723ae..62938ff 100644
> --- a/arch/x86/platform/efi/efi_stub_64.S
> +++ b/arch/x86/platform/efi/efi_stub_64.S
> @@ -43,7 +43,7 @@ ENTRY(efi_call)
>  	FRAME_BEGIN
>  	SAVE_XMM
>  	mov (%rsp), %rax
> -	mov 8(%rax), %rax
> +	mov 16(%rax), %rax
>  	subq $48, %rsp
>  	mov %r9, 32(%rsp)
>  	mov %rax, 40(%rsp)

Nice. Your fix looks good, so I've put it in the urgent queue and
tagged it for stable.

Re: [PATCH 2/2] Fix efi_call

[Matt Fleming]

On Thu, 12 May, at 08:48:35AM, Ingo Molnar wrote:
> 
> * Alex Thorlton <athorlton@sgi.com> wrote:
> 
> > The efi_call assembly code has a slight error that prevents us from
> > using arguments 7 and higher, which will be passed in on the stack.
> > 
> >         mov (%rsp), %rax
> >         mov 8(%rax), %rax
> > 	...
> >         mov %rax, 40(%rsp)
> > 
> > This code goes and grabs the return address for the current stack frame,
> > and puts it on the stack, next the 5th argument for the EFI runtime
> > call.  Considering the fact that having the return address in that
> > position on the stack makes no sense, I'm guessing that the intent of
> > this code was actually to grab an argument off the stack frame for this
> > call and place it into the frame for the next one.
> > 
> > The small change to that offset (i.e. 8(%rax) to 16(%rax)) ensures that
> > we grab the 7th argument off the stack, and pass it as the 6th argument
> > to the EFI runtime function that we're about to call.  This change gets
> > our EFI runtime calls that need to pass more than 6 arguments working
> > again.
> 
> I suppose the SGI/UV code is the only one using 7 arguments or more? Might make 
> sense to point that out in the changelog.
 
Yeah, I included that info when I applied this patch.

Re: [PATCH 1/2] Create UV efi_call macros

[Matt Fleming]

(Adding author of arch_efi_call code)

On Wed, 11 May, at 02:55:44PM, Alex Thorlton wrote:
> We need a slightly different macro than the standard efi_call_virt,
> since those macros all assume that the function pointer, f, that gets
> passed in will live somewhere in efi.systab->runtime.  Our EFI function
> pointer lives in efi.uv_systab, so we can't use the standard macros out
> of the box.
 
Is that true of all EFI services pointers? From reading the SGI/UV
code I got the impression that it's possible to call the standard
services via efi.systab->runtime but you also need the ability to call
these UV bios functions, which are not accessible via efi.systab.

Do you actually want the same environment setup and teardown to happen
when calling efi.uv_systab ? Or are you simply trying to reuse the
efi_call() implementation?

> This commit creates some new uv_* macros that are functionally
> equivalent to the standard ones, with the exception of allowing us to
> use a different function pointer.  I figure that we won't want to call
> these uv_* in the end (we'll probably want something more generic), but
> I thought I would get everyone's thoughts on how we might best reach
> that goal instead of just trying to come up with a new implementation on
> my own.
> 
> By itself, this commit does get our machines booting, but it needs the
> small fix to the efi_call assembly code for our modules to work.
 
Could you provide some more details in the changelog on why your
machine doesn't boot without this change?

> Signed-off-by: Alex Thorlton <athorlton@sgi.com>
> Cc: Dimitri Sivanich <sivanich@sgi.com>
> Cc: Russ Anderson <rja@sgi.com>
> Cc: Mike Travis <travis@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: x86@kernel.org
> Cc: linux-efi@vger.kernel.org
> ---
>  arch/x86/include/asm/efi.h              |  3 ++
>  arch/x86/platform/uv/bios_uv.c          |  3 +-
>  drivers/firmware/efi/runtime-wrappers.c | 44 +-------------------------
>  include/linux/efi.h                     | 55 +++++++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 78d1e74..f384047 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -84,6 +84,9 @@ struct efi_scratch {
>  #define arch_efi_call_virt(f, args...)					\
>  	efi_call((void *)efi.systab->runtime->f, args)			\
>  
> +#define uv_efi_call_virt(f, args...)					\
> +	efi_call((void *)f, args)			\
> +
>  #define arch_efi_call_virt_teardown()					\
>  ({									\
>  	if (efi_scratch.use_pgd) {					\
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 815fec6..62a46cb 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 = uv_call_virt(tab->function, (u64)which, a1, a2, a3, a4, a5);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(uv_bios_call);
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index 23bef6b..008a1a3 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -22,7 +22,7 @@
>  #include <linux/stringify.h>
>  #include <asm/efi.h>
>  
> -static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> +void efi_call_virt_check_flags(unsigned long flags, const char *call)
>  {
>  	unsigned long cur_flags, mismatch;
>  
> @@ -39,48 +39,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..f429269 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1471,4 +1471,59 @@ 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(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();					\
> +})
> +
> +#define uv_call_virt(f, args...)					\
> +({									\
> +	efi_status_t __s;						\
> +	unsigned long flags;						\
> +	arch_efi_call_virt_setup();					\
> +	local_save_flags(flags);					\
> +	__s = uv_efi_call_virt(f, args);				\
> +	efi_call_virt_check_flags(flags, __stringify(f));		\
> +	arch_efi_call_virt_teardown();					\
> +	__s;								\
> +})
>  #endif /* _LINUX_EFI_H */
> -- 
> 1.8.5.6
> 

Re: [PATCH 2/2] Fix efi_call

[Alex Thorlton]

On Thu, May 12, 2016 at 08:48:35AM +0200, Ingo Molnar wrote:
> I suppose the SGI/UV code is the only one using 7 arguments or more? Might make
> sense to point that out in the changelog.

First off, to everybody, sorry for the delayed responses.  I've been
AFK for a few days and forgot to set my vacation notice :(

Yes, I believe that's it.  I didn't do a full audit, but a quick glance
at the other users of this call showed that nobody else appears to be
using that many args.

> Just curious, how did you find this bug? It's a pretty obscure one, of the 
> 'developer tears out hairs from frustruation' type ...

Yes, this one was a real puzzle to figure out.  Basically I just stepped
through the assembly code from a known good point to see how we ended up
where we did.  I quite a bit of help from the vets around here, as well
as from our simulator that I used to step through our early boot code to
find the problem.

The real hair pulling mostly came from trying to figure out *WHY* we
were putting the return address in this seemingly random spot on the
stack.  After thoroughly re-reading assorted Intel (et. al.) docs about
a hundred times, I was able to piece together what I thought was
supposed to be going on here.  The solution may be simple, but arriving
there was anything but that :)

- Alex

Re: [PATCH 2/2] Fix efi_call

[Alex Thorlton]

On Thu, May 12, 2016 at 12:41:49PM +0100, Matt Fleming wrote:
> On Wed, 11 May, at 02:55:45PM, Alex Thorlton wrote:
> Nice. Your fix looks good, so I've put it in the urgent queue and
> tagged it for stable.

Great!  Thanks, Matt.

- Alex

Re: [PATCH 1/2] Create UV efi_call macros

[Alex Thorlton]

On Thu, May 12, 2016 at 01:06:00PM +0100, Matt Fleming wrote:
> (Adding author of arch_efi_call code)
> 
> On Wed, 11 May, at 02:55:44PM, Alex Thorlton wrote:
> > We need a slightly different macro than the standard efi_call_virt,
> > since those macros all assume that the function pointer, f, that gets
> > passed in will live somewhere in efi.systab->runtime.  Our EFI function
> > pointer lives in efi.uv_systab, so we can't use the standard macros out
> > of the box.
>  
> Is that true of all EFI services pointers? From reading the SGI/UV
> code I got the impression that it's possible to call the standard
> services via efi.systab->runtime but you also need the ability to call
> these UV bios functions, which are not accessible via efi.systab.

No, sorry.  I wasn't very clear there.  All of the standard EFI services
(get_time, get_variable, etc.) are still called through
efi.systab->runtime on UV.  It's only the special UV-specific function
pointer that is accessed through uv_systab, but, either way, we will
still need a slightly different macro to make that happen.

> Do you actually want the same environment setup and teardown to happen
> when calling efi.uv_systab ? Or are you simply trying to reuse the
> efi_call() implementation?

I was simply re-using the efi_call implementation.  Boris suggested that
I re-write this using the efi_call_virt macro, so I just went with that.
It all seems to work just fine, so I don't see much reason to stray away
from that implementation.  That being said, I'm obviously not a huge fun
of the code duplication across the macros.  I think there's probably a
way to minimize this, though I haven't quite worked out the best method
yet (ideas are welcome :)

> > This commit creates some new uv_* macros that are functionally
> > equivalent to the standard ones, with the exception of allowing us to
> > use a different function pointer.  I figure that we won't want to call
> > these uv_* in the end (we'll probably want something more generic), but
> > I thought I would get everyone's thoughts on how we might best reach
> > that goal instead of just trying to come up with a new implementation on
> > my own.
> > 
> > By itself, this commit does get our machines booting, but it needs the
> > small fix to the efi_call assembly code for our modules to work.
>  
> Could you provide some more details in the changelog on why your
> machine doesn't boot without this change?

Absolutely.  I wasn't sure exactly how much detail was necessary.  I'll
put a brief write-up of the original problem in the commit message for
the next version.

- Alex

Re: [PATCH 1/2] Create UV efi_call macros

[Alex Thorlton]

On Thu, May 12, 2016 at 10:17:39AM +0200, Ingo Molnar wrote:
> > Fine by me, although having a newline after arch_efi_call_virt_setup()
> > but not before arch_efi_call_virt_teardown() seems rather arbitrary
> 
> It's an oversight! :-)
> 
> #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;								\
> })
> 
> But if it's too segmented this is fine too:
> 
> #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;								\
> })

This makes sense to me.  I'll make sure to include something like this
in my next version of the patch.

Thanks, guys!

- Alex

Re: [PATCH 1/2] Create UV efi_call macros

[Matt Fleming]

On Mon, 16 May, at 05:58:40PM, Alex Thorlton wrote:
> 
> I was simply re-using the efi_call implementation.  Boris suggested that
> I re-write this using the efi_call_virt macro, so I just went with that.
> It all seems to work just fine, so I don't see much reason to stray away
> from that implementation.  That being said, I'm obviously not a huge fun
> of the code duplication across the macros.  I think there's probably a
> way to minimize this, though I haven't quite worked out the best method
> yet (ideas are welcome :)

The reason I'm pressing for details is that we have a related issue
with the EFI thunking code (CONFIG_EFI_MIXED), where the function
pointer we want to call isn't accessible via the EFI System Table, see
efi_thunk().

Well, technically it *is* accessible, you just can't dereference the
services at runtime because the pointers in the tables are not 64-bit.

But the same constraints exist for EFI thunk and UV code; given a
function pointer to execute that isn't in efi.systab, setup the EFI
runtime environment and call a custom ABI function.

I haven't tested this (or thought through all the implications), but
could you look at providing a table (or something) for mapping a
function name to a ptr,func pair, e.g.

	thunk_get_time:	runtime_services32(get_time), efi64_thunk
	thunk_set_time:	runtime_services32(set_time), efi64_thunk
	...
	uv_call_func: efi.uv_systab->function, uv_efi_call_virt

which we could use in arch_efi_call_virt()? That should give us much
less code duplication and hide all this inside arch/x86.

Re: [PATCH 1/2] Create UV efi_call macros

[Alex Thorlton]

On Tue, May 17, 2016 at 01:11:22PM +0100, Matt Fleming wrote:
> On Mon, 16 May, at 05:58:40PM, Alex Thorlton wrote:
> > 
> > I was simply re-using the efi_call implementation.  Boris suggested that
> > I re-write this using the efi_call_virt macro, so I just went with that.
> > It all seems to work just fine, so I don't see much reason to stray away
> > from that implementation.  That being said, I'm obviously not a huge fun
> > of the code duplication across the macros.  I think there's probably a
> > way to minimize this, though I haven't quite worked out the best method
> > yet (ideas are welcome :)
> 
> The reason I'm pressing for details is that we have a related issue
> with the EFI thunking code (CONFIG_EFI_MIXED), where the function
> pointer we want to call isn't accessible via the EFI System Table, see
> efi_thunk().
> 
> Well, technically it *is* accessible, you just can't dereference the
> services at runtime because the pointers in the tables are not 64-bit.
> 
> But the same constraints exist for EFI thunk and UV code; given a
> function pointer to execute that isn't in efi.systab, setup the EFI
> runtime environment and call a custom ABI function.

I took a look at this, and see what you mean.  You pass in the same
pointer to efi_thunk, which handles essentially the same setup
stuff as efi_call_virt (sync low mappings, disable interrupts, switch
page tables), sans a few of the finer details in
arch_efi_call_virt_setup.

The separate efi_thunk macro is necessary in this case, because you
need to use the efi64_thunk call, with your runtime_service32 massaged
pointer, instead of efi_call, with a pointer straight out of
systab->runtime. This is a similar scenario to ours, in that we
need uv_efi_call instead of efi_call, with our own pointer, instead of
systab->runtime.

The only difference here is that your efi64_thunk call needs a
slightly different setup/teardown than the regular efi_call, so you
need that efi_thunk to be hacked up a bit more (compared to
efi_call_virt) than my uv_efi_call_virt macro.

IINM, we could probably make up for this discrepancy by having a
different arch_efi_call_virt_setup/teardown for the !efi_is_native case
(not sure if that is a feasible idea, correct me if that's stupid).

> I haven't tested this (or thought through all the implications), but
> could you look at providing a table (or something) for mapping a
> function name to a ptr,func pair, e.g.
> 
> 	thunk_get_time:	runtime_services32(get_time), efi64_thunk
> 	thunk_set_time:	runtime_services32(set_time), efi64_thunk
> 	...
> 	uv_call_func: efi.uv_systab->function, uv_efi_call_virt
> 
> which we could use in arch_efi_call_virt()? That should give us much
> less code duplication and hide all this inside arch/x86.

This sounds like it could be a good way to handle this.  I will need to
think about it.  Unless someone can say for certain that we can use the
same arch_efi_call_virt_setup/teardown for your efi64_thunk call that we
use for efi_call, then we'll also have to take that into account, which
might make things uglier.  Not horrible, but more complicated.

I'm starting to play with this over here to see if I can get a cleaner
implementation working.

Let me know if you have other thoughts.  Thanks for the input!

- Alex