[Patch V2 0/6] Resolve Quark build and boot issues

[Patch V2 0/6] Resolve Quark build and boot issues

[Michael D Kinney]

New in V2
=========
* Add PcdSpeculationBarrierType to select between LFENCE, CPUID, and no
  operation in the x86 implementation of the BaseLib function
  SpeculationBarrier().
* Set PcdSpeculationBarrierType to CPUID on Quark platforms.

This series of patches resolves a few issues with building
and booting Quark platforms.

* Resolve name collision from API added to ResetSystemLib
* Set SMRAM region to UC when SMRAM region is closed
* Do no use LFENCE if CPU does not support SSE2
* Avoid MSR_IA32_APIC_BASE if there is only one CPU

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>

Michael D Kinney (6):
  MdePkg: Add PcdSpeculationBarrierType
  MdePkg/BaseLib: Use PcdSpeculationBarrierType
  QuarkPlatformPkg: Set PcdSpeculationBarrierType to CPUID
  UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core
  QuarkSocPkg/SmmAccessDxe: Set region to UC on SMRAM close
  QuarkPlatformPkg/PlatformInit: Resolve ResetSystemLib name collision

 MdePkg/Library/BaseLib/BaseLib.inf             |  1 +
 MdePkg/Library/BaseLib/X86SpeculationBarrier.c |  8 ++++++--
 MdePkg/MdePkg.dec                              |  9 +++++++++
 MdePkg/MdePkg.uni                              |  8 ++++++++
 .../Platform/Pei/PlatformInit/MemoryCallback.c |  6 +++---
 .../Pei/PlatformInit/PlatformEarlyInit.h       |  4 ++--
 QuarkPlatformPkg/Quark.dsc                     |  7 ++++++-
 QuarkPlatformPkg/QuarkMin.dsc                  |  5 +++++
 .../Smm/Dxe/SmmAccessDxe/SmmAccess.inf         |  3 ++-
 .../Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c     | 18 +++++++++++++++++-
 .../Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h     |  3 ++-
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c        | 15 ++++++++++++++-
 12 files changed, 75 insertions(+), 12 deletions(-)

-- 
2.21.0.windows.1

[Patch V2 1/6] MdePkg: Add PcdSpeculationBarrierType

[Michael D Kinney]

Add gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType that
uses the PCD type FixedAtBuild.  This performs a build time
selection for the type of speculation barrier to use in the
BaseLib function SpeculationBarrier().  The recommended
speculation barrier for x86 is LFENCE and this is the default
value for this PCD.  x86 CPUs that do not support LFENCE must
select one of the other supported values which includes CPUID
and nothing.

Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdePkg/MdePkg.dec | 9 +++++++++
 MdePkg/MdePkg.uni | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index e2ea8fff66..28d4a966c2 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2062,6 +2062,15 @@ [PcdsFixedAtBuild]
   # @Prompt Enable control flow enforcement.
   gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask|0x0|UINT32|0x30001017
 
+  ## Indicates the type of instruction sequence to use for a speculation
+  #  barrier.  The default instruction sequence is LFENCE.<BR><BR>
+  #   0x00 - No operation.<BR>
+  #   0x01 - LFENCE (IA32/X64).<BR>
+  #   0x02 - CPUID  (IA32/X64).<BR>
+  #   Other - reserved
+  # @Prompt Speculation Barrier Type.
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01|UINT8|0x30001018
+
 [PcdsFixedAtBuild,PcdsPatchableInModule]
   ## Indicates the maximum length of unicode string used in the following
   #  BaseLib functions: StrLen(), StrSize(), StrCmp(), StrnCmp(), StrCpy(), StrnCpy()<BR><BR>
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index c359bb4b5b..5c1fa24065 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -149,6 +149,14 @@
                                                                                                   " BIT0 - SMM CET Shadow Stack is enabled.<BR>\n"
                                                                                                   " Other - reserved"
 
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_PROMPT  #language en-US "Speculation Barrier Type."
+
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_HELP  #language en-US  "Indicates the type of instruction sequence to use for a speculation.barrier.  The default instruction sequence is LFENCE.<BR><BR>\n"
+                                                                                      "0x00 - No operation.<BR>\n"
+                                                                                      "0x01 - LFENCE (IA32/X64).<BR>\n"
+                                                                                      "0x02 - CPUID  (IA32/X64).<BR>\n"
+                                                                                      "Other - reserved"
+
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLength_PROMPT  #language en-US "Maximum Length of Ascii String"
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLength_HELP  #language en-US "Sets the maximum number of ASCII characters used for string functions.  This affects the following BaseLib functions: AsciiStrLen(), AsciiStrSize(), AsciiStrCmp(), AsciiStrnCmp(), AsciiStrCpy(), AsciiStrnCpy(). <BR><BR>\n"
-- 
2.21.0.windows.1

