Re: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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))"?