[Patch 0/4] Resolve Quark build and boot issues

[Patch 0/4] Resolve Quark build and boot issues

[Michael D Kinney]

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

* 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 (4):
  MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence()
  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/Ia32/Lfence.nasm        | 14 +++++++++++++-
 .../Platform/Pei/PlatformInit/MemoryCallback.c |  6 +++---
 .../Pei/PlatformInit/PlatformEarlyInit.h       |  4 ++--
 .../Smm/Dxe/SmmAccessDxe/SmmAccess.inf         |  3 ++-
 .../Smm/Dxe/SmmAccessDxe/SmmAccessDriver.c     | 18 +++++++++++++++++-
 .../Smm/Dxe/SmmAccessDxe/SmmAccessDriver.h     |  3 ++-
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c        | 15 ++++++++++++++-
 7 files changed, 53 insertions(+), 10 deletions(-)

-- 
2.21.0.windows.1

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

[Michael D Kinney]

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
-- 
2.21.0.windows.1

[Patch 2/4] 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 3/4] 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>
---
 .../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..205f51ddb0 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 4/4] 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>
---
 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 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, April 25, 2019 10:54 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>
> Subject: [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for
> single core
> 
> 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

Re: [Patch 3/4] QuarkSocPkg/SmmAccessDxe: Set region to UC on SMRAM close

[Steele, Kelly]

Reviewed-by: Kelly Steele <kelly.steele@intel.com>




> -----Original Message-----
> From: Kinney, Michael D
> Sent: April 25, 2019 10:54
> To: devel@edk2.groups.io
> Cc: Steele, Kelly <kelly.steele@intel.com>
> Subject: [Patch 3/4] QuarkSocPkg/SmmAccessDxe: Set region to UC on SMRAM
> close
> 
> 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/bfc87aa78e77ed15b09d1b4499c5e
> ab63e8842bb
> 
> 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>
> ---
>  .../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/SmmAccessDrive
> r.c
> b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDrive
> r.c
> index 6148dea1b4..205f51ddb0 100644
> ---
> a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDrive
> r.c
> +++
> b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDrive
> r.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/SmmAccessDrive
> r.h
> b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDrive
> r.h
> index 80f73ba0e3..aca169d3e2 100644
> ---
> a/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDrive
> r.h
> +++
> b/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe/SmmAccessDrive
> r.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

Re: [Patch 4/4] QuarkPlatformPkg/PlatformInit: Resolve ResetSystemLib name collision

[Steele, Kelly]

Reviewed-by: Kelly Steele <kelly.steele@intel.com>




> -----Original Message-----
> From: Kinney, Michael D
> Sent: April 25, 2019 10:54
> To: devel@edk2.groups.io
> Cc: Steele, Kelly <kelly.steele@intel.com>
> Subject: [Patch 4/4] QuarkPlatformPkg/PlatformInit: Resolve ResetSystemLib
> name collision
> 
> 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>
> ---
>  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 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, April 26, 2019 1:54 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>
> Subject: [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for
> single core
> 
> 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

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

[Laszlo Ersek]

(+Jian)

Hi Mike,

thank you for the CC.

On 04/25/19 19:53, 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) {
> +    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 ();
>

This patch leads me down on two paths:

(1) Specifically regarding the code change. I think this patch is unsafe
    on platforms that dynamically set the PCD to a value larger than 1.
    (Including OVMF.)

    If the value is larger than 1, then the system has at least one AP,
    and the AP may enter the function. In addition, because the PCD is
    dynamic, the PcdGet32() call will invoke the PCD PPI (if I
    understand correctly), which is not allowed for an AP.

    Is it not possible to determine the availability of
    MSR_IA32_APIC_BASE from CPUID, or a different MSR?


(2) More generally, this patch made me review OvmfPkg/PlatformPei:

    (a) OvmfPkg/PlatformPei sets the PCD in MaxCpuCountInitialization(),

    (b) later, OvmfPkg/PlatformPei publishes the permanent PEI RAM in
        PublishPeiMemory()

    (c) which in turn leads to the installation of
        gEfiPeiMemoryDiscoveredPpiGuid intto the PPI database, by the
        PEI Core

    (d) CpuMpPei can now be dispatched, because it has a depex on the
        "memory discovered" PPI

    (e) PeiMpInitLib, which is linked into CpuMpPei, can consume the PCD
        safely.

    I relied on this behavior in the following OVMF commit:

    commit 45a70db3c3a59b64e0f517870415963fbfacf507
    Author: Laszlo Ersek <lersek@redhat.com>
    Date:   Thu Nov 24 15:18:44 2016 +0100

        OvmfPkg/PlatformPei: take VCPU count from QEMU and configure MpInitLib

        These settings will allow CpuMpPei and CpuDxe to wait for the initial AP
        check-ins exactly as long as necessary.

        It is safe to set PcdCpuMaxLogicalProcessorNumber and
        PcdCpuApInitTimeOutInMicroSeconds in OvmfPkg/PlatformPei.
        OvmfPkg/PlatformPei installs the permanent PEI RAM, producing
        gEfiPeiMemoryDiscoveredPpiGuid, and UefiCpuPkg/CpuMpPei has a depex on
        gEfiPeiMemoryDiscoveredPpiGuid.

        [...]

    Except... in commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support
    stack guard feature", 2018-09-10), the DEPEX mentioned in step (d)
    was deleted.

    So now I got a bit nervous, because how are then the setting and
    reading of the PCD serialized between OvmfPkg/PlatformPei, and
    PeiMpInitLib in CpuMpPei?

    Luckily however, I think we're safe:

    - CpuMpPei itself doesn't consume the PCD,

    - MpInitLib consumes the PCD in several functions, but all clients
      of MpInitLib must call MpInitLibInitialize() first, before using
      other library APIs

    - CpuMpPei calls MpInitLibInitialize() in the
      InitializeCpuMpWorker() function

    - the InitializeCpuMpWorker() function is only called from the
      MemoryDiscoveredPpiNotifyCallback().

    So the PCD set/get order remains safe and deterministic. Even though
    CpuMpPei can now be dispatched before permanent memory is
    discovered, MpInitLibInitialize() -- and so the reading of the PCD
    -- is still delayed until after permanent PEI RAM is available.
    That's a relief.

    In fact, it looks like commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei:
    support stack guard feature", 2018-09-10) delayed the entire
    original entry point of CpuMpPei into the "memory discovered"
    callback. That appears OK to me, it's just that the patch (~1000
    lines) could have been split at least in two: one patch could have
    implemented the "PPI DEPEX --> PPI notify" transformation, without
    other changes in behavior, and the second patch could have extended
    the (now delayed) startup logic with
    InitializeMpExceptionStackSwitchHandlers() & friends.

Thanks
Laszlo

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

[Laszlo Ersek]

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

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

[Brian J. Johnson]

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

Re: [edk2-devel] [Patch 4/4] QuarkPlatformPkg/PlatformInit: Resolve ResetSystemLib name collision

[Liming Gao]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Michael D Kinney
>Sent: Friday, April 26, 2019 1:54 AM
>To: devel@edk2.groups.io
>Cc: Steele, Kelly <kelly.steele@intel.com>
>Subject: [edk2-devel] [Patch 4/4] QuarkPlatformPkg/PlatformInit: Resolve
>ResetSystemLib name collision
>
>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>
>---
> 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 1/4] MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence()

