Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Dynamic check SMRR enable & SmmFeatureControl capability

["Michael D Kinney" <michael.d.kinney@intel.com>]

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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin
> Sent: Sunday, July 17, 2022 1:38 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@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
> 
> 
> 
> 
>