[edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability
Wu, Jiaxin
jiaxin.wu at intel.com
Mon Jul 18 07:32:22 UTC 2022
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 at intel.com>
> Sent: Monday, July 18, 2022 8:13 AM
> To: devel at edk2.groups.io; Wu, Jiaxin <jiaxin.wu at intel.com>; Kinney, Michael
> D <michael.d.kinney at intel.com>
> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> > Sent: Sunday, July 17, 2022 1:38 AM
> > To: devel at edk2.groups.io
> > Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at 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 at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Signed-off-by: Jiaxin Wu <jiaxin.wu at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list