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

Groups > linux.kernel > #1399472 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] Fix EFI runtime calls on SGI UV Alex Thorlton <athorlton@sgi.com> - 2016-05-11 22:00 +0200
    [PATCH 2/2] Fix efi_call Alex Thorlton <athorlton@sgi.com> - 2016-05-11 22:00 +0200
      Re: [PATCH 2/2] Fix efi_call Ingo Molnar <mingo@kernel.org> - 2016-05-12 08:50 +0200
        Re: [PATCH 2/2] Fix efi_call Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-12 13:50 +0200
        Re: [PATCH 2/2] Fix efi_call Alex Thorlton <athorlton@sgi.com> - 2016-05-16 18:30 +0200
      Re: [PATCH 2/2] Fix efi_call Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-12 13:50 +0200
        Re: [PATCH 2/2] Fix efi_call Alex Thorlton <athorlton@sgi.com> - 2016-05-16 18:30 +0200

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

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

[toc] | [next] | [standalone]

#1399475 — [PATCH 2/2] Fix efi_call

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

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

#1399693 — Re: [PATCH 2/2] Fix efi_call

* 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.

> 
> 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)

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

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

#1399955 — Re: [PATCH 2/2] Fix efi_call

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.

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

#1401606 — Re: [PATCH 2/2] Fix efi_call

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

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

#1399953 — Re: [PATCH 2/2] Fix efi_call

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@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)

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

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

#1401612 — Re: [PATCH 2/2] Fix efi_call

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web