* [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support
@ 2022-06-28 8:47 Wu, Jiaxin
2022-06-28 9:16 ` Ni, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Jiaxin @ 2022-06-28 8:47 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni
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>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++
.../SmmCpuFeaturesLib/SmmCpuFeaturesLibCommon.c | 84 ++++------------------
.../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 4 ++
.../StandaloneMmCpuFeaturesLib.inf | 4 ++
UefiCpuPkg/UefiCpuPkg.dec | 12 ++++
UefiCpuPkg/UefiCpuPkg.uni | 12 ++++
6 files changed, 48 insertions(+), 72 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..b88cdece2a 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;
@@ -81,39 +71,27 @@ CpuFeaturesLibInitialization (
UINTN ModelId;
//
// Retrieve CPU Family and Model
//
- AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, &RegEdx);
+ AsmCpuid (CPUID_VERSION_INFO, &RegEax, NULL, NULL, NULL);
FamilyId = (RegEax >> 8) & 0xf;
ModelId = (RegEax >> 4) & 0xf;
if ((FamilyId == 0x06) || (FamilyId == 0x0f)) {
ModelId = ModelId | ((RegEax >> 12) & 0xf0);
}
- //
- // 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;
- }
- }
-
//
// Intel(R) 64 and IA-32 Architectures Software Developer's Manual
// Volume 3C, Section 35.3 MSRs in the Intel(R) Atom(TM) Processor Family
//
// 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
@@ -194,14 +172,10 @@ SmmCpuFeaturesInitializeProcessor (
IN CPU_HOT_PLUG_DATA *CpuHotPlugData
)
{
SMRAM_SAVE_STATE_MAP *CpuState;
UINT64 FeatureControl;
- UINT32 RegEax;
- UINT32 RegEdx;
- UINTN FamilyId;
- UINTN ModelId;
//
// Configure SMBASE.
//
CpuState = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
@@ -214,17 +188,17 @@ 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 (mSmrrPhysBaseMsr == SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE) {
FeatureControl = AsmReadMsr64 (SMM_FEATURES_LIB_IA32_FEATURE_CONTROL);
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));
}
}
}
//
@@ -232,11 +206,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
//
@@ -256,44 +230,10 @@ SmmCpuFeaturesInitializeProcessor (
AsmWriteMsr64 (mSmrrPhysMaskMsr, (~(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,11 +321,11 @@ VOID
EFIAPI
SmmCpuFeaturesDisableSmrr (
VOID
)
{
- if (mSmrrSupported && mNeedConfigureMtrrs) {
+ if (FeaturePcdGet (PcdSmrrEnable) && mNeedConfigureMtrrs) {
AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) & ~EFI_MSR_SMRR_PHYS_MASK_VALID);
}
}
/**
@@ -396,11 +336,11 @@ VOID
EFIAPI
SmmCpuFeaturesReenableSmrr (
VOID
)
{
- if (mSmrrSupported && mNeedConfigureMtrrs) {
+ if (FeaturePcdGet (PcdSmrrEnable) && mNeedConfigureMtrrs) {
AsmWriteMsr64 (mSmrrPhysMaskMsr, AsmReadMsr64 (mSmrrPhysMaskMsr) | EFI_MSR_SMRR_PHYS_MASK_VALID);
}
}
/**
@@ -417,11 +357,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 +398,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 +424,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 +450,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.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index ec97041d8b..3eacab48db 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -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|BOOLEAN|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|BOOLEAN|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_HELP #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_PROMPT #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 related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support
2022-06-28 8:47 [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support Wu, Jiaxin
@ 2022-06-28 9:16 ` Ni, Ray
2022-06-29 1:37 ` Wu, Jiaxin
2022-07-17 8:46 ` Wu, Jiaxin
0 siblings, 2 replies; 5+ messages in thread
From: Ni, Ray @ 2022-06-28 9:16 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io; +Cc: Dong, Eric
> - //
> - // 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))"?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support
2022-06-28 9:16 ` Ni, Ray
@ 2022-06-29 1:37 ` Wu, Jiaxin
2022-07-17 8:46 ` Wu, Jiaxin
1 sibling, 0 replies; 5+ messages in thread
From: Wu, Jiaxin @ 2022-06-29 1:37 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Eric
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))"?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support
2022-06-28 9:16 ` Ni, Ray
2022-06-29 1:37 ` Wu, Jiaxin
@ 2022-07-17 8:46 ` Wu, Jiaxin
1 sibling, 0 replies; 5+ messages in thread
From: Wu, Jiaxin @ 2022-07-17 8:46 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Eric
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))"?
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support
@ 2022-07-29 6:25 Wu, Jiaxin
0 siblings, 0 replies; 5+ messages in thread
From: Wu, Jiaxin @ 2022-07-29 6:25 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Star Zeng, Michael D Kinney
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.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index ec97041d8b..3eacab48db 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -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|BOOLEAN|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|BOOLEAN|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_HELP #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_PROMPT #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 related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-29 6:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-28 8:47 [PATCH v1] UefiCpuPkg: Add PCD to control SMRR enable & SmmFeatureControl support Wu, Jiaxin
2022-06-28 9:16 ` Ni, Ray
2022-06-29 1:37 ` Wu, Jiaxin
2022-07-17 8:46 ` Wu, Jiaxin
-- strict thread matches above, loose matches on Subject: below --
2022-07-29 6:25 Wu, Jiaxin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox