[edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Remove CPU generation check
Laszlo Ersek
lersek at redhat.com
Fri May 17 12:13:48 UTC 2019
Hi Star,
On 05/17/19 05:05, Zeng, Star wrote:
> Situation: All the generations (including the internal generations not listed in SDM) we saw have MSR 13Ch available when CpuInfo->CpuIdVersionInfoEcx.Bits.AESNI == 1.
This is a very accurate description, thank you; and it's exactly what I
feel is insufficient. "All generations we've seen" is not equal to "all
generations that (a) have ever existed plus (b) will ever exist".
Anyway, I now understand the motivation behind the patch, thanks. Given
that OVMF cannot be regressed by it, I don't intend to block it -- I
defer to other UefiCpuPkg reviewers. If they are OK with the patch, so am I.
Thanks!
Laszlo
>
> 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 at intel.com>; devel at edk2.groups.io;
>> lersek at redhat.com
>> Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Chandana C
>> <chandana.c.kumar at intel.com>
>> 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 at edk2.groups.io; lersek at redhat.com
>>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
>>> Kumar, Chandana C <chandana.c.kumar at intel.com>; Zeng, Star
>>> <star.zeng at intel.com>
>>> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib:
>>> Remove CPU generation check
>>>
>>> Laszlo,
>>>
>>>> -----Original Message-----
>>>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf
>>>> Of Laszlo Ersek
>>>> Sent: Thursday, May 16, 2019 9:06 PM
>>>> To: Zeng, Star <star.zeng at intel.com>; devel at edk2.groups.io
>>>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
>>>> Kumar, Chandana C <chandana.c.kumar at intel.com>
>>>> 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 at redhat.com>
>>>>> Cc: Eric Dong <eric.dong at intel.com>
>>>>> Cc: Ruiyu Ni <ruiyu.ni at intel.com>
>>>>> Cc: Chandana Kumar <chandana.c.kumar at intel.com>
>>>>> Signed-off-by: Star Zeng <star.zeng at intel.com>
>>>>> ---
>>>>> 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
>>>>
>>>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40913): https://edk2.groups.io/g/devel/message/40913
Mute This Topic: https://groups.io/mt/31639184/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list