[Patch V2 2/6] MdePkg/BaseLib: Use PcdSpeculationBarrierType

[Michael D Kinney]

Use PcdSpeculationBarrierType in the x86 implementation
of SpeculationBarrier() to select between AsmLfence(),
AsmCpuid(), and no operation.

Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdePkg/Library/BaseLib/BaseLib.inf             | 1 +
 MdePkg/Library/BaseLib/X86SpeculationBarrier.c | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 533e83e0b2..3586beb0ab 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -394,6 +394,7 @@ [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength     ## SOMETIMES_CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## SOMETIMES_CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType       ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
diff --git a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
index 8e5f983bb8..b28fd8de9b 100644
--- a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
+++ b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
@@ -1,7 +1,7 @@
 /** @file
   SpeculationBarrier() function for IA32 and x64.
 
-  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
 
@@ -22,5 +22,9 @@ SpeculationBarrier (
   VOID
   )
 {
-  AsmLfence ();
+  if (PcdGet8 (PcdSpeculationBarrierType) == 0x01) {
+    AsmLfence ();
+  } else if (PcdGet8 (PcdSpeculationBarrierType) == 0x02) {
+    AsmCpuid (0x01, NULL, NULL, NULL, NULL);
+  }
 }
-- 
2.21.0.windows.1

[Patch V2 3/6] QuarkPlatformPkg: Set PcdSpeculationBarrierType to CPUID

[Michael D Kinney]

Set PcdSpeculationBarrierType to use CPUID instead of the
default LFENCE in the BaseLib function SpeculationBarrier().
LFENCE requires SSE2, and Quark platforms do not support
SSE2.

Cc: Kelly Steele <kelly.steele@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 QuarkPlatformPkg/Quark.dsc    | 7 ++++++-
 QuarkPlatformPkg/QuarkMin.dsc | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/QuarkPlatformPkg/Quark.dsc b/QuarkPlatformPkg/Quark.dsc
index 422fd9cf8d..96ddc1565a 100644
--- a/QuarkPlatformPkg/Quark.dsc
+++ b/QuarkPlatformPkg/Quark.dsc
@@ -2,7 +2,7 @@
 # Clanton Peak CRB platform with 32-bit DXE for 4MB/8MB flash devices.
 #
 # This package provides Clanton Peak CRB platform specific modules.
-# Copyright (c) 2013 - 2018 Intel Corporation.
+# Copyright (c) 2013 - 2019 Intel Corporation.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -448,6 +448,11 @@ [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdRecoveryFileName|L"QUARKREC.Cap"
 !endif
 
+  #
+  # Quark does not support LFENCE.  Use CPUID as speculation barrier
+  #
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x02
+
 [PcdsPatchableInModule]
   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x803000C7
   gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
diff --git a/QuarkPlatformPkg/QuarkMin.dsc b/QuarkPlatformPkg/QuarkMin.dsc
index 00e2febb54..8ca75bc474 100644
--- a/QuarkPlatformPkg/QuarkMin.dsc
+++ b/QuarkPlatformPkg/QuarkMin.dsc
@@ -406,6 +406,11 @@ [PcdsFixedAtBuild]
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand|FALSE
 
+  #
+  # Quark does not support LFENCE.  Use CPUID as speculation barrier
+  #
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x02
+
 [PcdsPatchableInModule]
   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x803000C7
   gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
-- 
2.21.0.windows.1

[Patch V2 4/6] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Michael D Kinney]

Avoid access to MSR_IA32_APIC_BASE that may not be supported
on single core CPUs.  If PcdCpuMaxLogicalProcessorNumber is 1,
then there is only one CPU that must be the BSP.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 35dff91fd2..5488049c08 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -1,7 +1,7 @@
 /** @file
   MP initialize support functions for PEI phase.
 
-  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -101,6 +101,19 @@ GetCpuMpData (
   MSR_IA32_APIC_BASE_REGISTER  ApicBaseMsr;
   IA32_DESCRIPTOR              Idtr;
 
+  //
+  // If there is only 1 CPU, then it must be the BSP.  This avoids an access to
+  // MSR_IA32_APIC_BASE that may not be supported on single core CPUs.
+  //
+  if (PcdGet32 (PcdCpuMaxLogicalProcessorNumber) == 1) {
+    CpuMpData = GetCpuMpDataFromGuidedHob ();
+    ASSERT (CpuMpData != NULL);
+    return CpuMpData;
+  }
+
+  //
+  // Otherwise use MSR_IA32_APIC_BASE to determine if the CPU is BSP or AP.
+  //
   ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
   if (ApicBaseMsr.Bits.BSP == 1) {
     CpuMpData = GetCpuMpDataFromGuidedHob ();
-- 
2.21.0.windows.1

[Patch V2 5/6] QuarkSocPkg/SmmAccessDxe: Set region to UC on SMRAM close

[Michael D Kinney]

The following commit removed the unconditional UC setting
just prior to closing the SMRAM region.  This is a correct
change for most platforms.

https://github.com/tianocore/edk2/commit/bfc87aa78e77ed15b09d1b4499c5eab63e8842bb

The Quark platforms still require this UC setting, so move
the UC setting into the Quark specific SMM Access Protocol
when the Close() service is called.

Cc: Kelly Steele <kelly.steele@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Kelly Steele <kelly.steele@intel.com>
---
 .../Smm/Dxe/SmmAccessDxe/SmmAccess.inf         |  3 ++-
 .../Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c     | 18 +++++++++++++++++-
 .../Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h     |  3 ++-
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccess.inf b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccess.inf
index db916f686a..405e9eb7fd 100644
--- a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccess.inf
+++ b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccess.inf
@@ -1,7 +1,7 @@
 ## @file
 # Component description file for SmmAccess module
 #
-# Copyright (c) 2013-2015 Intel Corporation.
+# Copyright (c) 2013-2019 Intel Corporation.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -34,6 +34,7 @@ [LibraryClasses]
   S3BootScriptLib
   UefiDriverEntryPoint
   UefiBootServicesTableLib
+  DxeServicesTableLib
   PcdLib
   SmmLib
 
diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c
index 6148dea1b4..830f8b83c3 100644
--- a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c
+++ b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c
@@ -2,7 +2,7 @@
 This is the driver that publishes the SMM Access Protocol
 instance for the Tylersburg chipset.
 
-Copyright (c) 2013-2015 Intel Corporation.
+Copyright (c) 2013-2019 Intel Corporation.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -221,6 +221,7 @@ Returns:
 
 --*/
 {
+  EFI_STATUS              Status;
   SMM_ACCESS_PRIVATE_DATA *SmmAccess;
   BOOLEAN                 OpenState;
   UINTN                   Index;
@@ -239,6 +240,21 @@ Returns:
     return EFI_DEVICE_ERROR;
   }
 
+  //
+  // Reset SMRAM cacheability to UC
+  //
+  for (Index = 0; Index < mSmmAccess.NumberRegions; Index++) {
+    DEBUG ((DEBUG_INFO, "SmmAccess->Close: Set to UC Base=%016lx  Size=%016lx\n", SmmAccess->SmramDesc[Index].CpuStart, SmmAccess->SmramDesc[Index].PhysicalSize));
+    Status = gDS->SetMemorySpaceAttributes(
+                    SmmAccess->SmramDesc[Index].CpuStart,
+                    SmmAccess->SmramDesc[Index].PhysicalSize,
+                    EFI_MEMORY_UC
+                    );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_WARN, "SmmAccess: Failed to reset SMRAM window to EFI_MEMORY_UC\n"));
+    }
+  }
+
   //
   // Close TSEG
   //
diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h
index 80f73ba0e3..aca169d3e2 100644
--- a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h
+++ b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h
@@ -3,7 +3,7 @@ Header file for SMM Access Driver.
 
 This file includes package header files, library classes and protocol, PPI & GUID definitions.
 
-Copyright (c) 2013-2015 Intel Corporation.
+Copyright (c) 2013-2019 Intel Corporation.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
@@ -21,6 +21,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BaseMemoryLib.h>
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/DxeServicesTableLib.h>
 #include <Library/PcdLib.h>
 
 //
-- 
2.21.0.windows.1

[Patch V2 6/6] QuarkPlatformPkg/PlatformInit: Resolve ResetSystemLib name collision

[Michael D Kinney]

Change function name from ResetSystem() to PlatformResetSystem()
to resolve name collision with ResetSystemLib.

Cc: Kelly Steele <kelly.steele@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Kelly Steele <kelly.steele@intel.com>
---
 QuarkPlatformPkg/Platform/Pei/PlatformInit/MemoryCallback.c | 6 +++---
 .../Platform/Pei/PlatformInit/PlatformEarlyInit.h           | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/QuarkPlatformPkg/Platform/Pei/PlatformInit/MemoryCallback.c b/QuarkPlatformPkg/Platform/Pei/PlatformInit/MemoryCallback.c
