public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Brian J. Johnson" <brian.johnson@hpe.com>
To: devel@edk2.groups.io, lersek@redhat.com, michael.d.kinney@intel.com
Cc: Liming Gao <liming.gao@intel.com>
Subject: Re: [edk2-devel] [Patch 1/4] MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence()
Date: Fri, 26 Apr 2019 16:08:04 -0500	[thread overview]
Message-ID: <7b0ca337-3642-2070-0fff-515704a24bdf@hpe.com> (raw)
In-Reply-To: <cf4dc6a5-f064-3030-a4a0-a706b2dbff41@redhat.com>

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.

Is there a way to make this a compile-time decision?  I haven't tried to 
track down all the callers of AsmLfence....

Thanks,
-- 
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise


  reply	other threads:[~2019-04-26 21:08 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-25 17:53 [Patch 0/4] Resolve Quark build and boot issues Michael D Kinney
2019-04-25 17:53 ` [Patch 1/4] MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence() Michael D Kinney
2019-04-26 20:27   ` [edk2-devel] " Laszlo Ersek
2019-04-26 21:08     ` Brian J. Johnson [this message]
2019-04-29 11:15       ` Laszlo Ersek
2019-04-29 14:09         ` Liming Gao
2019-04-29 18:23           ` Michael D Kinney
2019-04-30 10:03             ` Laszlo Ersek
2019-04-28  8:28   ` Liming Gao
2019-04-25 17:53 ` [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core Michael D Kinney
2019-04-25 18:07   ` Ni, Ray
2019-04-26  0:04   ` Dong, Eric
2019-04-26 19:24   ` Laszlo Ersek
2019-04-29 17:11     ` [edk2-devel] " Michael D Kinney
2019-04-30 10:02       ` Laszlo Ersek
2019-04-30 15:21         ` Michael D Kinney
2019-04-25 17:53 ` [Patch 3/4] QuarkSocPkg/SmmAccessDxe: Set region to UC on SMRAM close Michael D Kinney
2019-04-25 21:40   ` Steele, Kelly
2019-04-25 17:53 ` [Patch 4/4] QuarkPlatformPkg/PlatformInit: Resolve ResetSystemLib name collision Michael D Kinney
2019-04-25 21:40   ` Steele, Kelly
2019-04-28  8:25   ` [edk2-devel] " Liming Gao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7b0ca337-3642-2070-0fff-515704a24bdf@hpe.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox