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

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

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