Re: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Remove CPU generation check


Zeng, Star
 

Laszlo,

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Laszlo Ersek
Sent: Thursday, May 16, 2019 9:06 PM
To: Zeng, Star <star.zeng@...>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Kumar,
Chandana C <chandana.c.kumar@...>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib:
Remove CPU generation check

Hi Star,

On 05/16/19 12:33, Star Zeng wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1679

The checking to CpuInfo->CpuIdVersionInfoEcx.Bits.AESNI is enough, the
checking to CPU generation could be removed, then the code could be
reused by more platforms.

Cc: Laszlo Ersek <lersek@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ruiyu Ni <ruiyu.ni@...>
Cc: Chandana Kumar <chandana.c.kumar@...>
Signed-off-by: Star Zeng <star.zeng@...>
---
UefiCpuPkg/Library/CpuCommonFeaturesLib/Aesni.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/Aesni.c
b/UefiCpuPkg/Library/CpuCommonFeaturesLib/Aesni.c
index b79446ba3ca9..4a56eec1b267 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/Aesni.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/Aesni.c
@@ -57,15 +57,9 @@ AesniSupport (
MSR_SANDY_BRIDGE_FEATURE_CONFIG_REGISTER *MsrFeatureConfig;

if (CpuInfo->CpuIdVersionInfoEcx.Bits.AESNI == 1) {
- if (IS_SANDY_BRIDGE_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo-
DisplayModel) ||
- IS_SILVERMONT_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo-
DisplayModel) ||
- IS_XEON_5600_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo-
DisplayModel) ||
- IS_XEON_E7_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo-
DisplayModel) ||
- IS_XEON_PHI_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo-
DisplayModel)) {
- MsrFeatureConfig =
(MSR_SANDY_BRIDGE_FEATURE_CONFIG_REGISTER *) ConfigData;
- ASSERT (MsrFeatureConfig != NULL);
- MsrFeatureConfig[ProcessorNumber].Uint64 = AsmReadMsr64
(MSR_SANDY_BRIDGE_FEATURE_CONFIG);
- }
+ MsrFeatureConfig =
(MSR_SANDY_BRIDGE_FEATURE_CONFIG_REGISTER *) ConfigData;
+ ASSERT (MsrFeatureConfig != NULL);
+ MsrFeatureConfig[ProcessorNumber].Uint64 = AsmReadMsr64
+ (MSR_SANDY_BRIDGE_FEATURE_CONFIG);
return TRUE;
}
return FALSE;
the patch and the bugzilla ticket claim that the AESNI bit's presence in CPUID
guarantees that MSR 0x13C is available.
That is the case we met. The purpose of this patch is to make the code more usable.


I don't see what guarantees this. According to the latest Intel SDM Vol 4,
which I just downloaded (335592-069US, January 2019),
MSR_FEATURE_CONFIG is available on the following (DisplayFamily,
DisplayModel) pairs:

- 06_37H, 06_4AH, 06_4DH, 06_5AH, 06_5DH, 06_5CH, 06_7AH
- 06_25H, 06_2CH
- 06_2FH
- 06_2AH, 06_2DH
- 06_57H
Yes, right.

Let me show some examples for the generations not in the list above.

1. MSR 0x13C is available: our some internal generations are in this case.
Without the patch, code needs to use function level override method in a CpuSpecificFeaturesLib.
Status = RegisterCpuFeature (
"AESNI",
NULL, // Use core function
SpecificAesniSupport, // Override core function
NULL, // Use core function
CPU_FEATURE_AESNI,
CPU_FEATURE_END
);
With the patch, the function level override will be not needed. The benefit of this patch is here.

2. MSR 0x13C is not available: let's assume some other MSR will be available for the case.
Without or with the patch, codes both need to use function level override method in a CpuSpecificFeaturesLib.
Status = RegisterCpuFeature (
"AESNI",
NULL, // Use core function
SpecificAesniSupport, // Override core function
SpecificAesniInitialize, // Override core function
CPU_FEATURE_AESNI,
CPU_FEATURE_END
);


Thanks,
Star


Which seems to indicate that at least *the approach* of the original code --
i.e. the family/model checking -- is correct. (It's possible that the
family/model list has to be extended from time to time, of course.)

Anyway, I don't intend to block this patch; OVMF does not use
CpuCommonFeaturesLib, so this change cannot regress it. I will let other
UefiCpuPkg reviewers decide about this patch.

Thanks!
Laszlo

Join devel@edk2.groups.io to automatically receive all group messages.