Re: [PATCH v2 10/21] efi/libstub/x86: avoid thunking for native firmware calls

From: Hans de Goede <hdegoede@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ard Biesheuvel <ardb@kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	Matthew Garrett <matthewgarrett@google.com>,
	Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Arvind Sankar <nivedita@alum.mit.edu>
Subject: Re: [PATCH v2 10/21] efi/libstub/x86: avoid thunking for native firmware calls
Date: Mon, 23 Dec 2019 12:49:48 +0100	[thread overview]
Message-ID: <c82d02c3-c6e7-70af-37ad-5a8209ab8f22@redhat.com> (raw)
In-Reply-To: <CAKv+Gu8gWLw5-LkKwU0reUcreLyC8s1hQSzrR__-myuHrdVr7Q@mail.gmail.com>

Hi Ard,

On 22-12-2019 13:02, Ard Biesheuvel wrote:
> On Sat, 21 Dec 2019 at 22:22, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ard,
>>
>> On 18-12-2019 18:01, Ard Biesheuvel wrote:
>>> We use special wrapper routines to invoke firmware services in the
>>> native case as well as the mixed mode case. For mixed mode, the need
>>> is obvious, but for the native cases, we can simply rely on the
>>> compiler to generate the indirect call, given that GCC now has
>>> support for the MS calling convention (and has had it for quite some
>>> time now). Note that on i386, the decompressor and the EFI stub are not
>>> built with -mregparm=3 like the rest of the i386 kernel, so we can
>>> safely allow the compiler to emit the indirect calls here as well.
>>>
>>> So drop all the wrappers and indirection, and switch to either native
>>> calls, or direct calls into the thunk routine for mixed mode.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>
>> I'm afraid that this patch breaks the boot on one of my machines.
>>
>> Specifically this patch breaks my GDP pocket machine. This is a Cherry
>> Trail device with a 64 UEFI running a 64 bit kernel build.
>>
>> As soon as I cherry pick this patch into my personal 5.5.0-rc2 based
>> tree, the GPD pocket stops booting and it stop so early on that I get 0
>> debug output. I guess I could try adding a few pr_efi_err calls
>> and see if those still do something.
>>
>> I noticed that you have made some changes to this patch, I've
>> tried updating it to the version from your efistub-x86-cleanup-v3
>> branch, commit id a37d90a2c570a25926fd1645482cb9f3c1d042a0
>> and I have also cherry-picked the latest version of all preceding
>> commits, unfortunately even with the new version, the GPD pocket
>> still hangs at boot.
>>
>> Unfortunately the nature of this patch makes it hard to figure
>> out the root cause of this issue...
>>
>> I've also tried another Cherry Trail device with 64 bit UEFI and
>> that does not suffer from this problem.
>>
> 
> Thanks Hans.
> 
> There are a number of things that change in the way the calls are
> made, but the most obvious thing to check is whether the stack needs
> to be aligned, since that is no longer being done.
> 
> If you have time to experiment a bit more, could you check whether
> doing 'and $~0xf %rsp' before 'call efi_main' in the .S stub code for
> x86_64 makes a difference?

Ok, so I made this change on top of a37d90a2c570a25926fd1645482cb9f3c1d042a0
(the last "efi/libstub/x86: avoid thunking for native firmware calls" version
I tried before reporting this problem) :

--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -464,6 +464,7 @@ SYM_FUNC_START(efi_pe_entry)

  handover_entry:
         movq    efi_config(%rip), %rdi
+       and     $~0xf, %rsp
         call    efi_main
         movq    %rax,%rsi
         cmpq    $0,%rax

And that does the trick, the GPD pocket boots successfully with commit
a37d90a2c570a25926fd1645482cb9f3c1d042a0 + the above change.

So it looks like your first hunch on how to fix this is correct :)

Regards,

Hans

