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 <[email protected]> > Sent: Tuesday, June 28, 2022 5:17 PM > To: Wu, Jiaxin <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]> > 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/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
