* Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support
[not found] ` <MN0PR11MB6158D980B948F25A46AF2ABEFE9C9@MN0PR11MB6158.namprd11.prod.outlook.com>
@ 2022-08-23 2:22 ` Ni, Ray
0 siblings, 0 replies; only message in thread
From: Ni, Ray @ 2022-08-23 2:22 UTC (permalink / raw)
To: Wu, Jiaxin; +Cc: devel@edk2.groups.io
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> Sent: Friday, July 29, 2022 2:26 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [edk2-devel] [PATCH v1] UefiCpuPkg: Add PCD to control SMRR
> enable & SmmFeatureControl support
>
> 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, add PCD to control SMRR & SmmFeatureControl
> enable.
>
> Change-Id: I6835e4b0e12c5e6f52effb60fd9224e3eb97fc0d
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +++
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c | 35 ++++++++----
> ----------
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 4 +++
> .../StandaloneMmCpuFeaturesLib.inf | 4 +++
> UefiCpuPkg/UefiCpuPkg.dec | 12 ++++++++
> UefiCpuPkg/UefiCpuPkg.uni | 12 ++++++++
> 6 files changed, 48 insertions(+), 23 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index 35292dac31..7b5cef9700 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -33,5 +33,9 @@
> MemoryAllocationLib
> DebugLib
>
> [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> SOMETIMES_CONSUMES
> +
> +[FeaturePcd]
> + gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable ## CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable ##
> CONSUMES
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> index 78de7f8407..75a0ec8e94 100644
> ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c
> @@ -35,20 +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;
> @@ -96,11 +86,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;
> + ASSERT (FeaturePcdGet (PcdSmrrEnable));
> }
> }
>
> //
> // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> @@ -109,11 +99,11 @@ 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;
> + ASSERT (!FeaturePcdGet (PcdSmrrEnable));
> }
> }
>
> //
> // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> @@ -214,17 +204,16 @@ SmmCpuFeaturesInitializeProcessor (
> // 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)) {
> + if ((FeaturePcdGet (PcdSmrrEnable)) && (mSmrrPhysBaseMsr ==
> SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE)) {
> FeatureControl = AsmReadMsr64
> (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
> if ((FeatureControl & BIT3) == 0) {
> + ASSERT ((FeatureControl & BIT0) == 0);
> if ((FeatureControl & BIT0) == 0) {
> AsmWriteMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL,
> FeatureControl | BIT3);
> - } else {
> - mSmrrSupported = FALSE;
> }
> }
> }
>
> //
> @@ -232,11 +221,11 @@ SmmCpuFeaturesInitializeProcessor (
> // 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 (FeaturePcdGet (PcdSmrrEnable)) {
> //
> // 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
> //
> @@ -285,11 +274,11 @@ SmmCpuFeaturesInitializeProcessor (
> //
> // 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;
> + ASSERT (FeaturePcdGet (PcdSmmFeatureControlEnable));
> }
> }
> }
>
> //
> @@ -381,11 +370,11 @@ VOID
> EFIAPI
> SmmCpuFeaturesDisableSmrr (
> VOID
> )
> {
> - if (mSmrrSupported && mNeedConfigureMtrrs) {
> + if (FeaturePcdGet (PcdSmrrEnable) && mNeedConfigureMtrrs) {
> AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
> }
> }
>
> /**
> @@ -396,11 +385,11 @@ VOID
> EFIAPI
> SmmCpuFeaturesReenableSmrr (
> VOID
> )
> {
> - if (mSmrrSupported && mNeedConfigureMtrrs) {
> + if (FeaturePcdGet (PcdSmrrEnable) && mNeedConfigureMtrrs) {
> AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> }
> }
>
> /**
> @@ -417,11 +406,11 @@ SmmCpuFeaturesRendezvousEntry (
> )
> {
> //
> // If SMRR is supported and this is the first normal SMI, then enable SMRR
> //
> - if (mSmrrSupported && !mSmrrEnabled[CpuIndex]) {
> + if (FeaturePcdGet (PcdSmrrEnable) && !mSmrrEnabled[CpuIndex]) {
> AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64
> (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
> mSmrrEnabled[CpuIndex] = TRUE;
> }
> }
>
> @@ -458,11 +447,11 @@ EFIAPI
> SmmCpuFeaturesIsSmmRegisterSupported (
> IN UINTN CpuIndex,
> IN SMM_REG_NAME RegName
> )
> {
> - if (mSmmFeatureControlSupported && (RegName ==
> SmmRegFeatureControl)) {
> + if (FeaturePcdGet (PcdSmmFeatureControlEnable) && (RegName ==
> SmmRegFeatureControl)) {
> return TRUE;
> }
>
> return FALSE;
> }
> @@ -484,11 +473,11 @@ EFIAPI
> SmmCpuFeaturesGetSmmRegister (
> IN UINTN CpuIndex,
> IN SMM_REG_NAME RegName
> )
> {
> - if (mSmmFeatureControlSupported && (RegName ==
> SmmRegFeatureControl)) {
> + if (FeaturePcdGet (PcdSmmFeatureControlEnable) && (RegName ==
> SmmRegFeatureControl)) {
> return AsmReadMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL);
> }
>
> return 0;
> }
> @@ -510,11 +499,11 @@ SmmCpuFeaturesSetSmmRegister (
> IN UINTN CpuIndex,
> IN SMM_REG_NAME RegName,
> IN UINT64 Value
> )
> {
> - if (mSmmFeatureControlSupported && (RegName ==
> SmmRegFeatureControl)) {
> + if (FeaturePcdGet (PcdSmmFeatureControlEnable) && (RegName ==
> SmmRegFeatureControl)) {
> AsmWriteMsr64 (SMM_FEATURES_LIB_SMM_FEATURE_CONTROL, Value);
> }
> }
>
> /**
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> index 022351f593..85214ee31c 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> @@ -68,7 +68,11 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ##
> CONSUMES
>
> +[FeaturePcd]
> + gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable ## CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable ##
> CONSUMES
> +
> [Depex]
> gEfiMpServiceProtocolGuid
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> index ec97041d8b..3eacab48db 100644
> ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> @@ -34,5 +34,9 @@
> MemoryAllocationLib
> PcdLib
>
> [FixedPcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> SOMETIMES_CONSUMES
> +
> +[FeaturePcd]
> + gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable ## CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable ##
> CONSUMES
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 1951eb294c..55cbe7605f 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -153,10 +153,22 @@
> # TRUE - SMM Feature Control MSR will be locked.<BR>
> # FALSE - SMM Feature Control MSR will not be locked.<BR>
> # @Prompt Lock SMM Feature Control MSR.
>
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|B
> OOLEAN|0x3213210B
>
> + ## Indicates if SMRR will be enabled.<BR><BR>
> + # TRUE - SMRR will be enabled.<BR>
> + # FALSE - SMRR will not be enabled.<BR>
> + # @Prompt Enable SMRR.
> +
> gUefiCpuPkgTokenSpaceGuid.PcdSmrrEnable|TRUE|BOOLEAN|0x3213210D
> +
> + ## Indicates if SmmFeatureControl will be enabled.<BR><BR>
> + # TRUE - SmmFeatureControl will be enabled.<BR>
> + # FALSE - SmmFeatureControl will not be enabled.<BR>
> + # @Prompt Support SmmFeatureControl.
> +
> gUefiCpuPkgTokenSpaceGuid.PcdSmmFeatureControlEnable|TRUE|BOOLEA
> N|0x32132110
> +
> [PcdsFixedAtBuild]
> ## List of exception vectors which need switching stack.
> # This PCD will only take into effect if PcdCpuStackGuard is enabled.
> # By default exception #DD(8), #PF(14) are supported.
> # @Prompt Specify exception vectors which need switching stack.
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index 219c1963bf..d17bcfd10c 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -98,10 +98,22 @@
>
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HE
> LP #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
> "TRUE - locked.<BR>\n"
> "FALSE - unlocked.<BR>"
>
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdSmrrEnable_PROMPT
> #language en-US "Indicates if SMRR will be enabled."
> +
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdSmrrEnable_HELP
> #language en-US "Indicates if SMRR will be enabled.<BR><BR>\n"
> + "TRUE - SMRR will be
> enabled.<BR>\n"
> + "FALSE - SMRR will not be
> enabled.<BR>"
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdSmmFeatureControlEnable_PROMPT
> #language en-US "Indicates if SmmFeatureControl will be enabled."
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdSmmFeatureControlEnable_HELP
> #language en-US "Indicates if SmmFeatureControl will be
> enabled.<BR><BR>\n"
> + "TRUE - SmmFeatureControl
> will be enabled.<BR>\n"
> + "FALSE - SmmFeatureControl
> will not be enabled.<BR>"
> +
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdPeiTemporaryRamStackSize_PROMP
> T #language en-US "Stack size in the temporary RAM"
>
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdPeiTemporaryRamStackSize_HELP
> #language en-US "Specifies stack size in the temporary RAM. 0 means half of
> TemporaryRamSize."
>
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmProfileSize_PROMPT
> #language en-US "SMM profile data buffer size"
> --
> 2.16.2.windows.1
>
>
>
>
>
^ permalink raw reply [flat|nested] only message in thread