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


Zeng, Star
 

Thanks for the understanding to all of you.


Star

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

Star,
I understand the motivation of the change.

Given your statement that all processors you met follows the rule, and I
know that you are currently working very actively on Intel processors,
Reviewed-by: Ray Ni <ray.ni@...>

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

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.