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, add PCD to control SMRR & SmmFeatureControl enable.

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/SmmCpuFeaturesLib.inf        |  4 ++
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c    | 84 ++++------------------
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |  4 ++
 .../StandaloneMmCpuFeaturesLib.inf                 |  4 ++
 UefiCpuPkg/UefiCpuPkg.dec                          | 12 ++++
 UefiCpuPkg/UefiCpuPkg.uni                          | 12 ++++
 6 files changed, 48 insertions(+), 72 deletions(-)

diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 35292dac31..7b5cef9700 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -33,5 +33,9 @@
   MemoryAllocationLib
   DebugLib
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
+
+[FeaturePcd]
+  gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable  ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable  ## CONSUMES
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
index 78de7f8407..b88cdece2a 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
@@ -35,20 +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;
@@ -81,39 +71,27 @@ CpuFeaturesLibInitialization (
   UINTN   ModelId;
 
   //
   // Retrieve CPU Family and Model
   //
-  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
+  AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, NULL);
   FamilyId = (RegEax >> 8) & 0xf;
   ModelId  = (RegEax >> 4) & 0xf;
   if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
     ModelId = ModelId | ((RegEax >> 12) & 0xf0);
   }
 
-  //
-  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
-  //
-  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;
-    }
-  }
-
   //
   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
   // Volume 3C, Section 35.3 MSRs in the Intel(R) Atom(TM) Processor Family
   //
   // 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;
+      ASSERT (!FeaturePcdGet (PcdSmrrEnable));
     }
   }
 
   //
   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
