Re: [edk2-devel] [Patch 1/4] MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence()

["Liming Gao" <liming.gao@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Monday, April 29, 2019 7:15 PM
> To: devel@edk2.groups.io; brian.johnson@hpe.com; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch 1/4] MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence()
> 
> On 04/26/19 23:08, Brian J. Johnson wrote:
> > On 4/26/19 3:27 PM, Laszlo Ersek wrote:
> >> On 04/25/19 19:53, Michael D Kinney wrote:
> >>> Use CPUID in IA32 implementation of AsmLfence() to verify
> >>> that SSE2 is supported before using the LFENCE instruction.
> >>>
> >>> Cc: Liming Gao <liming.gao@intel.com>
> >>> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> >>> ---
> >>>   MdePkg/Library/BaseLib/Ia32/Lfence.nasm | 14 +++++++++++++-
> >>>   1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/MdePkg/Library/BaseLib/Ia32/Lfence.nasm
> >>> b/MdePkg/Library/BaseLib/Ia32/Lfence.nasm
> >>> index 44478be35f..0a60ae1d57 100644
> >>> --- a/MdePkg/Library/BaseLib/Ia32/Lfence.nasm
> >>> +++ b/MdePkg/Library/BaseLib/Ia32/Lfence.nasm
> >>> @@ -1,5 +1,5 @@
> >>>
> >>> ;------------------------------------------------------------------------------
> >>> ;
> >>> -; Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> >>> +; Copyright (c) 2018 - 2019, Intel Corporation. All rights
> >>> reserved.<BR>
> >>>   ; SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>   ;
> >>>   ; Module Name:
> >>> @@ -26,5 +26,17 @@
> >>>
> >>> ;------------------------------------------------------------------------------
> >>>
> >>>   global ASM_PFX(AsmLfence)
> >>>   ASM_PFX(AsmLfence):
> >>> +    ;
> >>> +    ; Use CPUID instruction (CPUID.01H:EDX.SSE2[bit 26] = 1) to test
> >>> whether the
> >>> +    ; processor supports SSE2 instruction.  Save/restore
> >>> non-volatile register
> >>> +    ; EBX that is modified by CPUID
> >>> +    ;
> >>> +    push    ebx
> >>> +    mov     eax, 1
> >>> +    cpuid
> >>> +    bt      edx, 26
> >>> +    jnc     Done
> >>>       lfence
> >>> +Done:
> >>> +    pop     ebx
> >>>       ret
> >>>
> >>
> >> The SDM seems to confirm that lfence depends on SSE2.
> >>
> >> However, that raises another question:
> >>
> >> AsmLfence() is used for implementing SpeculationBarrier() on IA32/X64.
> >> And so I wonder: the plaforms where lfence is unavailable, do they *not*
> >> need a speculation barrier at all, or do they need a *different*
> >> implementation?
> >>
> >> Thanks,
> >> Laszlo
> >
> > Several issues:
> >
> > This patch introduces stronger fencing than is required.  The SDM says,
> > "Reads or writes cannot be reordered with I/O instructions, locked
> > instructions, or serializing instructions." (vol 3a, sec 8.2.2.)  The
> > cpuid instruction is a "serializing instruction" (sec 8.3).  So the
> > cpuid is essentially a load fence plus a store fence (plus other
> > functionality, such as draining the memory queues.)  That makes the
> > lfence following it redundant.
> >
> > Cpuid is a fairly heavyweight instruction due to its serializing
> > behavior.  It provides the load fencing semantics of lfence, but can
> > introduce a significant performance hit if it's called often.  Lfence is
> > a lot lighter weight instruction.  So using cpuid in AsmLfence may make
> > it a lot slower than the caller expects.
> >
> > Also, skipping a fencing instruction if it's not supported doesn't seem
> > like the right approach in any case.  The caller expects AsmLfence to
> > provide certain fencing semantics... the implementation isn't free to
> > just do nothing (unless it's on an architecture where load fencing is
> > not required, since memory is always ordered.)  Plus, the routine is
> > called "AsmLfence", so I'd expect it to always use lfence, and cause an
> > exception if the instruction isn't implemented.  I'd think the callers
> > should be modified to call AsmLfence or routines using other
> > instructions (such as cpuid) to provide fencing according to the CPU
> > architecture they are on.
> 
> That appears the right approach to me -- let AsmLfence do what its name
> says, and let platforms pick the right implementation for
> SpeculationBarrier -- possibly at build time, possibly at boot time.
> 
> > Is there a way to make this a compile-time decision?  I haven't tried to
> > track down all the callers of AsmLfence....
> 
> Right now AsmLfence is only called from
> "MdePkg/Library/BaseLib/X86SpeculationBarrier.c".
> 
This is a good suggestion to decide it at build time. One PCD can be introduced to control its logic. 
> SpeculationBarrier() is called in SMM drivers / lib instances:
> - FaultTolerantWriteSmm, FaultTolerantWriteStandaloneMm
> - SmmLockBoxLib
> - VariableSmm, VariableStandaloneMm
> - PiSmmCpuDxeSmm
> 
> Given that there's a mode switch anyway, the runtime cost of a simple
> CPUID-based implementation might be tolerable.
> 
> Thanks,
> Laszlo
> 
>