> On Dec 28, 2019, at 2:13 AM, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>
> 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())
Unless you’re somehow calling it from interrupt context, I doubt saving FP regs is needed. Certainly kernel_fpu_begin() doesn’t do anything that matters if we’re in the (misnamed) init task. If you’re calling it really really early, there’s a different potential issue: FP might not be fully initialized. We could have CR0.TS still set, or we might not have all the various “OS supports such-and-such regs” flags set.
Does the UEFI spec explicitly state what FP state can be used by the EFI functions?
