[edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability

Wu, Jiaxin posted 1 patch 1 year, 9 months ago
Failed in applying to current master (apply log)
.../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c    | 248 ++++++++++++---------
1 file changed, 141 insertions(+), 107 deletions(-)
[edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability
Posted by Wu, Jiaxin 1 year, 9 months ago
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3962

Two SMM variables (mSmrrSupported & mSmmFeatureControlSupported) are global
variables, they control whether the SMRR and SMM Feature Control MSR will
be restored respectively.
To avoid the TOCTOU, dynamic check SMRR enable & SmmFeatureControl capability.

Change-Id: I6835e4b0e12c5e6f52effb60fd9224e3eb97fc0d
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c    | 248 ++++++++++++---------
 1 file changed, 141 insertions(+), 107 deletions(-)

diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
index 78de7f8407..b2f31c993f 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
@@ -35,26 +35,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 // MSRs required for configuration of SMM Code Access Check
 //
 #define SMM_FEATURES_LIB_IA32_MCA_CAP  0x17D
 #define   SMM_CODE_ACCESS_CHK_BIT      BIT58
 
-//
-// Set default value to assume SMRR is not supported
-//
-BOOLEAN  mSmrrSupported = FALSE;
-
-//
-// Set default value to assume MSR_SMM_FEATURE_CONTROL is not supported
-//
-BOOLEAN  mSmmFeatureControlSupported = FALSE;
-
-//
-// Set default value to assume IA-32 Architectural MSRs are used
-//
-UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
-UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
-
 //
 // Set default value to assume MTRRs need to be configured on each SMI
 //
 BOOLEAN  mNeedConfigureMtrrs = TRUE;
 
@@ -62,26 +46,39 @@ BOOLEAN  mNeedConfigureMtrrs = TRUE;
 // Array for state of SMRR enable on all CPUs
 //
 BOOLEAN  *mSmrrEnabled;
 
 /**
-  Performs library initialization.
+  Return if SMRR is supported
 
-  This initialization function contains common functionality shared betwen all
-  library instance constructors.
+  @param[in] SmrrPhysBaseMsr   Pointer to SmrrPhysBaseMsr.
+  @param[in] SmrrPhysMaskMsr   Pointer to SmrrPhysMaskMsr.
+
+  @retval TRUE  SMRR is supported.
+  @retval FALSE SMRR is not supported.
 
 **/
-VOID
-CpuFeaturesLibInitialization (
-  VOID
+BOOLEAN
+IsSmrrSupported (
+  IN UINT32  *SmrrPhysBaseMsr    OPTIONAL,
+  IN UINT32  *SmrrPhysMaskMsr    OPTIONAL
   )
 {
+  BOOLEAN  ReturnValue;
+
   UINT32  RegEax;
   UINT32  RegEdx;
   UINTN   FamilyId;
   UINTN   ModelId;
 
+  UINT64  FeatureControl;
+
+  //
+  // Set default value to assume SMRR is not supported
+  //
+  ReturnValue = FALSE;
+
   //
   // Retrieve CPU Family and Model
   //
   AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
   FamilyId = (RegEax >> 8) & 0xf;
@@ -96,11 +93,11 @@ CpuFeaturesLibInitialization (
   if ((RegEdx & BIT12) != 0) {
     //
     // Check MTRR_CAP MSR bit 11 for SMRR support
     //
     if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0) {
-      mSmrrSupported = TRUE;
+      ReturnValue = TRUE;
     }
   }
 
   //
   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
@@ -109,28 +106,79 @@ CpuFeaturesLibInitialization (
   // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H, then
   // SMRR Physical Base and SMM Physical Mask MSRs are not available.
   //
   if (FamilyId == 0x06) {
     if ((ModelId == 0x1C) || (ModelId == 0x26) || (ModelId == 0x27) || (ModelId == 0x35) || (ModelId == 0x36)) {
-      mSmrrSupported = FALSE;
+      ReturnValue = FALSE;
     }
   }
 
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
-  //
-  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
-  // Processor Family MSRs
-  //
-  if (FamilyId == 0x06) {
-    if ((ModelId == 0x17) || (ModelId == 0x0f)) {
-      mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
-      mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
+  if (ReturnValue) {
+    //
+    // Return the SmrrPhysBaseMsr & SmrrPhysMaskMsr if required & Smrr Supported
+    //
+    if (SmrrPhysBaseMsr != NULL) {
+      *SmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
+    }
+
+    if (SmrrPhysBaseMsr != NULL) {
+      *SmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
+    }
+
+    //
+    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+    // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
+    //
+    // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
+    // Processor Family MSRs
+    //
+    if (FamilyId == 0x06) {
+      if ((ModelId == 0x17) || (ModelId == 0x0f)) {
+        if (SmrrPhysBaseMsr != NULL) {
+          *SmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
+        }
+
+        if (SmrrPhysMaskMsr != NULL) {
+          *SmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
+        }
+
+        //
+        // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+        // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
+        //
+        // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being used, then
+        // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A) is set before
+        // accessing SMRR base/mask MSRs.  If Lock(BIT0) of MSR_FEATURE_CONTROL MSR(0x3A)
+        // is set, then the MSR is locked and can not be modified.
+        //
+
+        FeatureControl = AsmReadMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
+        if (((FeatureControl & BIT3) == 0) && ((FeatureControl & BIT0) == 1)) {
+          ReturnValue = FALSE;
+        }
+      }
     }
   }
 
+  return ReturnValue;
+}
+
+/**
+  Performs library initialization.
+
+  This initialization function contains common functionality shared betwen all
+  library instance constructors.
+
+**/
+VOID
+CpuFeaturesLibInitialization (
+  VOID
+  )
+{
+  UINT32  RegEax;
+  UINT32  RegEdx;
+
   //
   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
   // Volume 3C, Section 34.4.2 SMRAM Caching
   //   An IA-32 processor does not automatically write back and invalidate its
   //   caches before entering SMM or before exiting SMM. Because of this behavior,
@@ -193,50 +241,27 @@ SmmCpuFeaturesInitializeProcessor (
   IN EFI_PROCESSOR_INFORMATION  *ProcessorInfo,
   IN CPU_HOT_PLUG_DATA          *CpuHotPlugData
   )
 {
   SMRAM_SAVE_STATE_MAP  *CpuState;
-  UINT64                FeatureControl;
-  UINT32                RegEax;
-  UINT32                RegEdx;
-  UINTN                 FamilyId;
-  UINTN                 ModelId;
+  UINT32                SmrrPhysBaseMsr;
+  UINT32                SmrrPhysMaskMsr;
 
   //
   // Configure SMBASE.
   //
   CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
   CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
 
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
-  //
-  // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being used, then
-  // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A) is set before
-  // accessing SMRR base/mask MSRs.  If Lock(BIT0) of MSR_FEATURE_CONTROL MSR(0x3A)
-  // is set, then the MSR is locked and can not be modified.
-  //
-  if (mSmrrSupported && (mSmrrPhysBaseMsr == SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE)) {
-    FeatureControl = AsmReadMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
-    if ((FeatureControl & BIT3) == 0) {
-      if ((FeatureControl & BIT0) == 0) {
-        AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL, FeatureControl | BIT3);
-      } else {
-        mSmrrSupported = FALSE;
-      }
-    }
-  }
-
   //
   // If SMRR is supported, then program SMRR base/mask MSRs.
   // The EFI_MSR_SMRR_PHYS_MASK_VALID bit is not set until the first normal SMI.
   // The code that initializes SMM environment is running in normal mode
   // from SMRAM region.  If SMRR is enabled here, then the SMRAM region
   // is protected and the normal mode code execution will fail.
   //
-  if (mSmrrSupported) {
+  if (IsSmrrSupported (&SmrrPhysBaseMsr, &SmrrPhysMaskMsr)) {
     //
     // SMRR size cannot be less than 4-KBytes
     // SMRR size must be of length 2^n
     // SMRR base alignment cannot be less than SMRR length
     //
@@ -250,50 +275,16 @@ SmmCpuFeaturesInitializeProcessor (
       if (IsMonarch) {
         DEBUG ((DEBUG_ERROR, "SMM Base/Size does not meet alignment/size requirement!\n"));
         CpuDeadLoop ();
       }
     } else {
-      AsmWriteMsr64 (mSmrrPhysBaseMsr, CpuHotPlugData->SmrrBase | MTRR_CACHE_WRITE_BACK);
-      AsmWriteMsr64 (mSmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1) & EFI_MSR_SMRR_MASK));
+      AsmWriteMsr64 (SmrrPhysBaseMsr, CpuHotPlugData->SmrrBase | MTRR_CACHE_WRITE_BACK);
+      AsmWriteMsr64 (SmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1) & EFI_MSR_SMRR_MASK));
       mSmrrEnabled[CpuIndex] = FALSE;
     }
   }
 
-  //
-  // Retrieve CPU Family and Model
-  //
-  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
-  FamilyId = (RegEax >> 8) & 0xf;
-  ModelId  = (RegEax >> 4) & 0xf;
-  if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
-    ModelId = ModelId | ((RegEax >> 12) & 0xf0);
-  }
-
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
-  // Processor Family.
-  //
-  // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th Generation
-  // Intel(R) Core(TM) Processor Family MSRs.
-  //
-  if (FamilyId == 0x06) {
-    if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
-        (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) || (ModelId == 0x4F) ||
-        (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) || (ModelId == 0x5C) ||
-        (ModelId == 0x8C))
-    {
-      //
-      // Check to see if the CPU supports the SMM Code Access Check feature
-      // Do not access this MSR unless the CPU supports the SmmRegFeatureControl
-      //
-      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) & SMM_CODE_ACCESS_CHK_BIT) != 0) {
-        mSmmFeatureControlSupported = TRUE;
-      }
-    }
-  }
-
   //
   //  Call internal worker function that completes the CPU initialization
   //
   FinishSmmCpuFeaturesInitializeProcessor ();
 }