@@ -194,14 +172,10 @@ SmmCpuFeaturesInitializeProcessor (
   IN CPU_HOT_PLUG_DATA          *CpuHotPlugData
   )
 {
   SMRAM_SAVE_STATE_MAP  *CpuState;
   UINT64                FeatureControl;
-  UINT32                RegEax;
-  UINT32                RegEdx;
-  UINTN                 FamilyId;
-  UINTN                 ModelId;
 
   //
   // Configure SMBASE.
   //
   CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
@@ -214,17 +188,17 @@ SmmCpuFeaturesInitializeProcessor (
   // 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)) {
+  if (mSmrrPhysBaseMsr == SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE) {
     FeatureControl = AsmReadMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
     if ((FeatureControl & BIT3) == 0) {
-      if ((FeatureControl & BIT0) == 0) {
+      if (((FeatureControl & BIT0) == 0) && (FeaturePcdGet (PcdSmrrEnable))) {
         AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL, FeatureControl | BIT3);
       } else {
-        mSmrrSupported = FALSE;
+        ASSERT (!FeaturePcdGet (PcdSmrrEnable));
       }
     }
   }
 
   //
@@ -232,11 +206,11 @@ SmmCpuFeaturesInitializeProcessor (
   // 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 (FeaturePcdGet (PcdSmrrEnable)) {
     //
     // 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
     //
@@ -256,44 +230,10 @@ SmmCpuFeaturesInitializeProcessor (
       AsmWriteMsr64 (mSmrrPhysMaskMsr, (~(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,11 +321,11 @@ VOID
 EFIAPI
 SmmCpuFeaturesDisableSmrr (
   VOID
   )
 {
-  if (mSmrrSupported && mNeedConfigureMtrrs) {
+  if (FeaturePcdGet (PcdSmrrEnable) && mNeedConfigureMtrrs) {
     AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
   }
 }
 
 /**
@@ -396,11 +336,11 @@ VOID
 EFIAPI
 SmmCpuFeaturesReenableSmrr (
   VOID
   )
 {
-  if (mSmrrSupported && mNeedConfigureMtrrs) {
+  if (FeaturePcdGet (PcdSmrrEnable) && mNeedConfigureMtrrs) {
     AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
   }
 }
 
 /**
@@ -417,11 +357,11 @@ SmmCpuFeaturesRendezvousEntry (
   )
 {
   //
   // If SMRR is supported and this is the first normal SMI, then enable SMRR
   //
-  if (mSmrrSupported && !mSmrrEnabled[CpuIndex]) {
+  if (FeaturePcdGet (PcdSmrrEnable) && !mSmrrEnabled[CpuIndex]) {
     AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
     mSmrrEnabled[CpuIndex] = TRUE;
   }
 }
 
@@ -458,11 +398,11 @@ EFIAPI
 SmmCpuFeaturesIsSmmRegisterSupported (
   IN UINTN         CpuIndex,
   IN SMM_REG_NAME  RegName
   )
 {
-  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
+  if (FeaturePcdGet (PcdSmmFeatureControlEnable) && (RegName == SmmRegFeatureControl)) {
     return TRUE;
   }
 
   return FALSE;
 }
@@ -484,11 +424,11 @@ EFIAPI
 SmmCpuFeaturesGetSmmRegister (
   IN UINTN         CpuIndex,
   IN SMM_REG_NAME  RegName
   )
 {
-  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
+  if (FeaturePcdGet (PcdSmmFeatureControlEnable) && (RegName == SmmRegFeatureControl)) {
     return AsmReadMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL);
   }
 
   return 0;
 }
@@ -510,11 +450,11 @@ SmmCpuFeaturesSetSmmRegister (
   IN UINTN         CpuIndex,
   IN SMM_REG_NAME  RegName,
   IN UINT64        Value
   )
 {
-  if (mSmmFeatureControlSupported && (RegName == SmmRegFeatureControl)) {
+  if (FeaturePcdGet (PcdSmmFeatureControlEnable) && (RegName == SmmRegFeatureControl)) {
     AsmWriteMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL, Value);
   }
 }
 
 /**
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 022351f593..85214ee31c 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -68,7 +68,11 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize                         ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize         ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
 
+[FeaturePcd]
+  gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable  ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable  ## CONSUMES
+
 [Depex]
   gEfiMpServiceProtocolGuid
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index ec97041d8b..3eacab48db 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -34,5 +34,9 @@
   MemoryAllocationLib
   PcdLib
 
 [FixedPcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
+
+[FeaturePcd]
+  gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable  ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable  ## CONSUMES
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 1951eb294c..55cbe7605f 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -153,10 +153,22 @@
   #   TRUE  - SMM Feature Control MSR will be locked.<BR>
   #   FALSE - SMM Feature Control MSR will not be locked.<BR>
   # @Prompt Lock SMM Feature Control MSR.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x3213210B
 
+  ## Indicates if SMRR will be enabled.<BR><BR>
+  #   TRUE  - SMRR will be enabled.<BR>
+  #   FALSE - SMRR will not be enabled.<BR>
+  # @Prompt Enable SMRR.
+  gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable|TRUE|BOOLEAN|0x3213210D
+
+  ## Indicates if SmmFeatureControl will be enabled.<BR><BR>
+  #   TRUE  - SmmFeatureControl will be enabled.<BR>
+  #   FALSE - SmmFeatureControl will not be enabled.<BR>
+  # @Prompt Support SmmFeatureControl.
+  gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable|TRUE|BOOLEAN|0x32132110
+
 [PcdsFixedAtBuild]
   ## List of exception vectors which need switching stack.
   #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
   #  By default exception #DD(8), #PF(14) are supported.
   # @Prompt Specify exception vectors which need switching stack.
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index 219c1963bf..d17bcfd10c 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -98,10 +98,22 @@
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP  #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
                                                                                            "TRUE  - locked.<BR>\n"
                                                                                            "FALSE - unlocked.<BR>"
 
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdSmrrEnable_PROMPT  #language en-US "Indicates if SMRR will be enabled."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdSmrrEnable_HELP  #language en-US "Indicates if SMRR will be enabled.<BR><BR>\n"
+                                                                                           "TRUE  - SMRR will be enabled.<BR>\n"
+                                                                                           "FALSE - SMRR will not be enabled.<BR>"
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdSmmFeatureControlEnable_PROMPT  #language en-US "Indicates if SmmFeatureControl will be enabled."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdSmmFeatureControlEnable_HELP  #language en-US "Indicates if SmmFeatureControl will be enabled.<BR><BR>\n"
+                                                                                           "TRUE  - SmmFeatureControl will be enabled.<BR>\n"
+                                                                                           "FALSE - SmmFeatureControl will not be enabled.<BR>"
+
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdPeiTemporaryRamStackSize_PROMPT  #language en-US "Stack size in the temporary RAM"
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdPeiTemporaryRamStackSize_HELP  #language en-US "Specifies stack size in the temporary RAM. 0 means half of TemporaryRamSize."
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmProfileSize_PROMPT  #language en-US "SMM profile data buffer size"
-- 
2.16.2.windows.1



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

> -  //
> -  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
> -  //
> -  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;

1. can we keep the logic but just replace the above line as "ASSERT (FeaturePcdGet (PcdSmrrEnable));"?

>      if ((FeatureControl & BIT3) == 0) {
> -      if ((FeatureControl & BIT0) == 0) {
> +      if (((FeatureControl & BIT0) == 0) && (FeaturePcdGet (PcdSmrrEnable)))
> {
>          AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL,
> FeatureControl | BIT3);
>        } else {
> -        mSmrrSupported = FALSE;
> +        ASSERT (!FeaturePcdGet (PcdSmrrEnable));

2. If PcdSmrrEnable is TRUE but the FeatureControl MSR is locked (BIT0 is set),
  above assertion will be hit. We may need to reconsider the code logic.

> -    {
> -      //
> -      // 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;

3. can we keep the logic but just replace the above line as "ASSERT (FeaturePcdGet (PcdSmmFeatureControlEnable))"?


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

Drop this patch replaced by new patch set "[edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability" since it's totally different solution for fix.

>  -----Original Message-----
> From: Wu, Jiaxin
> Sent: Wednesday, June 29, 2022 9:38 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable &
> SmmFeatureControl support
> 
> For below question 1&3:
> Since we have defined the PCD for the feature control, should we still need
> check the enable case? i though we only add the assert case for not support
> case, because we must make sure has the capability to enable it. But for
> support case, platform can still disable it via the pcd?
> 
> For below question 2:
> It's the intention because FeatureControl & BIT3 is 0, which means SMRR
> Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A) is *not* set before
> accessing SMRR base/mask MSRs, then ASSERT (!FeaturePcdGet
> (PcdSmrrEnable));
> 
> 
> Thanks,
> Jiaxin
> 
> 
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Tuesday, June 28, 2022 5:17 PM
> > To: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable &
> > SmmFeatureControl support
> >
> > > -  //
> > > -  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
> > > -  //
> > > -  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;
> >
> > 1. can we keep the logic but just replace the above line as "ASSERT
> > (FeaturePcdGet (PcdSmrrEnable));"?
> >
> > >      if ((FeatureControl & BIT3) == 0) {
> > > -      if ((FeatureControl & BIT0) == 0) {
> > > +      if (((FeatureControl & BIT0) == 0) && (FeaturePcdGet
> > (PcdSmrrEnable)))
> > > {
> > >          AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL,
> > > FeatureControl | BIT3);
> > >        } else {
> > > -        mSmrrSupported = FALSE;
> > > +        ASSERT (!FeaturePcdGet (PcdSmrrEnable));
> >
> > 2. If PcdSmrrEnable is TRUE but the FeatureControl MSR is locked (BIT0 is set),
> >   above assertion will be hit. We may need to reconsider the code logic.
> >
> > > -    {
> > > -      //
> > > -      // 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;
> >
> > 3. can we keep the logic but just replace the above line as "ASSERT
> > (FeaturePcdGet (PcdSmmFeatureControlEnable))"?


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

For below question 1&3:
Since we have defined the PCD for the feature control, should we still need check the enable case? i though we only add the assert case for not support case, because we must make sure has the capability to enable it. But for support case, platform can still disable it via the pcd?

For below question 2:
It's the intention because FeatureControl & BIT3 is 0, which means SMRR Enable(BIT3) of MSR_FEATURE_CONTROL MSR(0x3A) is *not* set before
accessing SMRR base/mask MSRs, then ASSERT (!FeaturePcdGet (PcdSmrrEnable));


Thanks,
Jiaxin



> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, June 28, 2022 5:17 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable &
> SmmFeatureControl support
> 
> > -  //
> > -  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
> > -  //
> > -  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;
> 
> 1. can we keep the logic but just replace the above line as "ASSERT
> (FeaturePcdGet (PcdSmrrEnable));"?
> 
> >      if ((FeatureControl & BIT3) == 0) {
> > -      if ((FeatureControl & BIT0) == 0) {
> > +      if (((FeatureControl & BIT0) == 0) && (FeaturePcdGet
> (PcdSmrrEnable)))
> > {
> >          AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL,
> > FeatureControl | BIT3);
> >        } else {
> > -        mSmrrSupported = FALSE;
> > +        ASSERT (!FeaturePcdGet (PcdSmrrEnable));
> 
> 2. If PcdSmrrEnable is TRUE but the FeatureControl MSR is locked (BIT0 is set),
>   above assertion will be hit. We may need to reconsider the code logic.
> 
> > -    {
> > -      //
> > -      // 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;
> 
> 3. can we keep the logic but just replace the above line as "ASSERT
> (FeaturePcdGet (PcdSmmFeatureControlEnable))"?


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