On Fri, 27 Dec 2019 at 19:08, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
>
> On Fri, Dec 27, 2019 at 12:51:56PM -0500, Arvind Sankar wrote:
> > On Thu, Dec 26, 2019 at 04:14:05PM +0100, Ard Biesheuvel wrote:
> > > The efi_call() wrapper used to invoke EFI runtime services serves
> > > a number of purposes:
> > > - realign the stack to 16 bytes
> > > - preserve FP register state
> > > - translate from SysV to MS calling convention.
> > >
> > > Preserving the FP register state is redundant in most cases, since
> > > efi_call() is almost always used from within the scope of a pair of
> > > kernel_fpu_begin()/_end() calls, with the exception of the early
> > > call to SetVirtualAddressMap() and the SGI UV support code. So let's
> > > add a pair of kernel_fpu_begin()/_end() calls there as well, and
> > > remove the unnecessary code from the assembly implementation of
> > > efi_call(), and only keep the pieces that deal with the stack
> > > alignment and the ABI translation.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > ---
> > > arch/x86/platform/efi/efi_64.c | 4 +++
> > > arch/x86/platform/efi/efi_stub_64.S | 36 ++------------------
> > > arch/x86/platform/uv/bios_uv.c | 7 ++--
> > > 3 files changed, 11 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index 03c2ed3c645c..3690df1d31c6 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -84,6 +84,7 @@ pgd_t * __init efi_call_phys_prolog(void)
> > >
> > > if (!efi_enabled(EFI_OLD_MEMMAP)) {
> > > efi_switch_mm(&efi_mm);
> > > + kernel_fpu_begin();
> > > return efi_mm.pgd;
> > > }
> > >
> > > @@ -141,6 +142,7 @@ pgd_t * __init efi_call_phys_prolog(void)
> > > }
> > >
> > > __flush_tlb_all();
> > > + kernel_fpu_begin();
> > > return save_pgd;
> > > out:
> > > efi_call_phys_epilog(save_pgd);
> > > @@ -158,6 +160,8 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
> > > p4d_t *p4d;
> > > pud_t *pud;
> > >
> > > + kernel_fpu_end();
> > > +
> > > if (!efi_enabled(EFI_OLD_MEMMAP)) {
> > > efi_switch_mm(efi_scratch.prev_mm);
> > > return;
> >
> > Does kernel_fpu_begin/kernel_fpu_end need to be outside the efi_switch_mm?
> >
> > If there's an error in efi_call_phys_prolog during the old memmap code,
> > it will call efi_call_phys_epilog without having called
> > kernel_fpu_begin, which will cause an unbalanced kernel_fpu_end. Looks
> > like the next step will be a panic anyway though.
>
> Do we even need to save/restore the fpu state at this point in boot? The
> mixed-mode code path doesn't appear to be saving/restoring the XMM
> registers during SetVirtualAddressMap.
That is an excellent question, and I was hoping Andy or Ingo could
shed some light on that.
I tested without and it booted fine, and it does seem to me that there
should be very little to preserve given how early this call happens
(from efi_enter_virtual_mode() which gets called from start_kernel())