index cc2a6674d0..cfdcba8e02 100644
--- a/QuarkPlatformPkg/Platform/Pei/PlatformInit/MemoryCallback.c
+++ b/QuarkPlatformPkg/Platform/Pei/PlatformInit/MemoryCallback.c
@@ -7,7 +7,7 @@ following action is performed in this file,
   4. Set MTRR for PEI
   5. Create FV HOB and Flash HOB
 
-Copyright (c) 2013 - 2016, Intel Corporation.
+Copyright (c) 2013 - 2019, Intel Corporation.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -20,7 +20,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 extern EFI_PEI_PPI_DESCRIPTOR mPpiStall[];
 
-EFI_PEI_RESET_PPI mResetPpi = { ResetSystem };
+EFI_PEI_RESET_PPI mResetPpi = { PlatformResetSystem };
 
 EFI_PEI_PPI_DESCRIPTOR mPpiList[1] = {
   {
@@ -40,7 +40,7 @@ EFI_PEI_PPI_DESCRIPTOR mPpiList[1] = {
 **/
 EFI_STATUS
 EFIAPI
-ResetSystem (
+PlatformResetSystem (
   IN CONST EFI_PEI_SERVICES          **PeiServices
   )
 {
diff --git a/QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.h b/QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.h
index 6792538d42..84def44717 100644
--- a/QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.h
+++ b/QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.h
@@ -1,7 +1,7 @@
 /** @file
 The header file of Platform PEIM.
 
-Copyright (c) 2013 Intel Corporation.
+Copyright (c) 2013 - 2019 Intel Corporation.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -59,7 +59,7 @@ UpdateBootMode (
 **/
 EFI_STATUS
 EFIAPI
-ResetSystem (
+PlatformResetSystem (
   IN CONST EFI_PEI_SERVICES          **PeiServices
   );
 
-- 
2.21.0.windows.1

Re: [Patch V2 4/6] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Laszlo Ersek]

Hi Mike,

On 04/30/19 03:30, Michael D Kinney wrote:
> Avoid access to MSR_IA32_APIC_BASE that may not be supported
> on single core CPUs.  If PcdCpuMaxLogicalProcessorNumber is 1,
> then there is only one CPU that must be the BSP.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 35dff91fd2..5488049c08 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    MP initialize support functions for PEI phase.
>  
> -  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -101,6 +101,19 @@ GetCpuMpData (
>    MSR_IA32_APIC_BASE_REGISTER  ApicBaseMsr;
>    IA32_DESCRIPTOR              Idtr;
>  
> +  //
> +  // If there is only 1 CPU, then it must be the BSP.  This avoids an access to
> +  // MSR_IA32_APIC_BASE that may not be supported on single core CPUs.
> +  //
> +  if (PcdGet32 (PcdCpuMaxLogicalProcessorNumber) == 1) {

this PcdGet32() call has the same issue as in v1. It is unsafe because:

- if the platform makes PcdCpuMaxLogicalProcessorNumber a dynamic or
dynamic-ex PCD, then the above call will result in a PPI member function
invocation,

- and said invocation may be executed on an AP, not necessarily on the BSP.

But, APs must not call PPI member functions, to my knowledge.

We can certainly use a PCD here, but both UefiCpuPkg.dec, and the code
added here, must prevent platforms from making that PCD dynamic or
dynamic-ex.

Thanks
Laszlo

> +    CpuMpData = GetCpuMpDataFromGuidedHob ();
> +    ASSERT (CpuMpData != NULL);
> +    return CpuMpData;
> +  }
> +
> +  //
> +  // Otherwise use MSR_IA32_APIC_BASE to determine if the CPU is BSP or AP.
> +  //
>    ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
>    if (ApicBaseMsr.Bits.BSP == 1) {
>      CpuMpData = GetCpuMpDataFromGuidedHob ();
> 

Re: [edk2-devel] [Patch V2 1/6] MdePkg: Add PcdSpeculationBarrierType

[Laszlo Ersek]

On 04/30/19 03:30, Michael D Kinney wrote:
> Add gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType that
> uses the PCD type FixedAtBuild.  This performs a build time
> selection for the type of speculation barrier to use in the
> BaseLib function SpeculationBarrier().  The recommended
> speculation barrier for x86 is LFENCE and this is the default
> value for this PCD.  x86 CPUs that do not support LFENCE must
> select one of the other supported values which includes CPUID
> and nothing.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdePkg/MdePkg.dec | 9 +++++++++
>  MdePkg/MdePkg.uni | 8 ++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index e2ea8fff66..28d4a966c2 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2062,6 +2062,15 @@ [PcdsFixedAtBuild]
>    # @Prompt Enable control flow enforcement.
>    gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask|0x0|UINT32|0x30001017
>  
> +  ## Indicates the type of instruction sequence to use for a speculation
> +  #  barrier.  The default instruction sequence is LFENCE.<BR><BR>
> +  #   0x00 - No operation.<BR>
> +  #   0x01 - LFENCE (IA32/X64).<BR>
> +  #   0x02 - CPUID  (IA32/X64).<BR>
> +  #   Other - reserved
> +  # @Prompt Speculation Barrier Type.
> +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01|UINT8|0x30001018
> +
>  [PcdsFixedAtBuild,PcdsPatchableInModule]
>    ## Indicates the maximum length of unicode string used in the following
>    #  BaseLib functions: StrLen(), StrSize(), StrCmp(), StrnCmp(), StrCpy(), StrnCpy()<BR><BR>

In MdePkg.dec, we have:
- [Includes.X64]
- [LibraryClasses.X64]
- [Guids.X64]

but no PCD declarations that are architecture-specific. Is that
intentional? Because, this PCD could be a good candidate for "IA32/X64
only". (Looking at the next patch too.)

But, that's just my curiosity.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> index c359bb4b5b..5c1fa24065 100644
> --- a/MdePkg/MdePkg.uni
> +++ b/MdePkg/MdePkg.uni
> @@ -149,6 +149,14 @@
>                                                                                                    " BIT0 - SMM CET Shadow Stack is enabled.<BR>\n"
>                                                                                                    " Other - reserved"
>  
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_PROMPT  #language en-US "Speculation Barrier Type."
> +
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_HELP  #language en-US  "Indicates the type of instruction sequence to use for a speculation.barrier.  The default instruction sequence is LFENCE.<BR><BR>\n"
> +                                                                                      "0x00 - No operation.<BR>\n"
> +                                                                                      "0x01 - LFENCE (IA32/X64).<BR>\n"
> +                                                                                      "0x02 - CPUID  (IA32/X64).<BR>\n"
> +                                                                                      "Other - reserved"
> +
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLength_PROMPT  #language en-US "Maximum Length of Ascii String"
>  
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLength_HELP  #language en-US "Sets the maximum number of ASCII characters used for string functions.  This affects the following BaseLib functions: AsciiStrLen(), AsciiStrSize(), AsciiStrCmp(), AsciiStrnCmp(), AsciiStrCpy(), AsciiStrnCpy(). <BR><BR>\n"
> 

Re: [edk2-devel] [Patch V2 2/6] MdePkg/BaseLib: Use PcdSpeculationBarrierType

[Laszlo Ersek]

On 04/30/19 03:30, Michael D Kinney wrote:
> Use PcdSpeculationBarrierType in the x86 implementation
> of SpeculationBarrier() to select between AsmLfence(),
> AsmCpuid(), and no operation.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdePkg/Library/BaseLib/BaseLib.inf             | 1 +
>  MdePkg/Library/BaseLib/X86SpeculationBarrier.c | 8 ++++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index 533e83e0b2..3586beb0ab 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -394,6 +394,7 @@ [Pcd]
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength     ## SOMETIMES_CONSUMES
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## SOMETIMES_CONSUMES
>    gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## SOMETIMES_CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType       ## SOMETIMES_CONSUMES
>  
>  [FeaturePcd]
>    gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES

So, based on the example of

  MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf

and a few other INF files, I think we could use

  [Pcd.IA32, Pcd.X64]

here as well.

Just an idea.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> diff --git a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> index 8e5f983bb8..b28fd8de9b 100644
> --- a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> +++ b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> @@ -1,7 +1,7 @@
>  /** @file
>    SpeculationBarrier() function for IA32 and x64.
>  
> -  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
>  
> @@ -22,5 +22,9 @@ SpeculationBarrier (
>    VOID
>    )
>  {
> -  AsmLfence ();
> +  if (PcdGet8 (PcdSpeculationBarrierType) == 0x01) {
> +    AsmLfence ();
> +  } else if (PcdGet8 (PcdSpeculationBarrierType) == 0x02) {
> +    AsmCpuid (0x01, NULL, NULL, NULL, NULL);
> +  }
>  }
> 

Re: [edk2-devel] [Patch V2 1/6] MdePkg: Add PcdSpeculationBarrierType

[Michael D Kinney]

Laszlo,

I tried to design this PCD so it could be used for other
architectures as needed in the future by expanding the enum.
I marked enum values 0x01(LFENCE) and 0x02(CPUID) for
IA32/X64.  Value 0x00 (NOP) is for all archs.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Tuesday, April 30, 2019 4:47 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch V2 1/6] MdePkg: Add
> PcdSpeculationBarrierType
> 
> On 04/30/19 03:30, Michael D Kinney wrote:
> > Add
> gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType that
> > uses the PCD type FixedAtBuild.  This performs a
> build time
> > selection for the type of speculation barrier to use
> in the
> > BaseLib function SpeculationBarrier().  The
> recommended
> > speculation barrier for x86 is LFENCE and this is the
> default
> > value for this PCD.  x86 CPUs that do not support
> LFENCE must
> > select one of the other supported values which
> includes CPUID
> > and nothing.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > ---
> >  MdePkg/MdePkg.dec | 9 +++++++++
> >  MdePkg/MdePkg.uni | 8 ++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index e2ea8fff66..28d4a966c2 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -2062,6 +2062,15 @@ [PcdsFixedAtBuild]
> >    # @Prompt Enable control flow enforcement.
> >
> gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPrope
> rtyMask|0x0|UINT32|0x30001017
> >
> > +  ## Indicates the type of instruction sequence to
> use for a speculation
> > +  #  barrier.  The default instruction sequence is
> LFENCE.<BR><BR>
> > +  #   0x00 - No operation.<BR>
> > +  #   0x01 - LFENCE (IA32/X64).<BR>
> > +  #   0x02 - CPUID  (IA32/X64).<BR>
> > +  #   Other - reserved
> > +  # @Prompt Speculation Barrier Type.
> > +
> gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01
> |UINT8|0x30001018
> > +
> >  [PcdsFixedAtBuild,PcdsPatchableInModule]
> >    ## Indicates the maximum length of unicode string
> used in the following
> >    #  BaseLib functions: StrLen(), StrSize(),
> StrCmp(), StrnCmp(), StrCpy(), StrnCpy()<BR><BR>
> 
> In MdePkg.dec, we have:
> - [Includes.X64]
> - [LibraryClasses.X64]
> - [Guids.X64]
> 
> but no PCD declarations that are architecture-specific.
> Is that
> intentional? Because, this PCD could be a good
> candidate for "IA32/X64
> only". (Looking at the next patch too.)
> 
> But, that's just my curiosity.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 
> 
> > diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> > index c359bb4b5b..5c1fa24065 100644
> > --- a/MdePkg/MdePkg.uni
> > +++ b/MdePkg/MdePkg.uni
> > @@ -149,6 +149,14 @@
> >
> " BIT0 - SMM CET Shadow Stack is enabled.<BR>\n"
> >
> " Other - reserved"
> >
> > +#string
> STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_
> PROMPT  #language en-US "Speculation Barrier Type."
> > +
> > +#string
> STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_
> HELP  #language en-US  "Indicates the type of
> instruction sequence to use for a speculation.barrier.
> The default instruction sequence is LFENCE.<BR><BR>\n"
> > +
> "0x00 - No operation.<BR>\n"
> > +
> "0x01 - LFENCE (IA32/X64).<BR>\n"
> > +
> "0x02 - CPUID  (IA32/X64).<BR>\n"
> > +
> "Other - reserved"
> > +
> >  #string
> STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLengt
> h_PROMPT  #language en-US "Maximum Length of Ascii
> String"
> >
> >  #string
> STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLengt
> h_HELP  #language en-US "Sets the maximum number of
> ASCII characters used for string functions.  This
> affects the following BaseLib functions: AsciiStrLen(),
> AsciiStrSize(), AsciiStrCmp(), AsciiStrnCmp(),
> AsciiStrCpy(), AsciiStrnCpy(). <BR><BR>\n"
> >
> 
> 
> 

Re: [edk2-devel] [Patch V2 2/6] MdePkg/BaseLib: Use PcdSpeculationBarrierType

[Michael D Kinney]

Laszlo,

I agree the current usage is only for IA32/X64 source
files so your suggestion make sense.  If there is
a use case for other archs in the future for this PCD,
then the INF would have to be updated.

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 30, 2019 4:50 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch V2 2/6]
> MdePkg/BaseLib: Use PcdSpeculationBarrierType
> 
> On 04/30/19 03:30, Michael D Kinney wrote:
> > Use PcdSpeculationBarrierType in the x86
> implementation
> > of SpeculationBarrier() to select between
> AsmLfence(),
> > AsmCpuid(), and no operation.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > ---
> >  MdePkg/Library/BaseLib/BaseLib.inf             | 1 +
> >  MdePkg/Library/BaseLib/X86SpeculationBarrier.c | 8
> ++++++--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
> b/MdePkg/Library/BaseLib/BaseLib.inf
> > index 533e83e0b2..3586beb0ab 100644
> > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > @@ -394,6 +394,7 @@ [Pcd]
> >
> gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength
> ## SOMETIMES_CONSUMES
> >
> gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength
> ## SOMETIMES_CONSUMES
> >
> gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPrope
> rtyMask   ## SOMETIMES_CONSUMES
> > +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType
> ## SOMETIMES_CONSUMES
> >
> >  [FeaturePcd]
> >    gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ##
> CONSUMES
> 
> So, based on the example of
> 
> 
> MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCp
> u.inf
> 
> and a few other INF files, I think we could use
> 
>   [Pcd.IA32, Pcd.X64]
> 
> here as well.
> 
> Just an idea.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 
> > diff --git
> a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> > index 8e5f983bb8..b28fd8de9b 100644
> > --- a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> > +++ b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    SpeculationBarrier() function for IA32 and x64.
> >
> > -  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
> >
> > @@ -22,5 +22,9 @@ SpeculationBarrier (
> >    VOID
> >    )
> >  {
> > -  AsmLfence ();
> > +  if (PcdGet8 (PcdSpeculationBarrierType) == 0x01) {
> > +    AsmLfence ();
> > +  } else if (PcdGet8 (PcdSpeculationBarrierType) ==
> 0x02) {
> > +    AsmCpuid (0x01, NULL, NULL, NULL, NULL);
> > +  }
> >  }
> >

Re: [edk2-devel] [Patch V2 1/6] MdePkg: Add PcdSpeculationBarrierType

[Laszlo Ersek]

On 04/30/19 17:16, Kinney, Michael D wrote:
> Laszlo,
> 
> I tried to design this PCD so it could be used for other
> architectures as needed in the future by expanding the enum.
> I marked enum values 0x01(LFENCE) and 0x02(CPUID) for
> IA32/X64.  Value 0x00 (NOP) is for all archs.

Ah, good point. In fact, this has more or less crossed my mind, but I
ruled out the idea, as (I thought) a multi-arch PCD would have to be a
bitmap, not a simple enum.

Of course, I was wrong about that -- in any given platform build, the
PCD doesn't have to contain the right setting for every possible
architecture supported by edk2. It only need contain the right setting
for the arch of the current platform build.

So yes, this design is great; please apply my R-b.

Thanks
Laszlo



>> -----Original Message-----
>> From: devel@edk2.groups.io
>> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
>> Sent: Tuesday, April 30, 2019 4:47 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [Patch V2 1/6] MdePkg: Add
>> PcdSpeculationBarrierType
>>
>> On 04/30/19 03:30, Michael D Kinney wrote:
>>> Add
>> gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType that
>>> uses the PCD type FixedAtBuild.  This performs a
>> build time
>>> selection for the type of speculation barrier to use
>> in the
>>> BaseLib function SpeculationBarrier().  The
>> recommended
>>> speculation barrier for x86 is LFENCE and this is the
>> default
>>> value for this PCD.  x86 CPUs that do not support
>> LFENCE must
>>> select one of the other supported values which
>> includes CPUID
>>> and nothing.
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Signed-off-by: Michael D Kinney
>> <michael.d.kinney@intel.com>
>>> ---
>>>  MdePkg/MdePkg.dec | 9 +++++++++
>>>  MdePkg/MdePkg.uni | 8 ++++++++
>>>  2 files changed, 17 insertions(+)
>>>
>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>>> index e2ea8fff66..28d4a966c2 100644
>>> --- a/MdePkg/MdePkg.dec
>>> +++ b/MdePkg/MdePkg.dec
>>> @@ -2062,6 +2062,15 @@ [PcdsFixedAtBuild]
>>>    # @Prompt Enable control flow enforcement.
>>>
>> gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPrope
>> rtyMask|0x0|UINT32|0x30001017
>>>
>>> +  ## Indicates the type of instruction sequence to
>> use for a speculation
>>> +  #  barrier.  The default instruction sequence is
>> LFENCE.<BR><BR>
>>> +  #   0x00 - No operation.<BR>
>>> +  #   0x01 - LFENCE (IA32/X64).<BR>
>>> +  #   0x02 - CPUID  (IA32/X64).<BR>
>>> +  #   Other - reserved
>>> +  # @Prompt Speculation Barrier Type.
>>> +
>> gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01
>> |UINT8|0x30001018
>>> +
>>>  [PcdsFixedAtBuild,PcdsPatchableInModule]
>>>    ## Indicates the maximum length of unicode string
>> used in the following
>>>    #  BaseLib functions: StrLen(), StrSize(),
>> StrCmp(), StrnCmp(), StrCpy(), StrnCpy()<BR><BR>
>>
>> In MdePkg.dec, we have:
>> - [Includes.X64]
>> - [LibraryClasses.X64]
>> - [Guids.X64]
>>
>> but no PCD declarations that are architecture-specific.
>> Is that
>> intentional? Because, this PCD could be a good
>> candidate for "IA32/X64
>> only". (Looking at the next patch too.)
>>
>> But, that's just my curiosity.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>>
>>> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
>>> index c359bb4b5b..5c1fa24065 100644
>>> --- a/MdePkg/MdePkg.uni
>>> +++ b/MdePkg/MdePkg.uni
>>> @@ -149,6 +149,14 @@
>>>
>> " BIT0 - SMM CET Shadow Stack is enabled.<BR>\n"
>>>
>> " Other - reserved"
>>>
>>> +#string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_
>> PROMPT  #language en-US "Speculation Barrier Type."
>>> +
>>> +#string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdSpeculationBarrierType_
>> HELP  #language en-US  "Indicates the type of
>> instruction sequence to use for a speculation.barrier.
>> The default instruction sequence is LFENCE.<BR><BR>\n"
>>> +
>> "0x00 - No operation.<BR>\n"
>>> +
>> "0x01 - LFENCE (IA32/X64).<BR>\n"
>>> +
>> "0x02 - CPUID  (IA32/X64).<BR>\n"
>>> +
>> "Other - reserved"
>>> +
>>>  #string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLengt
>> h_PROMPT  #language en-US "Maximum Length of Ascii
>> String"
>>>
>>>  #string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLengt
>> h_HELP  #language en-US "Sets the maximum number of
>> ASCII characters used for string functions.  This
>> affects the following BaseLib functions: AsciiStrLen(),
>> AsciiStrSize(), AsciiStrCmp(), AsciiStrnCmp(),
>> AsciiStrCpy(), AsciiStrnCpy(). <BR><BR>\n"
>>>
>>
>>
>> 
> 

Re: [edk2-devel] [Patch V2 2/6] MdePkg/BaseLib: Use PcdSpeculationBarrierType

[Brian J. Johnson]

On 4/29/19 8:30 PM, Michael D Kinney wrote:
> Use PcdSpeculationBarrierType in the x86 implementation
> of SpeculationBarrier() to select between AsmLfence(),
> AsmCpuid(), and no operation.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>   MdePkg/Library/BaseLib/BaseLib.inf             | 1 +
>   MdePkg/Library/BaseLib/X86SpeculationBarrier.c | 8 ++++++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index 533e83e0b2..3586beb0ab 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -394,6 +394,7 @@ [Pcd]
>     gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength     ## SOMETIMES_CONSUMES
>     gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## SOMETIMES_CONSUMES
>     gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## SOMETIMES_CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType       ## SOMETIMES_CONSUMES
>   
>   [FeaturePcd]
>     gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
> diff --git a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> index 8e5f983bb8..b28fd8de9b 100644
> --- a/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> +++ b/MdePkg/Library/BaseLib/X86SpeculationBarrier.c
> @@ -1,7 +1,7 @@
>   /** @file
>     SpeculationBarrier() function for IA32 and x64.
>   
> -  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
>   
> @@ -22,5 +22,9 @@ SpeculationBarrier (
>     VOID
>     )
>   {
> -  AsmLfence ();
> +  if (PcdGet8 (PcdSpeculationBarrierType) == 0x01) {
> +    AsmLfence ();
> +  } else if (PcdGet8 (PcdSpeculationBarrierType) == 0x02) {
> +    AsmCpuid (0x01, NULL, NULL, NULL, NULL);
> +  }
>   }
> 

Looks good.  I'm not a maintainer, but FWIW:

Reviewed-by: Brian J. Johnson <brian.johnson@hpe.com>

-- 
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

brian.johnson@hpe.com
+1 651 683 7521  Office

Eagan, MN
hpe.com