* [RFC PATCH 0/2] Fix EFI runtime calls on SGI UV @ 2016-05-11 19:55 Alex Thorlton 2016-05-11 19:55 ` [PATCH 1/2] Create UV efi_call macros Alex Thorlton 2016-05-11 19:55 ` [PATCH 2/2] Fix efi_call Alex Thorlton 0 siblings, 2 replies; 16+ messages in thread From: Alex Thorlton @ 2016-05-11 19:55 UTC (permalink / raw) To: linux-kernel Cc: Alex Thorlton, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-efi 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] Create UV efi_call macros 2016-05-11 19:55 [RFC PATCH 0/2] Fix EFI runtime calls on SGI UV Alex Thorlton @ 2016-05-11 19:55 ` Alex Thorlton [not found] ` <1462996545-98387-2-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> 2016-05-12 12:06 ` Matt Fleming 2016-05-11 19:55 ` [PATCH 2/2] Fix efi_call Alex Thorlton 1 sibling, 2 replies; 16+ messages in thread From: Alex Thorlton @ 2016-05-11 19:55 UTC (permalink / raw) To: linux-kernel Cc: Alex Thorlton, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-efi 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
[parent not found: <1462996545-98387-2-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org>]
* Re: [PATCH 1/2] Create UV efi_call macros [not found] ` <1462996545-98387-2-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> @ 2016-05-12 6:46 ` Ingo Molnar [not found] ` <20160512064606.GA30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Ingo Molnar @ 2016-05-12 6:46 UTC (permalink / raw) To: Alex Thorlton Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA * 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20160512064606.GA30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 1/2] Create UV efi_call macros [not found] ` <20160512064606.GA30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-05-12 7:35 ` Ard Biesheuvel [not found] ` <CAKv+Gu8Z0faffrN8Jnz9fQPkyn6K69cFaRD348w+m_Lv4Jgynw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Ard Biesheuvel @ 2016-05-12 7:35 UTC (permalink / raw) To: Ingo Molnar Cc: Alex Thorlton, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CAKv+Gu8Z0faffrN8Jnz9fQPkyn6K69cFaRD348w+m_Lv4Jgynw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/2] Create UV efi_call macros [not found] ` <CAKv+Gu8Z0faffrN8Jnz9fQPkyn6K69cFaRD348w+m_Lv4Jgynw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2016-05-12 8:17 ` Ingo Molnar [not found] ` <20160512081739.GA25826-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Ingo Molnar @ 2016-05-12 8:17 UTC (permalink / raw) To: Ard Biesheuvel Cc: Alex Thorlton, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA * 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20160512081739.GA25826-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 1/2] Create UV efi_call macros [not found] ` <20160512081739.GA25826-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-05-16 23:00 ` Alex Thorlton 0 siblings, 0 replies; 16+ messages in thread From: Alex Thorlton @ 2016-05-16 23:00 UTC (permalink / raw) To: Ingo Molnar Cc: Ard Biesheuvel, Alex Thorlton, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] Create UV efi_call macros 2016-05-11 19:55 ` [PATCH 1/2] Create UV efi_call macros Alex Thorlton [not found] ` <1462996545-98387-2-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> @ 2016-05-12 12:06 ` Matt Fleming 2016-05-16 22:58 ` Alex Thorlton 1 sibling, 1 reply; 16+ messages in thread From: Matt Fleming @ 2016-05-12 12:06 UTC (permalink / raw) To: Alex Thorlton Cc: linux-kernel, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-efi, Mark Rutland (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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] Create UV efi_call macros 2016-05-12 12:06 ` Matt Fleming @ 2016-05-16 22:58 ` Alex Thorlton [not found] ` <20160516225840.GL98477-7ppMa7wkY9tKToyKb8PD+Zs2JHu2awxn0E9HWUfgJXw@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Alex Thorlton @ 2016-05-16 22:58 UTC (permalink / raw) To: Matt Fleming Cc: Alex Thorlton, linux-kernel, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-efi, Mark Rutland 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20160516225840.GL98477-7ppMa7wkY9tKToyKb8PD+Zs2JHu2awxn0E9HWUfgJXw@public.gmane.org>]
* Re: [PATCH 1/2] Create UV efi_call macros [not found] ` <20160516225840.GL98477-7ppMa7wkY9tKToyKb8PD+Zs2JHu2awxn0E9HWUfgJXw@public.gmane.org> @ 2016-05-17 12:11 ` Matt Fleming [not found] ` <20160517121122.GC21993-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Matt Fleming @ 2016-05-17 12:11 UTC (permalink / raw) To: Alex Thorlton Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA, Mark Rutland 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20160517121122.GC21993-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]
* Re: [PATCH 1/2] Create UV efi_call macros [not found] ` <20160517121122.GC21993-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> @ 2016-05-17 20:14 ` Alex Thorlton 0 siblings, 0 replies; 16+ messages in thread From: Alex Thorlton @ 2016-05-17 20:14 UTC (permalink / raw) To: Matt Fleming Cc: Alex Thorlton, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA, Mark Rutland 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] Fix efi_call 2016-05-11 19:55 [RFC PATCH 0/2] Fix EFI runtime calls on SGI UV Alex Thorlton 2016-05-11 19:55 ` [PATCH 1/2] Create UV efi_call macros Alex Thorlton @ 2016-05-11 19:55 ` Alex Thorlton [not found] ` <1462996545-98387-3-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> 1 sibling, 1 reply; 16+ messages in thread From: Alex Thorlton @ 2016-05-11 19:55 UTC (permalink / raw) To: linux-kernel Cc: Alex Thorlton, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-efi 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
[parent not found: <1462996545-98387-3-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org>]
* Re: [PATCH 2/2] Fix efi_call [not found] ` <1462996545-98387-3-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> @ 2016-05-12 6:48 ` Ingo Molnar 2016-05-12 11:43 ` Matt Fleming [not found] ` <20160512064835.GB30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-12 11:41 ` Matt Fleming 1 sibling, 2 replies; 16+ messages in thread From: Ingo Molnar @ 2016-05-12 6:48 UTC (permalink / raw) To: Alex Thorlton Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA * 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] Fix efi_call 2016-05-12 6:48 ` Ingo Molnar @ 2016-05-12 11:43 ` Matt Fleming [not found] ` <20160512064835.GB30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 1 sibling, 0 replies; 16+ messages in thread From: Matt Fleming @ 2016-05-12 11:43 UTC (permalink / raw) To: Ingo Molnar Cc: Alex Thorlton, linux-kernel, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-efi 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20160512064835.GB30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 2/2] Fix efi_call [not found] ` <20160512064835.GB30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-05-16 16:24 ` Alex Thorlton 0 siblings, 0 replies; 16+ messages in thread From: Alex Thorlton @ 2016-05-16 16:24 UTC (permalink / raw) To: Ingo Molnar Cc: Alex Thorlton, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Matt Fleming, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] Fix efi_call [not found] ` <1462996545-98387-3-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> 2016-05-12 6:48 ` Ingo Molnar @ 2016-05-12 11:41 ` Matt Fleming [not found] ` <20160512114149.GD2728-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 1 sibling, 1 reply; 16+ messages in thread From: Matt Fleming @ 2016-05-12 11:41 UTC (permalink / raw) To: Alex Thorlton Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20160512114149.GD2728-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]
* Re: [PATCH 2/2] Fix efi_call [not found] ` <20160512114149.GD2728-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> @ 2016-05-16 16:25 ` Alex Thorlton 0 siblings, 0 replies; 16+ messages in thread From: Alex Thorlton @ 2016-05-16 16:25 UTC (permalink / raw) To: Matt Fleming Cc: Alex Thorlton, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Dimitri Sivanich, Russ Anderson, Mike Travis, Borislav Petkov, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86-DgEjT+Ai2ygdnm+yROfE0A, linux-efi-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-05-17 20:14 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-11 19:55 [RFC PATCH 0/2] Fix EFI runtime calls on SGI UV Alex Thorlton 2016-05-11 19:55 ` [PATCH 1/2] Create UV efi_call macros Alex Thorlton [not found] ` <1462996545-98387-2-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> 2016-05-12 6:46 ` Ingo Molnar [not found] ` <20160512064606.GA30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-12 7:35 ` Ard Biesheuvel [not found] ` <CAKv+Gu8Z0faffrN8Jnz9fQPkyn6K69cFaRD348w+m_Lv4Jgynw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-05-12 8:17 ` Ingo Molnar [not found] ` <20160512081739.GA25826-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-16 23:00 ` Alex Thorlton 2016-05-12 12:06 ` Matt Fleming 2016-05-16 22:58 ` Alex Thorlton [not found] ` <20160516225840.GL98477-7ppMa7wkY9tKToyKb8PD+Zs2JHu2awxn0E9HWUfgJXw@public.gmane.org> 2016-05-17 12:11 ` Matt Fleming [not found] ` <20160517121122.GC21993-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 2016-05-17 20:14 ` Alex Thorlton 2016-05-11 19:55 ` [PATCH 2/2] Fix efi_call Alex Thorlton [not found] ` <1462996545-98387-3-git-send-email-athorlton-sJ/iWh9BUns@public.gmane.org> 2016-05-12 6:48 ` Ingo Molnar 2016-05-12 11:43 ` Matt Fleming [not found] ` <20160512064835.GB30717-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-05-16 16:24 ` Alex Thorlton 2016-05-12 11:41 ` Matt Fleming [not found] ` <20160512114149.GD2728-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 2016-05-16 16:25 ` Alex Thorlton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).