[Liming Gao]

Mike:
  I think X64 implementation also needs to add this check. 

Thanks
Liming
>-----Original Message-----
>From: Kinney, Michael D
>Sent: Friday, April 26, 2019 1:54 AM
>To: devel@edk2.groups.io
>Cc: Gao, Liming <liming.gao@intel.com>
>Subject: [Patch 1/4] MdePkg/BaseLib: Verify SSE2 support in IA32 AsmLfence()
>
>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
>--
>2.21.0.windows.1

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

[Laszlo Ersek]

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".

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

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

[Liming Gao]

> -----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
> 
> 

Re: [edk2-devel] [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Michael D Kinney]

Laszlo,

I was attempting to follow the equivalent detection logic
that is used in the SourceLevelDebugPkg.

https://github.com/tianocore/edk2/blob/e2d3a25f1a3135221a9c8061e1b8f90245d727eb/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugMp.c#L140

Yes.  CPUID can be used to determine availability of
MSR_IA32_APIC_BASE.  That would be safer if the maximum
number of CPUs can start with a value of 1 and change to
a higher value later.  But based on your analysis,
it looks like the max number of CPUs is known when this 
function runs which is always after memory is discovered.

The MSR access is in the function GetCpuMpData(), which is
called from all the MP service functions.  I was trying
to avoid an extra CPUID check on all those paths.

