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


Zeng, Star
 

Situation: All the generations (including the internal generations not listed in SDM) we saw have MSR 13Ch available when CpuInfo->CpuIdVersionInfoEcx.Bits.AESNI == 1.

Requirement: Reuse more code.

Could you help think the good method and even propose the patch for that? I am ok to any method to improve the code's reusability.
Otherwise, we can only use function level override method ina CpuSpecificFeaturesLib.
Status = RegisterCpuFeature (
"AESNI",
NULL, // Use core function
SpecificAesniSupport, // Override core function
NULL, // Use core function
CPU_FEATURE_AESNI,
CPU_FEATURE_END
);

Thanks,
Star

-----Original Message-----
From: Ni, Ray
Sent: Friday, May 17, 2019 9:04 AM
To: Zeng, Star <star.zeng@...>; devel@edk2.groups.io;
lersek@...
Cc: Dong, Eric <eric.dong@...>; Kumar, Chandana C
<chandana.c.kumar@...>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib:
Remove CPU generation check

Star,
I think the discussion is about providing the evidence to support removing
the generation check.
Not just the benefit of that.

Thanks,
Ray

-----Original Message-----
From: Zeng, Star
Sent: Thursday, May 16, 2019 10:52 PM
To: devel@edk2.groups.io; lersek@...
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>;
Kumar, Chandana C <chandana.c.kumar@...>; Zeng, Star
<star.zeng@...>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib:
Remove CPU generation check

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.