@@ -381,12 +372,14 @@ VOID
 EFIAPI
 SmmCpuFeaturesDisableSmrr (
   VOID
   )
 {
-  if (mSmrrSupported && mNeedConfigureMtrrs) {
-    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
+  UINT32  SmrrPhysMaskMsr;
+
+  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) && mNeedConfigureMtrrs) {
+    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
   }
 }
 
 /**
   Enable SMRR register if SMRR is supported and SmmCpuFeaturesNeedConfigureMtrrs()
@@ -396,12 +389,14 @@ VOID
 EFIAPI
 SmmCpuFeaturesReenableSmrr (
   VOID
   )
 {
-  if (mSmrrSupported && mNeedConfigureMtrrs) {
-    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
+  UINT32  SmrrPhysMaskMsr;
+
+  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) && mNeedConfigureMtrrs) {
+    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
   }
 }
 
 /**
   Processor specific hook point each time a CPU enters System Management Mode.
@@ -414,15 +409,17 @@ VOID
 EFIAPI
 SmmCpuFeaturesRendezvousEntry (
   IN UINTN  CpuIndex
   )
 {
+  UINT32  SmrrPhysMaskMsr;
+
   //
   // If SMRR is supported and this is the first normal SMI, then enable SMRR
   //
-  if (mSmrrSupported && !mSmrrEnabled[CpuIndex]) {
-    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
+  if (!mSmrrEnabled[CpuIndex] && IsSmrrSupported (NULL, &SmrrPhysMaskMsr)) {
+    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
     mSmrrEnabled[CpuIndex] = TRUE;
   }
 }
 
 /**
@@ -458,12 +455,49 @@ EFIAPI
 SmmCpuFeaturesIsSmmRegisterSupported (
   IN UINTN         CpuIndex,
   IN SMM_REG_NAME  RegName
   )
 {
-  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
-    return TRUE;
+  UINT32  RegEax;
+  UINT32  RegEdx;
+  UINTN   FamilyId;
+  UINTN   ModelId;
+
+  if (RegName == SmmRegFeatureControl) {
+    //
+    // Retrieve CPU Family and Model
+    //
+    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
+    FamilyId = (RegEax >> 8) & 0xf;
+    ModelId  = (RegEax >> 4) & 0xf;
+    if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
+      ModelId = ModelId | ((RegEax >> 12) & 0xf0);
+    }
+
+    //
+    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+    // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
+    // Processor Family.
+    //
+    // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th Generation
+    // Intel(R) Core(TM) Processor Family MSRs.
+    //
+    if (FamilyId == 0x06) {
+      if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
+          (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) || (ModelId == 0x4F) ||
+          (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) || (ModelId == 0x5C) ||
+          (ModelId == 0x8C))
+      {
+        //
+        // Check to see if the CPU supports the SMM Code Access Check feature
+        // Do not access this MSR unless the CPU supports the SmmRegFeatureControl
+        //
+        if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) & SMM_CODE_ACCESS_CHK_BIT) != 0) {
+          return TRUE;
+        }
+      }
+    }
   }
 
   return FALSE;
 }
 
@@ -484,11 +518,11 @@ EFIAPI
 SmmCpuFeaturesGetSmmRegister (
   IN UINTN         CpuIndex,
   IN SMM_REG_NAME  RegName
   )
 {
-  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
+  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
     return AsmReadMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL);
   }
 
   return 0;
 }
@@ -510,11 +544,11 @@ SmmCpuFeaturesSetSmmRegister (
   IN UINTN         CpuIndex,
   IN SMM_REG_NAME  RegName,
   IN UINT64        Value
   )
 {
-  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
+  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
     AsmWriteMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL, Value);
   }
 }
 
 /**
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91432): https://edk2.groups.io/g/devel/message/91432
Mute This Topic: https://groups.io/mt/92435488/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability
Posted by Michael D Kinney 1 year, 9 months ago
Are these checks made on every SMI?

What is the impact to SMI latency to do the check dynamically?

FeatureFlag and FixedAtBuild PCDs are declared as const global variables
which are used by optimizing compiler as constants in instructions or
optimize away condition checks all together.  This option should still
be considered.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin
> Sent: Sunday, July 17, 2022 1:38 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3962
> 
> Two SMM variables (mSmrrSupported & mSmmFeatureControlSupported) are global
> variables, they control whether the SMRR and SMM Feature Control MSR will
> be restored respectively.
> To avoid the TOCTOU, dynamic check SMRR enable & SmmFeatureControl capability.
> 
> Change-Id: I6835e4b0e12c5e6f52effb60fd9224e3eb97fc0d
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c    | 248 ++++++++++++---------
>  1 file changed, 141 insertions(+), 107 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> index 78de7f8407..b2f31c993f 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> @@ -35,26 +35,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  // MSRs required for configuration of SMM Code Access Check
>  //
>  #define SMM_FEATURES_LIB_IA32_MCA_CAP  0x17D
>  #define   SMM_CODE_ACCESS_CHK_BIT      BIT58
> 
> -//
> -// Set default value to assume SMRR is not supported
> -//
> -BOOLEAN  mSmrrSupported = FALSE;
> -
> -//
> -// Set default value to assume MSR_SMM_FEATURE_CONTROL is not supported
> -//
> -BOOLEAN  mSmmFeatureControlSupported = FALSE;
> -
> -//
> -// Set default value to assume IA-32 Architectural MSRs are used
> -//
> -UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> -UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> -
>  //
>  // Set default value to assume MTRRs need to be configured on each SMI
>  //
>  BOOLEAN  mNeedConfigureMtrrs = TRUE;
> 
> @@ -62,26 +46,39 @@ BOOLEAN  mNeedConfigureMtrrs = TRUE;
>  // Array for state of SMRR enable on all CPUs
>  //
>  BOOLEAN  *mSmrrEnabled;
> 
>  /**
> -  Performs library initialization.
> +  Return if SMRR is supported
> 
> -  This initialization function contains common functionality shared betwen all
> -  library instance constructors.
> +  @param[in] SmrrPhysBaseMsr   Pointer to SmrrPhysBaseMsr.
> +  @param[in] SmrrPhysMaskMsr   Pointer to SmrrPhysMaskMsr.
> +
> +  @retval TRUE  SMRR is supported.
> +  @retval FALSE SMRR is not supported.
> 
>  **/
> -VOID
> -CpuFeaturesLibInitialization (
> -  VOID
> +BOOLEAN
> +IsSmrrSupported (
> +  IN UINT32  *SmrrPhysBaseMsr    OPTIONAL,
> +  IN UINT32  *SmrrPhysMaskMsr    OPTIONAL
>    )
>  {
> +  BOOLEAN  ReturnValue;
> +
>    UINT32  RegEax;
>    UINT32  RegEdx;
>    UINTN   FamilyId;
>    UINTN   ModelId;
> 
> +  UINT64  FeatureControl;
> +
> +  //
> +  // Set default value to assume SMRR is not supported
> +  //
> +  ReturnValue = FALSE;
> +
>    //
>    // Retrieve CPU Family and Model
>    //
>    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
>    FamilyId = (RegEax >> 8) & 0xf;
> @@ -96,11 +93,11 @@ CpuFeaturesLibInitialization (
>    if ((RegEdx & BIT12) != 0) {
>      //
>      // Check MTRR_CAP MSR bit 11 for SMRR support
>      //
>      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0) {
> -      mSmrrSupported = TRUE;
> +      ReturnValue = TRUE;
>      }
>    }
> 
>    //
>    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> @@ -109,28 +106,79 @@ CpuFeaturesLibInitialization (
>    // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H, then
>    // SMRR Physical Base and SMM Physical Mask MSRs are not available.
>    //
>    if (FamilyId == 0x06) {
>      if ((ModelId == 0x1C) || (ModelId == 0x26) || (ModelId == 0x27) || (ModelId == 0x35) || (ModelId == 0x36)) {
> -      mSmrrSupported = FALSE;
> +      ReturnValue = FALSE;
>      }
>    }
> 
> -  //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
> -  //
> -  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
> -  // Processor Family MSRs
> -  //
> -  if (FamilyId == 0x06) {
> -    if ((ModelId == 0x17) || (ModelId == 0x0f)) {
> -      mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> -      mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> +  if (ReturnValue) {
> +    //
> +    // Return the SmrrPhysBaseMsr & SmrrPhysMaskMsr if required & Smrr Supported
> +    //
> +    if (SmrrPhysBaseMsr != NULL) {
> +      *SmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> +    }
> +
> +    if (SmrrPhysBaseMsr != NULL) {
> +      *SmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> +    }
> +
> +    //
> +    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> +    // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
> +    //
> +    // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
> +    // Processor Family MSRs
> +    //
> +    if (FamilyId == 0x06) {
> +      if ((ModelId == 0x17) || (ModelId == 0x0f)) {
> +        if (SmrrPhysBaseMsr != NULL) {
> +          *SmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> +        }
> +
> +        if (SmrrPhysMaskMsr != NULL) {
> +          *SmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> +        }
> +
> +        //
> +        // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> +        // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
> +        //
> +        // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being used, then
> +        // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A) is set before
> +        // accessing SMRR base/mask MSRs.  If Lock(BIT0) of MSR_FEATURE_CONTROL MSR(0x3A)
> +        // is set, then the MSR is locked and can not be modified.
> +        //
> +
> +        FeatureControl = AsmReadMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> +        if (((FeatureControl & BIT3) == 0) && ((FeatureControl & BIT0) == 1)) {
> +          ReturnValue = FALSE;
> +        }
> +      }
>      }
>    }
> 
> +  return ReturnValue;
> +}
> +
> +/**
> +  Performs library initialization.
> +
> +  This initialization function contains common functionality shared betwen all
> +  library instance constructors.
> +
> +**/
> +VOID
> +CpuFeaturesLibInitialization (
> +  VOID
> +  )
> +{
> +  UINT32  RegEax;
> +  UINT32  RegEdx;
> +
>    //
>    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
>    // Volume 3C, Section 34.4.2 SMRAM Caching
>    //   An IA-32 processor does not automatically write back and invalidate its
>    //   caches before entering SMM or before exiting SMM. Because of this behavior,
> @@ -193,50 +241,27 @@ SmmCpuFeaturesInitializeProcessor (
>    IN EFI_PROCESSOR_INFORMATION  *ProcessorInfo,
>    IN CPU_HOT_PLUG_DATA          *CpuHotPlugData
>    )
>  {
>    SMRAM_SAVE_STATE_MAP  *CpuState;
> -  UINT64                FeatureControl;
> -  UINT32                RegEax;
> -  UINT32                RegEdx;
> -  UINTN                 FamilyId;
> -  UINTN                 ModelId;
> +  UINT32                SmrrPhysBaseMsr;
> +  UINT32                SmrrPhysMaskMsr;
> 
>    //
>    // Configure SMBASE.
>    //
>    CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
>    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> 
> -  //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
> -  //
> -  // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being used, then
> -  // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A) is set before
> -  // accessing SMRR base/mask MSRs.  If Lock(BIT0) of MSR_FEATURE_CONTROL MSR(0x3A)
> -  // is set, then the MSR is locked and can not be modified.
> -  //
> -  if (mSmrrSupported && (mSmrrPhysBaseMsr == SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE)) {
> -    FeatureControl = AsmReadMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> -    if ((FeatureControl & BIT3) == 0) {
> -      if ((FeatureControl & BIT0) == 0) {
> -        AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL, FeatureControl | BIT3);
> -      } else {
> -        mSmrrSupported = FALSE;
> -      }
> -    }
> -  }
> -
>    //
>    // If SMRR is supported, then program SMRR base/mask MSRs.
>    // The EFI_MSR_SMRR_PHYS_MASK_VALID bit is not set until the first normal SMI.
>    // The code that initializes SMM environment is running in normal mode
>    // from SMRAM region.  If SMRR is enabled here, then the SMRAM region
>    // is protected and the normal mode code execution will fail.
>    //
> -  if (mSmrrSupported) {
> +  if (IsSmrrSupported (&SmrrPhysBaseMsr, &SmrrPhysMaskMsr)) {
>      //
>      // SMRR size cannot be less than 4-KBytes
>      // SMRR size must be of length 2^n
>      // SMRR base alignment cannot be less than SMRR length
>      //
> @@ -250,50 +275,16 @@ SmmCpuFeaturesInitializeProcessor (
>        if (IsMonarch) {
>          DEBUG ((DEBUG_ERROR, "SMM Base/Size does not meet alignment/size requirement!\n"));
>          CpuDeadLoop ();
>        }
>      } else {
> -      AsmWriteMsr64 (mSmrrPhysBaseMsr, CpuHotPlugData->SmrrBase | MTRR_CACHE_WRITE_BACK);
> -      AsmWriteMsr64 (mSmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1) & EFI_MSR_SMRR_MASK));
> +      AsmWriteMsr64 (SmrrPhysBaseMsr, CpuHotPlugData->SmrrBase | MTRR_CACHE_WRITE_BACK);
> +      AsmWriteMsr64 (SmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1) & EFI_MSR_SMRR_MASK));
>        mSmrrEnabled[CpuIndex] = FALSE;
>      }
>    }
> 
> -  //
> -  // Retrieve CPU Family and Model
> -  //
> -  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> -  FamilyId = (RegEax >> 8) & 0xf;
> -  ModelId  = (RegEax >> 4) & 0xf;
> -  if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
> -    ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> -  }
> -
> -  //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
> -  // Processor Family.
> -  //
> -  // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th Generation
> -  // Intel(R) Core(TM) Processor Family MSRs.
> -  //
> -  if (FamilyId == 0x06) {
> -    if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
> -        (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) || (ModelId == 0x4F) ||
> -        (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) || (ModelId == 0x5C) ||
> -        (ModelId == 0x8C))
> -    {
> -      //
> -      // Check to see if the CPU supports the SMM Code Access Check feature
> -      // Do not access this MSR unless the CPU supports the SmmRegFeatureControl
> -      //
> -      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) & SMM_CODE_ACCESS_CHK_BIT) != 0) {
> -        mSmmFeatureControlSupported = TRUE;
> -      }
> -    }
> -  }
> -
>    //
>    //  Call internal worker function that completes the CPU initialization
>    //
>    FinishSmmCpuFeaturesInitializeProcessor ();
>  }
> @@ -381,12 +372,14 @@ VOID
>  EFIAPI
>  SmmCpuFeaturesDisableSmrr (
>    VOID
>    )
>  {
> -  if (mSmrrSupported && mNeedConfigureMtrrs) {
> -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
> +  UINT32  SmrrPhysMaskMsr;
> +
> +  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) && mNeedConfigureMtrrs) {
> +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
>    }
>  }
> 
>  /**
>    Enable SMRR register if SMRR is supported and SmmCpuFeaturesNeedConfigureMtrrs()
> @@ -396,12 +389,14 @@ VOID
>  EFIAPI
>  SmmCpuFeaturesReenableSmrr (
>    VOID
>    )
>  {
> -  if (mSmrrSupported && mNeedConfigureMtrrs) {
> -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> +  UINT32  SmrrPhysMaskMsr;
> +
> +  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) && mNeedConfigureMtrrs) {
> +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
>    }
>  }
> 
>  /**
>    Processor specific hook point each time a CPU enters System Management Mode.
> @@ -414,15 +409,17 @@ VOID
>  EFIAPI
>  SmmCpuFeaturesRendezvousEntry (
>    IN UINTN  CpuIndex
>    )
>  {
> +  UINT32  SmrrPhysMaskMsr;
> +
>    //
>    // If SMRR is supported and this is the first normal SMI, then enable SMRR
>    //
> -  if (mSmrrSupported && !mSmrrEnabled[CpuIndex]) {
> -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> +  if (!mSmrrEnabled[CpuIndex] && IsSmrrSupported (NULL, &SmrrPhysMaskMsr)) {
> +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
>      mSmrrEnabled[CpuIndex] = TRUE;
>    }
>  }
> 
>  /**
> @@ -458,12 +455,49 @@ EFIAPI
>  SmmCpuFeaturesIsSmmRegisterSupported (
>    IN UINTN         CpuIndex,
>    IN SMM_REG_NAME  RegName
>    )
>  {
> -  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
> -    return TRUE;
> +  UINT32  RegEax;
> +  UINT32  RegEdx;
> +  UINTN   FamilyId;
> +  UINTN   ModelId;
> +
> +  if (RegName == SmmRegFeatureControl) {
> +    //
> +    // Retrieve CPU Family and Model
> +    //
> +    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> +    FamilyId = (RegEax >> 8) & 0xf;
> +    ModelId  = (RegEax >> 4) & 0xf;
> +    if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
> +      ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> +    }
> +
> +    //
> +    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> +    // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
> +    // Processor Family.
> +    //
> +    // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th Generation
> +    // Intel(R) Core(TM) Processor Family MSRs.
> +    //
> +    if (FamilyId == 0x06) {
> +      if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
> +          (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) || (ModelId == 0x4F) ||
> +          (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) || (ModelId == 0x5C) ||
> +          (ModelId == 0x8C))
> +      {
> +        //
> +        // Check to see if the CPU supports the SMM Code Access Check feature
> +        // Do not access this MSR unless the CPU supports the SmmRegFeatureControl
> +        //
> +        if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) & SMM_CODE_ACCESS_CHK_BIT) != 0) {
> +          return TRUE;
> +        }
> +      }
> +    }
>    }
> 
>    return FALSE;
>  }
> 
> @@ -484,11 +518,11 @@ EFIAPI
>  SmmCpuFeaturesGetSmmRegister (
>    IN UINTN         CpuIndex,
>    IN SMM_REG_NAME  RegName
>    )
>  {
> -  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
> +  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
>      return AsmReadMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL);
>    }
> 
>    return 0;
>  }
> @@ -510,11 +544,11 @@ SmmCpuFeaturesSetSmmRegister (
>    IN UINTN         CpuIndex,
>    IN SMM_REG_NAME  RegName,
>    IN UINT64        Value
>    )
>  {
> -  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
> +  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
>      AsmWriteMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL, Value);
>    }
>  }
> 
>  /**
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91434): https://edk2.groups.io/g/devel/message/91434
Mute This Topic: https://groups.io/mt/92435488/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability
Posted by Wu, Jiaxin 1 year, 9 months ago
Due to the SMI latency impact for IA-32 processor, I will drop this change & replace with the PCD check. I will resend the new patch for review.

Thanks,
Jiaxin  

> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Monday, July 18, 2022 3:32 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>
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable
> & SmmFeatureControl capability
> 
> Hi Mike,
> 
> Thanks the comments. Only IA-32 processor will check on every SMI since it
> needs configure Mtrr. Do you think the impact is acceptable or not?
> 
> For fixed PCD solution, the original concern: the fixed PCD will be treated as
> global variable. Should we need consider no compiler optimization case or it
> must be optimized away condition checks?
> 
> Thanks,
> Jiaxin
> 
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Monday, July 18, 2022 8:13 AM
> > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>; Kinney, Michael
> > D <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR
> enable
> > & SmmFeatureControl capability
> >
> > Are these checks made on every SMI?
> >
> > What is the impact to SMI latency to do the check dynamically?
> >
> > FeatureFlag and FixedAtBuild PCDs are declared as const global variables
> > which are used by optimizing compiler as constants in instructions or
> > optimize away condition checks all together.  This option should still
> > be considered.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> > Jiaxin
> > > Sent: Sunday, July 17, 2022 1:38 AM
> > > To: devel@edk2.groups.io
> > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> > > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable
> &
> > SmmFeatureControl capability
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3962
> > >
> > > Two SMM variables (mSmrrSupported & mSmmFeatureControlSupported)
> > are global
> > > variables, they control whether the SMRR and SMM Feature Control MSR
> will
> > > be restored respectively.
> > > To avoid the TOCTOU, dynamic check SMRR enable & SmmFeatureControl
> > capability.
> > >
> > > Change-Id: I6835e4b0e12c5e6f52effb60fd9224e3eb97fc0d
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> > > ---
> > >  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c    | 248
> > ++++++++++++---------
> > >  1 file changed, 141 insertions(+), 107 deletions(-)
> > >
> > > diff --git
> > a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > > index 78de7f8407..b2f31c993f 100644
> > > ---
> > a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > > +++
> > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > > @@ -35,26 +35,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >  // MSRs required for configuration of SMM Code Access Check
> > >  //
> > >  #define SMM_FEATURES_LIB_IA32_MCA_CAP  0x17D
> > >  #define   SMM_CODE_ACCESS_CHK_BIT      BIT58
> > >
> > > -//
> > > -// Set default value to assume SMRR is not supported
> > > -//
> > > -BOOLEAN  mSmrrSupported = FALSE;
> > > -
> > > -//
> > > -// Set default value to assume MSR_SMM_FEATURE_CONTROL is not
> > supported
> > > -//
> > > -BOOLEAN  mSmmFeatureControlSupported = FALSE;
> > > -
> > > -//
> > > -// Set default value to assume IA-32 Architectural MSRs are used
> > > -//
> > > -UINT32  mSmrrPhysBaseMsr =
> SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> > > -UINT32  mSmrrPhysMaskMsr =
> > SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> > > -
> > >  //
> > >  // Set default value to assume MTRRs need to be configured on each SMI
> > >  //
> > >  BOOLEAN  mNeedConfigureMtrrs = TRUE;
> > >
> > > @@ -62,26 +46,39 @@ BOOLEAN  mNeedConfigureMtrrs = TRUE;
> > >  // Array for state of SMRR enable on all CPUs
> > >  //
> > >  BOOLEAN  *mSmrrEnabled;
> > >
> > >  /**
> > > -  Performs library initialization.
> > > +  Return if SMRR is supported
> > >
> > > -  This initialization function contains common functionality shared betwen
> all
> > > -  library instance constructors.
> > > +  @param[in] SmrrPhysBaseMsr   Pointer to SmrrPhysBaseMsr.
> > > +  @param[in] SmrrPhysMaskMsr   Pointer to SmrrPhysMaskMsr.
> > > +
> > > +  @retval TRUE  SMRR is supported.
> > > +  @retval FALSE SMRR is not supported.
> > >
> > >  **/
> > > -VOID
> > > -CpuFeaturesLibInitialization (
> > > -  VOID
> > > +BOOLEAN
> > > +IsSmrrSupported (
> > > +  IN UINT32  *SmrrPhysBaseMsr    OPTIONAL,
> > > +  IN UINT32  *SmrrPhysMaskMsr    OPTIONAL
> > >    )
> > >  {
> > > +  BOOLEAN  ReturnValue;
> > > +
> > >    UINT32  RegEax;
> > >    UINT32  RegEdx;
> > >    UINTN   FamilyId;
> > >    UINTN   ModelId;
> > >
> > > +  UINT64  FeatureControl;
> > > +
> > > +  //
> > > +  // Set default value to assume SMRR is not supported
> > > +  //
> > > +  ReturnValue = FALSE;
> > > +
> > >    //
> > >    // Retrieve CPU Family and Model
> > >    //
> > >    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> > >    FamilyId = (RegEax >> 8) & 0xf;
> > > @@ -96,11 +93,11 @@ CpuFeaturesLibInitialization (
> > >    if ((RegEdx & BIT12) != 0) {
> > >      //
> > >      // Check MTRR_CAP MSR bit 11 for SMRR support
> > >      //
> > >      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) !=
> 0)
> > {
> > > -      mSmrrSupported = TRUE;
> > > +      ReturnValue = TRUE;
> > >      }
> > >    }
> > >
> > >    //
> > >    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > @@ -109,28 +106,79 @@ CpuFeaturesLibInitialization (
> > >    // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H,
> > then
> > >    // SMRR Physical Base and SMM Physical Mask MSRs are not available.
> > >    //
> > >    if (FamilyId == 0x06) {
> > >      if ((ModelId == 0x1C) || (ModelId == 0x26) || (ModelId == 0x27) ||
> > (ModelId == 0x35) || (ModelId == 0x36)) {
> > > -      mSmrrSupported = FALSE;
> > > +      ReturnValue = FALSE;
> > >      }
> > >    }
> > >
> > > -  //
> > > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> > Family
> > > -  //
> > > -  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
> > > -  // Processor Family MSRs
> > > -  //
> > > -  if (FamilyId == 0x06) {
> > > -    if ((ModelId == 0x17) || (ModelId == 0x0f)) {
> > > -      mSmrrPhysBaseMsr =
> > SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> > > -      mSmrrPhysMaskMsr =
> > SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> > > +  if (ReturnValue) {
> > > +    //
> > > +    // Return the SmrrPhysBaseMsr & SmrrPhysMaskMsr if required &
> Smrr
> > Supported
> > > +    //
> > > +    if (SmrrPhysBaseMsr != NULL) {
> > > +      *SmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> > > +    }
> > > +
> > > +    if (SmrrPhysBaseMsr != NULL) {
> > > +      *SmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> > > +    }
> > > +
> > > +    //
> > > +    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > +    // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> > Family
> > > +    //
> > > +    // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
> > > +    // Processor Family MSRs
> > > +    //
> > > +    if (FamilyId == 0x06) {
> > > +      if ((ModelId == 0x17) || (ModelId == 0x0f)) {
> > > +        if (SmrrPhysBaseMsr != NULL) {
> > > +          *SmrrPhysBaseMsr =
> > SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> > > +        }
> > > +
> > > +        if (SmrrPhysMaskMsr != NULL) {
> > > +          *SmrrPhysMaskMsr =
> > SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> > > +        }
> > > +
> > > +        //
> > > +        // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > +        // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> > Family
> > > +        //
> > > +        // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being
> > used, then
> > > +        // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL
> > MSR(0x3A) is set before
> > > +        // accessing SMRR base/mask MSRs.  If Lock(BIT0) of
> > MSR_FEATURE_CONTROL MSR(0x3A)
> > > +        // is set, then the MSR is locked and can not be modified.
> > > +        //
> > > +
> > > +        FeatureControl = AsmReadMsr64
> > (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> > > +        if (((FeatureControl & BIT3) == 0) && ((FeatureControl & BIT0) == 1))
> {
> > > +          ReturnValue = FALSE;
> > > +        }
> > > +      }
> > >      }
> > >    }
> > >
> > > +  return ReturnValue;
> > > +}
> > > +
> > > +/**
> > > +  Performs library initialization.
> > > +
> > > +  This initialization function contains common functionality shared betwen
> all
> > > +  library instance constructors.
> > > +
> > > +**/
> > > +VOID
> > > +CpuFeaturesLibInitialization (
> > > +  VOID
> > > +  )
> > > +{
> > > +  UINT32  RegEax;
> > > +  UINT32  RegEdx;
> > > +
> > >    //
> > >    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > >    // Volume 3C, Section 34.4.2 SMRAM Caching
> > >    //   An IA-32 processor does not automatically write back and invalidate
> its
> > >    //   caches before entering SMM or before exiting SMM. Because of this
> > behavior,
> > > @@ -193,50 +241,27 @@ SmmCpuFeaturesInitializeProcessor (
> > >    IN EFI_PROCESSOR_INFORMATION  *ProcessorInfo,
> > >    IN CPU_HOT_PLUG_DATA          *CpuHotPlugData
> > >    )
> > >  {
> > >    SMRAM_SAVE_STATE_MAP  *CpuState;
> > > -  UINT64                FeatureControl;
> > > -  UINT32                RegEax;
> > > -  UINT32                RegEdx;
> > > -  UINTN                 FamilyId;
> > > -  UINTN                 ModelId;
> > > +  UINT32                SmrrPhysBaseMsr;
> > > +  UINT32                SmrrPhysMaskMsr;
> > >
> > >    //
> > >    // Configure SMBASE.
> > >    //
> > >    CpuState             = (SMRAM_SAVE_STATE_MAP
> > *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
> > >    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> > >
> > > -  //
> > > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> > Family
> > > -  //
> > > -  // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being used,
> > then
> > > -  // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A)
> > is set before
> > > -  // accessing SMRR base/mask MSRs.  If Lock(BIT0) of
> > MSR_FEATURE_CONTROL MSR(0x3A)
> > > -  // is set, then the MSR is locked and can not be modified.
> > > -  //
> > > -  if (mSmrrSupported && (mSmrrPhysBaseMsr ==
> > SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE)) {
> > > -    FeatureControl = AsmReadMsr64
> > (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> > > -    if ((FeatureControl & BIT3) == 0) {
> > > -      if ((FeatureControl & BIT0) == 0) {
> > > -        AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL,
> > FeatureControl | BIT3);
> > > -      } else {
> > > -        mSmrrSupported = FALSE;
> > > -      }
> > > -    }
> > > -  }
> > > -
> > >    //
> > >    // If SMRR is supported, then program SMRR base/mask MSRs.
> > >    // The EFI_MSR_SMRR_PHYS_MASK_VALID bit is not set until the first
> > normal SMI.
> > >    // The code that initializes SMM environment is running in normal mode
> > >    // from SMRAM region.  If SMRR is enabled here, then the SMRAM
> region
> > >    // is protected and the normal mode code execution will fail.
> > >    //
> > > -  if (mSmrrSupported) {
> > > +  if (IsSmrrSupported (&SmrrPhysBaseMsr, &SmrrPhysMaskMsr)) {
> > >      //
> > >      // SMRR size cannot be less than 4-KBytes
> > >      // SMRR size must be of length 2^n
> > >      // SMRR base alignment cannot be less than SMRR length
> > >      //
> > > @@ -250,50 +275,16 @@ SmmCpuFeaturesInitializeProcessor (
> > >        if (IsMonarch) {
> > >          DEBUG ((DEBUG_ERROR, "SMM Base/Size does not meet
> > alignment/size requirement!\n"));
> > >          CpuDeadLoop ();
> > >        }
> > >      } else {
> > > -      AsmWriteMsr64 (mSmrrPhysBaseMsr, CpuHotPlugData->SmrrBase |
> > MTRR_CACHE_WRITE_BACK);
> > > -      AsmWriteMsr64 (mSmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize -
> 1)
> > & EFI_MSR_SMRR_MASK));
> > > +      AsmWriteMsr64 (SmrrPhysBaseMsr, CpuHotPlugData->SmrrBase |
> > MTRR_CACHE_WRITE_BACK);
> > > +      AsmWriteMsr64 (SmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1)
> > & EFI_MSR_SMRR_MASK));
> > >        mSmrrEnabled[CpuIndex] = FALSE;
> > >      }
> > >    }
> > >
> > > -  //
> > > -  // Retrieve CPU Family and Model
> > > -  //
> > > -  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> > > -  FamilyId = (RegEax >> 8) & 0xf;
> > > -  ModelId  = (RegEax >> 4) & 0xf;
> > > -  if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
> > > -    ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> > > -  }
> > > -
> > > -  //
> > > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > -  // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
> > > -  // Processor Family.
> > > -  //
> > > -  // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th
> Generation
> > > -  // Intel(R) Core(TM) Processor Family MSRs.
> > > -  //
> > > -  if (FamilyId == 0x06) {
> > > -    if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
> > > -        (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) ||
> > (ModelId == 0x4F) ||
> > > -        (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) ||
> > (ModelId == 0x5C) ||
> > > -        (ModelId == 0x8C))
> > > -    {
> > > -      //
> > > -      // Check to see if the CPU supports the SMM Code Access Check
> feature
> > > -      // Do not access this MSR unless the CPU supports the
> > SmmRegFeatureControl
> > > -      //
> > > -      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) &
> > SMM_CODE_ACCESS_CHK_BIT) != 0) {
> > > -        mSmmFeatureControlSupported = TRUE;
> > > -      }
> > > -    }
> > > -  }
> > > -
> > >    //
> > >    //  Call internal worker function that completes the CPU initialization
> > >    //
> > >    FinishSmmCpuFeaturesInitializeProcessor ();
> > >  }
> > > @@ -381,12 +372,14 @@ VOID
> > >  EFIAPI
> > >  SmmCpuFeaturesDisableSmrr (
> > >    VOID
> > >    )
> > >  {
> > > -  if (mSmrrSupported && mNeedConfigureMtrrs) {
> > > -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> > (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
> > > +  UINT32  SmrrPhysMaskMsr;
> > > +
> > > +  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) &&
> > mNeedConfigureMtrrs) {
> > > +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64
> (SmrrPhysMaskMsr)
> > & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
> > >    }
> > >  }
> > >
> > >  /**
> > >    Enable SMRR register if SMRR is supported and
> > SmmCpuFeaturesNeedConfigureMtrrs()
> > > @@ -396,12 +389,14 @@ VOID
> > >  EFIAPI
> > >  SmmCpuFeaturesReenableSmrr (
> > >    VOID
> > >    )
> > >  {
> > > -  if (mSmrrSupported && mNeedConfigureMtrrs) {
> > > -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> > (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> > > +  UINT32  SmrrPhysMaskMsr;
> > > +
> > > +  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) &&
> > mNeedConfigureMtrrs) {
> > > +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64
> (SmrrPhysMaskMsr)
> > | EFI_MSR_SMRR_PHYS_MASK_VALID);
> > >    }
> > >  }
> > >
> > >  /**
> > >    Processor specific hook point each time a CPU enters System
> Management
> > Mode.
> > > @@ -414,15 +409,17 @@ VOID
> > >  EFIAPI
> > >  SmmCpuFeaturesRendezvousEntry (
> > >    IN UINTN  CpuIndex
> > >    )
> > >  {
> > > +  UINT32  SmrrPhysMaskMsr;
> > > +
> > >    //
> > >    // If SMRR is supported and this is the first normal SMI, then enable SMRR
> > >    //
> > > -  if (mSmrrSupported && !mSmrrEnabled[CpuIndex]) {
> > > -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> > (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> > > +  if (!mSmrrEnabled[CpuIndex] && IsSmrrSupported (NULL,
> > &SmrrPhysMaskMsr)) {
> > > +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64
> (SmrrPhysMaskMsr)
> > | EFI_MSR_SMRR_PHYS_MASK_VALID);
> > >      mSmrrEnabled[CpuIndex] = TRUE;
> > >    }
> > >  }
> > >
> > >  /**
> > > @@ -458,12 +455,49 @@ EFIAPI
> > >  SmmCpuFeaturesIsSmmRegisterSupported (
> > >    IN UINTN         CpuIndex,
> > >    IN SMM_REG_NAME  RegName
> > >    )
> > >  {
> > > -  if (mSmmFeatureControlSupported && (RegName ==
> > SmmRegFeatureControl)) {
> > > -    return TRUE;
> > > +  UINT32  RegEax;
> > > +  UINT32  RegEdx;
> > > +  UINTN   FamilyId;
> > > +  UINTN   ModelId;
> > > +
> > > +  if (RegName == SmmRegFeatureControl) {
> > > +    //
> > > +    // Retrieve CPU Family and Model
> > > +    //
> > > +    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> > > +    FamilyId = (RegEax >> 8) & 0xf;
> > > +    ModelId  = (RegEax >> 4) & 0xf;
> > > +    if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
> > > +      ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> > > +    }
> > > +
> > > +    //
> > > +    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > > +    // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
> > > +    // Processor Family.
> > > +    //
> > > +    // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th
> > Generation
> > > +    // Intel(R) Core(TM) Processor Family MSRs.
> > > +    //
> > > +    if (FamilyId == 0x06) {
> > > +      if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
> > > +          (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) ||
> > (ModelId == 0x4F) ||
> > > +          (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) ||
> > (ModelId == 0x5C) ||
> > > +          (ModelId == 0x8C))
> > > +      {
> > > +        //
> > > +        // Check to see if the CPU supports the SMM Code Access Check
> > feature
> > > +        // Do not access this MSR unless the CPU supports the
> > SmmRegFeatureControl
> > > +        //
> > > +        if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) &
> > SMM_CODE_ACCESS_CHK_BIT) != 0) {
> > > +          return TRUE;
> > > +        }
> > > +      }
> > > +    }
> > >    }
> > >
> > >    return FALSE;
> > >  }
> > >
> > > @@ -484,11 +518,11 @@ EFIAPI
> > >  SmmCpuFeaturesGetSmmRegister (
> > >    IN UINTN         CpuIndex,
> > >    IN SMM_REG_NAME  RegName
> > >    )
> > >  {
> > > -  if (mSmmFeatureControlSupported && (RegName ==
> > SmmRegFeatureControl)) {
> > > +  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
> > >      return AsmReadMsr64
> (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL);
> > >    }
> > >
> > >    return 0;
> > >  }
> > > @@ -510,11 +544,11 @@ SmmCpuFeaturesSetSmmRegister (
> > >    IN UINTN         CpuIndex,
> > >    IN SMM_REG_NAME  RegName,
> > >    IN UINT64        Value
> > >    )
> > >  {
> > > -  if (mSmmFeatureControlSupported && (RegName ==
> > SmmRegFeatureControl)) {
> > > +  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
> > >      AsmWriteMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL,
> Value);
> > >    }
> > >  }
> > >
> > >  /**
> > > --
> > > 2.16.2.windows.1
> > >
> > >
> > >
> > > 
> > >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91961): https://edk2.groups.io/g/devel/message/91961
Mute This Topic: https://groups.io/mt/92435488/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability
Posted by Wu, Jiaxin 1 year, 9 months ago
Hi Mike,

Thanks the comments. Only IA-32 processor will check on every SMI since it needs configure Mtrr. Do you think the impact is acceptable or not?

For fixed PCD solution, the original concern: the fixed PCD will be treated as global variable. Should we need consider no compiler optimization case or it must be optimized away condition checks?

Thanks,
Jiaxin


> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, July 18, 2022 8:13 AM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable
> & SmmFeatureControl capability
> 
> Are these checks made on every SMI?
> 
> What is the impact to SMI latency to do the check dynamically?
> 
> FeatureFlag and FixedAtBuild PCDs are declared as const global variables
> which are used by optimizing compiler as constants in instructions or
> optimize away condition checks all together.  This option should still
> be considered.
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> > Sent: Sunday, July 17, 2022 1:38 AM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable &
> SmmFeatureControl capability
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3962
> >
> > Two SMM variables (mSmrrSupported & mSmmFeatureControlSupported)
> are global
> > variables, they control whether the SMRR and SMM Feature Control MSR will
> > be restored respectively.
> > To avoid the TOCTOU, dynamic check SMRR enable & SmmFeatureControl
> capability.
> >
> > Change-Id: I6835e4b0e12c5e6f52effb60fd9224e3eb97fc0d
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> > ---
> >  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c    | 248
> ++++++++++++---------
> >  1 file changed, 141 insertions(+), 107 deletions(-)
> >
> > diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > index 78de7f8407..b2f31c993f 100644
> > ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> > @@ -35,26 +35,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >  // MSRs required for configuration of SMM Code Access Check
> >  //
> >  #define SMM_FEATURES_LIB_IA32_MCA_CAP  0x17D
> >  #define   SMM_CODE_ACCESS_CHK_BIT      BIT58
> >
> > -//
> > -// Set default value to assume SMRR is not supported
> > -//
> > -BOOLEAN  mSmrrSupported = FALSE;
> > -
> > -//
> > -// Set default value to assume MSR_SMM_FEATURE_CONTROL is not
> supported
> > -//
> > -BOOLEAN  mSmmFeatureControlSupported = FALSE;
> > -
> > -//
> > -// Set default value to assume IA-32 Architectural MSRs are used
> > -//
> > -UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> > -UINT32  mSmrrPhysMaskMsr =
> SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> > -
> >  //
> >  // Set default value to assume MTRRs need to be configured on each SMI
> >  //
> >  BOOLEAN  mNeedConfigureMtrrs = TRUE;
> >
> > @@ -62,26 +46,39 @@ BOOLEAN  mNeedConfigureMtrrs = TRUE;
> >  // Array for state of SMRR enable on all CPUs
> >  //
> >  BOOLEAN  *mSmrrEnabled;
> >
> >  /**
> > -  Performs library initialization.
> > +  Return if SMRR is supported
> >
> > -  This initialization function contains common functionality shared betwen all
> > -  library instance constructors.
> > +  @param[in] SmrrPhysBaseMsr   Pointer to SmrrPhysBaseMsr.
> > +  @param[in] SmrrPhysMaskMsr   Pointer to SmrrPhysMaskMsr.
> > +
> > +  @retval TRUE  SMRR is supported.
> > +  @retval FALSE SMRR is not supported.
> >
> >  **/
> > -VOID
> > -CpuFeaturesLibInitialization (
> > -  VOID
> > +BOOLEAN
> > +IsSmrrSupported (
> > +  IN UINT32  *SmrrPhysBaseMsr    OPTIONAL,
> > +  IN UINT32  *SmrrPhysMaskMsr    OPTIONAL
> >    )
> >  {
> > +  BOOLEAN  ReturnValue;
> > +
> >    UINT32  RegEax;
> >    UINT32  RegEdx;
> >    UINTN   FamilyId;
> >    UINTN   ModelId;
> >
> > +  UINT64  FeatureControl;
> > +
> > +  //
> > +  // Set default value to assume SMRR is not supported
> > +  //
> > +  ReturnValue = FALSE;
> > +
> >    //
> >    // Retrieve CPU Family and Model
> >    //
> >    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> >    FamilyId = (RegEax >> 8) & 0xf;
> > @@ -96,11 +93,11 @@ CpuFeaturesLibInitialization (
> >    if ((RegEdx & BIT12) != 0) {
> >      //
> >      // Check MTRR_CAP MSR bit 11 for SMRR support
> >      //
> >      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0)
> {
> > -      mSmrrSupported = TRUE;
> > +      ReturnValue = TRUE;
> >      }
> >    }
> >
> >    //
> >    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > @@ -109,28 +106,79 @@ CpuFeaturesLibInitialization (
> >    // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H,
> then
> >    // SMRR Physical Base and SMM Physical Mask MSRs are not available.
> >    //
> >    if (FamilyId == 0x06) {
> >      if ((ModelId == 0x1C) || (ModelId == 0x26) || (ModelId == 0x27) ||
> (ModelId == 0x35) || (ModelId == 0x36)) {
> > -      mSmrrSupported = FALSE;
> > +      ReturnValue = FALSE;
> >      }
> >    }
> >
> > -  //
> > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> Family
> > -  //
> > -  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
> > -  // Processor Family MSRs
> > -  //
> > -  if (FamilyId == 0x06) {
> > -    if ((ModelId == 0x17) || (ModelId == 0x0f)) {
> > -      mSmrrPhysBaseMsr =
> SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> > -      mSmrrPhysMaskMsr =
> SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> > +  if (ReturnValue) {
> > +    //
> > +    // Return the SmrrPhysBaseMsr & SmrrPhysMaskMsr if required & Smrr
> Supported
> > +    //
> > +    if (SmrrPhysBaseMsr != NULL) {
> > +      *SmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> > +    }
> > +
> > +    if (SmrrPhysBaseMsr != NULL) {
> > +      *SmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> > +    }
> > +
> > +    //
> > +    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > +    // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> Family
> > +    //
> > +    // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2
> > +    // Processor Family MSRs
> > +    //
> > +    if (FamilyId == 0x06) {
> > +      if ((ModelId == 0x17) || (ModelId == 0x0f)) {
> > +        if (SmrrPhysBaseMsr != NULL) {
> > +          *SmrrPhysBaseMsr =
> SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> > +        }
> > +
> > +        if (SmrrPhysMaskMsr != NULL) {
> > +          *SmrrPhysMaskMsr =
> SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> > +        }
> > +
> > +        //
> > +        // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > +        // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> Family
> > +        //
> > +        // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being
> used, then
> > +        // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL
> MSR(0x3A) is set before
> > +        // accessing SMRR base/mask MSRs.  If Lock(BIT0) of
> MSR_FEATURE_CONTROL MSR(0x3A)
> > +        // is set, then the MSR is locked and can not be modified.
> > +        //
> > +
> > +        FeatureControl = AsmReadMsr64
> (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> > +        if (((FeatureControl & BIT3) == 0) && ((FeatureControl & BIT0) == 1)) {
> > +          ReturnValue = FALSE;
> > +        }
> > +      }
> >      }
> >    }
> >
> > +  return ReturnValue;
> > +}
> > +
> > +/**
> > +  Performs library initialization.
> > +
> > +  This initialization function contains common functionality shared betwen all
> > +  library instance constructors.
> > +
> > +**/
> > +VOID
> > +CpuFeaturesLibInitialization (
> > +  VOID
> > +  )
> > +{
> > +  UINT32  RegEax;
> > +  UINT32  RegEdx;
> > +
> >    //
> >    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> >    // Volume 3C, Section 34.4.2 SMRAM Caching
> >    //   An IA-32 processor does not automatically write back and invalidate its
> >    //   caches before entering SMM or before exiting SMM. Because of this
> behavior,
> > @@ -193,50 +241,27 @@ SmmCpuFeaturesInitializeProcessor (
> >    IN EFI_PROCESSOR_INFORMATION  *ProcessorInfo,
> >    IN CPU_HOT_PLUG_DATA          *CpuHotPlugData
> >    )
> >  {
> >    SMRAM_SAVE_STATE_MAP  *CpuState;
> > -  UINT64                FeatureControl;
> > -  UINT32                RegEax;
> > -  UINT32                RegEdx;
> > -  UINTN                 FamilyId;
> > -  UINTN                 ModelId;
> > +  UINT32                SmrrPhysBaseMsr;
> > +  UINT32                SmrrPhysMaskMsr;
> >
> >    //
> >    // Configure SMBASE.
> >    //
> >    CpuState             = (SMRAM_SAVE_STATE_MAP
> *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
> >    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> >
> > -  //
> > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> Family
> > -  //
> > -  // If Intel(R) Core(TM) Core(TM) 2 Processor Family MSRs are being used,
> then
> > -  // make sure SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A)
> is set before
> > -  // accessing SMRR base/mask MSRs.  If Lock(BIT0) of
> MSR_FEATURE_CONTROL MSR(0x3A)
> > -  // is set, then the MSR is locked and can not be modified.
> > -  //
> > -  if (mSmrrSupported && (mSmrrPhysBaseMsr ==
> SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE)) {
> > -    FeatureControl = AsmReadMsr64
> (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> > -    if ((FeatureControl & BIT3) == 0) {
> > -      if ((FeatureControl & BIT0) == 0) {
> > -        AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL,
> FeatureControl | BIT3);
> > -      } else {
> > -        mSmrrSupported = FALSE;
> > -      }
> > -    }
> > -  }
> > -
> >    //
> >    // If SMRR is supported, then program SMRR base/mask MSRs.
> >    // The EFI_MSR_SMRR_PHYS_MASK_VALID bit is not set until the first
> normal SMI.
> >    // The code that initializes SMM environment is running in normal mode
> >    // from SMRAM region.  If SMRR is enabled here, then the SMRAM region
> >    // is protected and the normal mode code execution will fail.
> >    //
> > -  if (mSmrrSupported) {
> > +  if (IsSmrrSupported (&SmrrPhysBaseMsr, &SmrrPhysMaskMsr)) {
> >      //
> >      // SMRR size cannot be less than 4-KBytes
> >      // SMRR size must be of length 2^n
> >      // SMRR base alignment cannot be less than SMRR length
> >      //
> > @@ -250,50 +275,16 @@ SmmCpuFeaturesInitializeProcessor (
> >        if (IsMonarch) {
> >          DEBUG ((DEBUG_ERROR, "SMM Base/Size does not meet
> alignment/size requirement!\n"));
> >          CpuDeadLoop ();
> >        }
> >      } else {
> > -      AsmWriteMsr64 (mSmrrPhysBaseMsr, CpuHotPlugData->SmrrBase |
> MTRR_CACHE_WRITE_BACK);
> > -      AsmWriteMsr64 (mSmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1)
> & EFI_MSR_SMRR_MASK));
> > +      AsmWriteMsr64 (SmrrPhysBaseMsr, CpuHotPlugData->SmrrBase |
> MTRR_CACHE_WRITE_BACK);
> > +      AsmWriteMsr64 (SmrrPhysMaskMsr, (~(CpuHotPlugData->SmrrSize - 1)
> & EFI_MSR_SMRR_MASK));
> >        mSmrrEnabled[CpuIndex] = FALSE;
> >      }
> >    }
> >
> > -  //
> > -  // Retrieve CPU Family and Model
> > -  //
> > -  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> > -  FamilyId = (RegEax >> 8) & 0xf;
> > -  ModelId  = (RegEax >> 4) & 0xf;
> > -  if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
> > -    ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> > -  }
> > -
> > -  //
> > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > -  // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
> > -  // Processor Family.
> > -  //
> > -  // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th Generation
> > -  // Intel(R) Core(TM) Processor Family MSRs.
> > -  //
> > -  if (FamilyId == 0x06) {
> > -    if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
> > -        (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) ||
> (ModelId == 0x4F) ||
> > -        (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) ||
> (ModelId == 0x5C) ||
> > -        (ModelId == 0x8C))
> > -    {
> > -      //
> > -      // Check to see if the CPU supports the SMM Code Access Check feature
> > -      // Do not access this MSR unless the CPU supports the
> SmmRegFeatureControl
> > -      //
> > -      if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) &
> SMM_CODE_ACCESS_CHK_BIT) != 0) {
> > -        mSmmFeatureControlSupported = TRUE;
> > -      }
> > -    }
> > -  }
> > -
> >    //
> >    //  Call internal worker function that completes the CPU initialization
> >    //
> >    FinishSmmCpuFeaturesInitializeProcessor ();
> >  }
> > @@ -381,12 +372,14 @@ VOID
> >  EFIAPI
> >  SmmCpuFeaturesDisableSmrr (
> >    VOID
> >    )
> >  {
> > -  if (mSmrrSupported && mNeedConfigureMtrrs) {
> > -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
> > +  UINT32  SmrrPhysMaskMsr;
> > +
> > +  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) &&
> mNeedConfigureMtrrs) {
> > +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr)
> & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
> >    }
> >  }
> >
> >  /**
> >    Enable SMRR register if SMRR is supported and
> SmmCpuFeaturesNeedConfigureMtrrs()
> > @@ -396,12 +389,14 @@ VOID
> >  EFIAPI
> >  SmmCpuFeaturesReenableSmrr (
> >    VOID
> >    )
> >  {
> > -  if (mSmrrSupported && mNeedConfigureMtrrs) {
> > -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> > +  UINT32  SmrrPhysMaskMsr;
> > +
> > +  if (IsSmrrSupported (NULL, &SmrrPhysMaskMsr) &&
> mNeedConfigureMtrrs) {
> > +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr)
> | EFI_MSR_SMRR_PHYS_MASK_VALID);
> >    }
> >  }
> >
> >  /**
> >    Processor specific hook point each time a CPU enters System Management
> Mode.
> > @@ -414,15 +409,17 @@ VOID
> >  EFIAPI
> >  SmmCpuFeaturesRendezvousEntry (
> >    IN UINTN  CpuIndex
> >    )
> >  {
> > +  UINT32  SmrrPhysMaskMsr;
> > +
> >    //
> >    // If SMRR is supported and this is the first normal SMI, then enable SMRR
> >    //
> > -  if (mSmrrSupported && !mSmrrEnabled[CpuIndex]) {
> > -    AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> > +  if (!mSmrrEnabled[CpuIndex] && IsSmrrSupported (NULL,
> &SmrrPhysMaskMsr)) {
> > +    AsmWriteMsr64 (SmrrPhysMaskMsr, AsmReadMsr64 (SmrrPhysMaskMsr)
> | EFI_MSR_SMRR_PHYS_MASK_VALID);
> >      mSmrrEnabled[CpuIndex] = TRUE;
> >    }
> >  }
> >
> >  /**
> > @@ -458,12 +455,49 @@ EFIAPI
> >  SmmCpuFeaturesIsSmmRegisterSupported (
> >    IN UINTN         CpuIndex,
> >    IN SMM_REG_NAME  RegName
> >    )
> >  {
> > -  if (mSmmFeatureControlSupported && (RegName ==
> SmmRegFeatureControl)) {
> > -    return TRUE;
> > +  UINT32  RegEax;
> > +  UINT32  RegEdx;
> > +  UINTN   FamilyId;
> > +  UINTN   ModelId;
> > +
> > +  if (RegName == SmmRegFeatureControl) {
> > +    //
> > +    // Retrieve CPU Family and Model
> > +    //
> > +    AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
> > +    FamilyId = (RegEax >> 8) & 0xf;
> > +    ModelId  = (RegEax >> 4) & 0xf;
> > +    if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
> > +      ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> > +    }
> > +
> > +    //
> > +    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > +    // Volume 3C, Section 35.10.1 MSRs in 4th Generation Intel(R) Core(TM)
> > +    // Processor Family.
> > +    //
> > +    // If CPU Family/Model is 06_3C, 06_45, or 06_46 then use 4th
> Generation
> > +    // Intel(R) Core(TM) Processor Family MSRs.
> > +    //
> > +    if (FamilyId == 0x06) {
> > +      if ((ModelId == 0x3C) || (ModelId == 0x45) || (ModelId == 0x46) ||
> > +          (ModelId == 0x3D) || (ModelId == 0x47) || (ModelId == 0x4E) ||
> (ModelId == 0x4F) ||
> > +          (ModelId == 0x3F) || (ModelId == 0x56) || (ModelId == 0x57) ||
> (ModelId == 0x5C) ||
> > +          (ModelId == 0x8C))
> > +      {
> > +        //
> > +        // Check to see if the CPU supports the SMM Code Access Check
> feature
> > +        // Do not access this MSR unless the CPU supports the
> SmmRegFeatureControl
> > +        //
> > +        if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MCA_CAP) &
> SMM_CODE_ACCESS_CHK_BIT) != 0) {
> > +          return TRUE;
> > +        }
> > +      }
> > +    }
> >    }
> >
> >    return FALSE;
> >  }
> >
> > @@ -484,11 +518,11 @@ EFIAPI
> >  SmmCpuFeaturesGetSmmRegister (
> >    IN UINTN         CpuIndex,
> >    IN SMM_REG_NAME  RegName
> >    )
> >  {
> > -  if (mSmmFeatureControlSupported && (RegName ==
> SmmRegFeatureControl)) {
> > +  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
> >      return AsmReadMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL);
> >    }
> >
> >    return 0;
> >  }
> > @@ -510,11 +544,11 @@ SmmCpuFeaturesSetSmmRegister (
> >    IN UINTN         CpuIndex,
> >    IN SMM_REG_NAME  RegName,
> >    IN UINT64        Value
> >    )
> >  {
> > -  if (mSmmFeatureControlSupported && (RegName ==
> SmmRegFeatureControl)) {
> > +  if (SmmCpuFeaturesIsSmmRegisterSupported (CpuIndex, RegName)) {
> >      AsmWriteMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL, Value);
> >    }
> >  }
> >
> >  /**
> > --
> > 2.16.2.windows.1
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91437): https://edk2.groups.io/g/devel/message/91437
Mute This Topic: https://groups.io/mt/92435488/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-