I prefer the current patch if it is safe.  Please let me
know if you think extra comments are required in the code
or the commit message.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Friday, April 26, 2019 12:25 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [Patch 2/4]
> UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for
> single core
> 
> (+Jian)
> 
> Hi Mike,
> 
> thank you for the CC.
> 
> On 04/25/19 19:53, 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) {
> > +    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 ();
> >
> 
> This patch leads me down on two paths:
> 
> (1) Specifically regarding the code change. I think this
> patch is unsafe
>     on platforms that dynamically set the PCD to a value
> larger than 1.
>     (Including OVMF.)
> 
>     If the value is larger than 1, then the system has
> at least one AP,
>     and the AP may enter the function. In addition,
> because the PCD is
>     dynamic, the PcdGet32() call will invoke the PCD PPI
> (if I
>     understand correctly), which is not allowed for an
> AP.
> 
>     Is it not possible to determine the availability of
>     MSR_IA32_APIC_BASE from CPUID, or a different MSR?
> 
> 
> (2) More generally, this patch made me review
> OvmfPkg/PlatformPei:
> 
>     (a) OvmfPkg/PlatformPei sets the PCD in
> MaxCpuCountInitialization(),
> 
>     (b) later, OvmfPkg/PlatformPei publishes the
> permanent PEI RAM in
>         PublishPeiMemory()
> 
>     (c) which in turn leads to the installation of
>         gEfiPeiMemoryDiscoveredPpiGuid intto the PPI
> database, by the
>         PEI Core
> 
>     (d) CpuMpPei can now be dispatched, because it has a
> depex on the
>         "memory discovered" PPI
> 
>     (e) PeiMpInitLib, which is linked into CpuMpPei, can
> consume the PCD
>         safely.
> 
>     I relied on this behavior in the following OVMF
> commit:
> 
>     commit 45a70db3c3a59b64e0f517870415963fbfacf507
>     Author: Laszlo Ersek <lersek@redhat.com>
>     Date:   Thu Nov 24 15:18:44 2016 +0100
> 
>         OvmfPkg/PlatformPei: take VCPU count from QEMU
> and configure MpInitLib
> 
>         These settings will allow CpuMpPei and CpuDxe to
> wait for the initial AP
>         check-ins exactly as long as necessary.
> 
>         It is safe to set
> PcdCpuMaxLogicalProcessorNumber and
>         PcdCpuApInitTimeOutInMicroSeconds in
> OvmfPkg/PlatformPei.
>         OvmfPkg/PlatformPei installs the permanent PEI
> RAM, producing
>         gEfiPeiMemoryDiscoveredPpiGuid, and
> UefiCpuPkg/CpuMpPei has a depex on
>         gEfiPeiMemoryDiscoveredPpiGuid.
> 
>         [...]
> 
>     Except... in commit 0a0d5296e448
> ("UefiCpuPkg/CpuMpPei: support
>     stack guard feature", 2018-09-10), the DEPEX
> mentioned in step (d)
>     was deleted.
> 
>     So now I got a bit nervous, because how are then the
> setting and
>     reading of the PCD serialized between
> OvmfPkg/PlatformPei, and
>     PeiMpInitLib in CpuMpPei?
> 
>     Luckily however, I think we're safe:
> 
>     - CpuMpPei itself doesn't consume the PCD,
> 
>     - MpInitLib consumes the PCD in several functions,
> but all clients
>       of MpInitLib must call MpInitLibInitialize()
> first, before using
>       other library APIs
> 
>     - CpuMpPei calls MpInitLibInitialize() in the
>       InitializeCpuMpWorker() function
> 
>     - the InitializeCpuMpWorker() function is only
> called from the
>       MemoryDiscoveredPpiNotifyCallback().
> 
>     So the PCD set/get order remains safe and
> deterministic. Even though
>     CpuMpPei can now be dispatched before permanent
> memory is
>     discovered, MpInitLibInitialize() -- and so the
> reading of the PCD
>     -- is still delayed until after permanent PEI RAM is
> available.
>     That's a relief.
> 
>     In fact, it looks like commit 0a0d5296e448
> ("UefiCpuPkg/CpuMpPei:
>     support stack guard feature", 2018-09-10) delayed
> the entire
>     original entry point of CpuMpPei into the "memory
> discovered"
>     callback. That appears OK to me, it's just that the
> patch (~1000
>     lines) could have been split at least in two: one
> patch could have
>     implemented the "PPI DEPEX --> PPI notify"
> transformation, without
>     other changes in behavior, and the second patch
> could have extended
>     the (now delayed) startup logic with
>     InitializeMpExceptionStackSwitchHandlers() &
> friends.
> 
> Thanks
> Laszlo
> 
> 

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

[Michael D Kinney]

Hi,

Thanks for the detailed feedback.

I did consider moving the CPUID check up a level so
AsmLfence() would always do the requested instruction.
This would have been a larger patch set that would
introduce an IA32 and X64 specific version of
SpeculationBarrier().

I agree that a build time PCD would is a better
approach.  It should have a default value of enabled,
and only platforms that use CPUs that do not support
LFENCE would avoid the call to AsmLfence() and use an
alternate serializing instruction.

Here is the proposed PCD:

[PcdsFixedAtBuild]
  ## Indicates the type of instruction sequence to use
  #  for a speculation barrier.  The default instruction
  #  sequence is LFENCE.<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

Best regards,

Mike

> -----Original Message-----
> From: Gao, Liming
> Sent: Monday, April 29, 2019 7:10 AM
> To: devel@edk2.groups.io; lersek@redhat.com;
> brian.johnson@hpe.com; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [Patch 1/4] MdePkg/BaseLib:
> Verify SSE2 support in IA32 AsmLfence()
> 
> > -----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
> >
> > 

Re: [edk2-devel] [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Laszlo Ersek]

On 04/29/19 19:11, Kinney, Michael D wrote:
> Laszlo,
> 
> I was attempting to follow the equivalent detection logic
> that is used in the SourceLevelDebugPkg.
> 
> https://github.com/tianocore/edk2/blob/e2d3a25f1a3135221a9c8061e1b8f90245d727eb/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugMp.c#L140
> 
> Yes.  CPUID can be used to determine availability of
> MSR_IA32_APIC_BASE.  That would be safer if the maximum
> number of CPUs can start with a value of 1 and change to
> a higher value later.  But based on your analysis,
> it looks like the max number of CPUs is known when this 
> function runs which is always after memory is discovered.

The proposed patch is safe wrt. the PCD production-consumption sequence,
yes.

However, the patch is unsafe due to APs calling a PPI member function
(namely, the PCD_PPI.Get32 function).

To my understanding, APs may not call PPIs.

> The MSR access is in the function GetCpuMpData(), which is
> called from all the MP service functions.  I was trying
> to avoid an extra CPUID check on all those paths.
> 
> I prefer the current patch if it is safe.  Please let me
> know if you think extra comments are required in the code
> or the commit message.

I think the patch is unsafe, because it is possible for both of the
following conditions to hold, at the same time:

- the function may be entered by an AP
- the PCD may be dynamic

That would lead to an AP calling

  (GetPcdPpiPointer ())->Get32 (TokenNumber)

which I believe is forbidden.


If the patch called FixedPcdGet32(), then the patch would be safe.
Unfortunately, in that case, the patch wouldn't compile for OvmfPkg.

If we can use a new PCD for this, such that UefiCpuPkg.dec does not
permit platforms to pick "dynamic" for that PCD -- i.e. it would have to
be Feature, Fixed, or Patch --, and then the patch is updated to call
the appropriate flavor-specific PCD macro (FeaturePcdGet, PatchPcdGet*,
FixedPcdGet*), then the patch will be fine.

The point is that the "getting the PCD" action never enter a library
function that is not explicitly MP-safe, nor enter a PPI member function.

Thanks
Laszlo

>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Friday, April 26, 2019 12:25 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
>> Subject: Re: [edk2-devel] [Patch 2/4]
>> UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for
>> single core
>>
>> (+Jian)
>>
>> Hi Mike,
>>
>> thank you for the CC.
>>
>> On 04/25/19 19:53, 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) {
>>> +    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 ();
>>>
>>
>> This patch leads me down on two paths:
>>
>> (1) Specifically regarding the code change. I think this
>> patch is unsafe
>>     on platforms that dynamically set the PCD to a value
>> larger than 1.
>>     (Including OVMF.)
>>
>>     If the value is larger than 1, then the system has
>> at least one AP,
>>     and the AP may enter the function. In addition,
>> because the PCD is
>>     dynamic, the PcdGet32() call will invoke the PCD PPI
>> (if I
>>     understand correctly), which is not allowed for an
>> AP.
>>
>>     Is it not possible to determine the availability of
>>     MSR_IA32_APIC_BASE from CPUID, or a different MSR?
>>
>>
>> (2) More generally, this patch made me review
>> OvmfPkg/PlatformPei:
>>
>>     (a) OvmfPkg/PlatformPei sets the PCD in
>> MaxCpuCountInitialization(),
>>
>>     (b) later, OvmfPkg/PlatformPei publishes the
>> permanent PEI RAM in
>>         PublishPeiMemory()
>>
>>     (c) which in turn leads to the installation of
>>         gEfiPeiMemoryDiscoveredPpiGuid intto the PPI
>> database, by the
>>         PEI Core
>>
>>     (d) CpuMpPei can now be dispatched, because it has a
>> depex on the
>>         "memory discovered" PPI
>>
>>     (e) PeiMpInitLib, which is linked into CpuMpPei, can
>> consume the PCD
>>         safely.
>>
>>     I relied on this behavior in the following OVMF
>> commit:
>>
>>     commit 45a70db3c3a59b64e0f517870415963fbfacf507
>>     Author: Laszlo Ersek <lersek@redhat.com>
>>     Date:   Thu Nov 24 15:18:44 2016 +0100
>>
>>         OvmfPkg/PlatformPei: take VCPU count from QEMU
>> and configure MpInitLib
>>
>>         These settings will allow CpuMpPei and CpuDxe to
>> wait for the initial AP
>>         check-ins exactly as long as necessary.
>>
>>         It is safe to set
>> PcdCpuMaxLogicalProcessorNumber and
>>         PcdCpuApInitTimeOutInMicroSeconds in
>> OvmfPkg/PlatformPei.
>>         OvmfPkg/PlatformPei installs the permanent PEI
>> RAM, producing
>>         gEfiPeiMemoryDiscoveredPpiGuid, and
>> UefiCpuPkg/CpuMpPei has a depex on
>>         gEfiPeiMemoryDiscoveredPpiGuid.
>>
>>         [...]
>>
>>     Except... in commit 0a0d5296e448
>> ("UefiCpuPkg/CpuMpPei: support
>>     stack guard feature", 2018-09-10), the DEPEX
>> mentioned in step (d)
>>     was deleted.
>>
>>     So now I got a bit nervous, because how are then the
>> setting and
>>     reading of the PCD serialized between
>> OvmfPkg/PlatformPei, and
>>     PeiMpInitLib in CpuMpPei?
>>
>>     Luckily however, I think we're safe:
>>
>>     - CpuMpPei itself doesn't consume the PCD,
>>
>>     - MpInitLib consumes the PCD in several functions,
>> but all clients
>>       of MpInitLib must call MpInitLibInitialize()
>> first, before using
>>       other library APIs
>>
>>     - CpuMpPei calls MpInitLibInitialize() in the
>>       InitializeCpuMpWorker() function
>>
>>     - the InitializeCpuMpWorker() function is only
>> called from the
>>       MemoryDiscoveredPpiNotifyCallback().
>>
>>     So the PCD set/get order remains safe and
>> deterministic. Even though
>>     CpuMpPei can now be dispatched before permanent
>> memory is
>>     discovered, MpInitLibInitialize() -- and so the
>> reading of the PCD
>>     -- is still delayed until after permanent PEI RAM is
>> available.
>>     That's a relief.
>>
>>     In fact, it looks like commit 0a0d5296e448
>> ("UefiCpuPkg/CpuMpPei:
>>     support stack guard feature", 2018-09-10) delayed
>> the entire
>>     original entry point of CpuMpPei into the "memory
>> discovered"
>>     callback. That appears OK to me, it's just that the
>> patch (~1000
>>     lines) could have been split at least in two: one
>> patch could have
>>     implemented the "PPI DEPEX --> PPI notify"
>> transformation, without
>>     other changes in behavior, and the second patch
>> could have extended
>>     the (now delayed) startup logic with
>>     InitializeMpExceptionStackSwitchHandlers() &
>> friends.
>>
>> Thanks
>> Laszlo
>>
>> 
> 

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

[Laszlo Ersek]

On 04/29/19 20:23, Kinney, Michael D wrote:
> Hi,
> 
> Thanks for the detailed feedback.
> 
> I did consider moving the CPUID check up a level so
> AsmLfence() would always do the requested instruction.
> This would have been a larger patch set that would
> introduce an IA32 and X64 specific version of
> SpeculationBarrier().
> 
> I agree that a build time PCD would is a better
> approach.  It should have a default value of enabled,
> and only platforms that use CPUs that do not support
> LFENCE would avoid the call to AsmLfence() and use an
> alternate serializing instruction.
> 
> Here is the proposed PCD:
> 
> [PcdsFixedAtBuild]
>   ## Indicates the type of instruction sequence to use
>   #  for a speculation barrier.  The default instruction
>   #  sequence is LFENCE.<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

Looks great to me.

Thanks,
Laszlo

> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: Gao, Liming
>> Sent: Monday, April 29, 2019 7:10 AM
>> To: devel@edk2.groups.io; lersek@redhat.com;
>> brian.johnson@hpe.com; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Subject: RE: [edk2-devel] [Patch 1/4] MdePkg/BaseLib:
>> Verify SSE2 support in IA32 AsmLfence()
>>
>>> -----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
>>>
>>> 
> 

Re: [edk2-devel] [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

[Michael D Kinney]

Laszlo,

I agree with your analysis.  I will work on a 
different solution.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Tuesday, April 30, 2019 3:02 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [Patch 2/4]
> UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for
> single core
> 
> On 04/29/19 19:11, Kinney, Michael D wrote:
> > Laszlo,
> >
> > I was attempting to follow the equivalent detection
> logic
> > that is used in the SourceLevelDebugPkg.
> >
> >
> https://github.com/tianocore/edk2/blob/e2d3a25f1a313522
> 1a9c8061e1b8f90245d727eb/SourceLevelDebugPkg/Library/De
> bugAgent/DebugAgentCommon/DebugMp.c#L140
> >
> > Yes.  CPUID can be used to determine availability of
> > MSR_IA32_APIC_BASE.  That would be safer if the
> maximum
> > number of CPUs can start with a value of 1 and change
> to
> > a higher value later.  But based on your analysis,
> > it looks like the max number of CPUs is known when
> this
> > function runs which is always after memory is
> discovered.
> 
> The proposed patch is safe wrt. the PCD production-
> consumption sequence,
> yes.
> 
> However, the patch is unsafe due to APs calling a PPI
> member function
> (namely, the PCD_PPI.Get32 function).
> 
> To my understanding, APs may not call PPIs.
> 
> > The MSR access is in the function GetCpuMpData(),
> which is
> > called from all the MP service functions.  I was
> trying
> > to avoid an extra CPUID check on all those paths.
> >
> > I prefer the current patch if it is safe.  Please let
> me
> > know if you think extra comments are required in the
> code
> > or the commit message.
> 
> I think the patch is unsafe, because it is possible for
> both of the
> following conditions to hold, at the same time:
> 
> - the function may be entered by an AP
> - the PCD may be dynamic
> 
> That would lead to an AP calling
> 
>   (GetPcdPpiPointer ())->Get32 (TokenNumber)
> 
> which I believe is forbidden.
> 
> 
> If the patch called FixedPcdGet32(), then the patch
> would be safe.
> Unfortunately, in that case, the patch wouldn't compile
> for OvmfPkg.
> 
> If we can use a new PCD for this, such that
> UefiCpuPkg.dec does not
> permit platforms to pick "dynamic" for that PCD -- i.e.
> it would have to
> be Feature, Fixed, or Patch --, and then the patch is
> updated to call
> the appropriate flavor-specific PCD macro
> (FeaturePcdGet, PatchPcdGet*,
> FixedPcdGet*), then the patch will be fine.
> 
> The point is that the "getting the PCD" action never
> enter a library
> function that is not explicitly MP-safe, nor enter a
> PPI member function.
> 
> Thanks
> Laszlo
> 
> >> -----Original Message-----
> >> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Friday, April 26, 2019 12:25 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >> devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> >> Subject: Re: [edk2-devel] [Patch 2/4]
> >> UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for
> >> single core
> >>
> >> (+Jian)
> >>
> >> Hi Mike,
> >>
> >> thank you for the CC.
> >>
> >> On 04/25/19 19:53, 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) {
> >>> +    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 ();
> >>>
> >>
> >> This patch leads me down on two paths:
> >>
> >> (1) Specifically regarding the code change. I think
> this
> >> patch is unsafe
> >>     on platforms that dynamically set the PCD to a
> value
> >> larger than 1.
> >>     (Including OVMF.)
> >>
> >>     If the value is larger than 1, then the system
> has
> >> at least one AP,
> >>     and the AP may enter the function. In addition,
> >> because the PCD is
> >>     dynamic, the PcdGet32() call will invoke the PCD
> PPI
> >> (if I
> >>     understand correctly), which is not allowed for
> an
> >> AP.
> >>
> >>     Is it not possible to determine the availability
> of
> >>     MSR_IA32_APIC_BASE from CPUID, or a different
> MSR?
> >>
> >>
> >> (2) More generally, this patch made me review
> >> OvmfPkg/PlatformPei:
> >>
> >>     (a) OvmfPkg/PlatformPei sets the PCD in
> >> MaxCpuCountInitialization(),
> >>
> >>     (b) later, OvmfPkg/PlatformPei publishes the
> >> permanent PEI RAM in
> >>         PublishPeiMemory()
> >>
> >>     (c) which in turn leads to the installation of
> >>         gEfiPeiMemoryDiscoveredPpiGuid intto the PPI
> >> database, by the
> >>         PEI Core
> >>
> >>     (d) CpuMpPei can now be dispatched, because it
> has a
> >> depex on the
> >>         "memory discovered" PPI
> >>
> >>     (e) PeiMpInitLib, which is linked into CpuMpPei,
> can
> >> consume the PCD
> >>         safely.
> >>
> >>     I relied on this behavior in the following OVMF
> >> commit:
> >>
> >>     commit 45a70db3c3a59b64e0f517870415963fbfacf507
> >>     Author: Laszlo Ersek <lersek@redhat.com>
> >>     Date:   Thu Nov 24 15:18:44 2016 +0100
> >>
> >>         OvmfPkg/PlatformPei: take VCPU count from
> QEMU
> >> and configure MpInitLib
> >>
> >>         These settings will allow CpuMpPei and
> CpuDxe to
> >> wait for the initial AP
> >>         check-ins exactly as long as necessary.
> >>
> >>         It is safe to set
> >> PcdCpuMaxLogicalProcessorNumber and
> >>         PcdCpuApInitTimeOutInMicroSeconds in
> >> OvmfPkg/PlatformPei.
> >>         OvmfPkg/PlatformPei installs the permanent
> PEI
> >> RAM, producing
> >>         gEfiPeiMemoryDiscoveredPpiGuid, and
> >> UefiCpuPkg/CpuMpPei has a depex on
> >>         gEfiPeiMemoryDiscoveredPpiGuid.
> >>
> >>         [...]
> >>
> >>     Except... in commit 0a0d5296e448
> >> ("UefiCpuPkg/CpuMpPei: support
> >>     stack guard feature", 2018-09-10), the DEPEX
> >> mentioned in step (d)
> >>     was deleted.
> >>
> >>     So now I got a bit nervous, because how are then
> the
> >> setting and
> >>     reading of the PCD serialized between
> >> OvmfPkg/PlatformPei, and
> >>     PeiMpInitLib in CpuMpPei?
> >>
> >>     Luckily however, I think we're safe:
> >>
> >>     - CpuMpPei itself doesn't consume the PCD,
> >>
> >>     - MpInitLib consumes the PCD in several
> functions,
> >> but all clients
> >>       of MpInitLib must call MpInitLibInitialize()
> >> first, before using
> >>       other library APIs
> >>
> >>     - CpuMpPei calls MpInitLibInitialize() in the
> >>       InitializeCpuMpWorker() function
> >>
> >>     - the InitializeCpuMpWorker() function is only
> >> called from the
> >>       MemoryDiscoveredPpiNotifyCallback().
> >>
> >>     So the PCD set/get order remains safe and
> >> deterministic. Even though
> >>     CpuMpPei can now be dispatched before permanent
> >> memory is
> >>     discovered, MpInitLibInitialize() -- and so the
> >> reading of the PCD
> >>     -- is still delayed until after permanent PEI
> RAM is
> >> available.
> >>     That's a relief.
> >>
> >>     In fact, it looks like commit 0a0d5296e448
> >> ("UefiCpuPkg/CpuMpPei:
> >>     support stack guard feature", 2018-09-10)
> delayed
> >> the entire
> >>     original entry point of CpuMpPei into the
> "memory
> >> discovered"
> >>     callback. That appears OK to me, it's just that
> the
> >> patch (~1000
> >>     lines) could have been split at least in two:
> one
> >> patch could have
> >>     implemented the "PPI DEPEX --> PPI notify"
> >> transformation, without
> >>     other changes in behavior, and the second patch
> >> could have extended
> >>     the (now delayed) startup logic with
> >>     InitializeMpExceptionStackSwitchHandlers() &
> >> friends.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>
> >
> 
> 
>