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.z...@intel.com>; devel@edk2.groups.io; >> ler...@redhat.com >> Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Chandana C >> <chandana.c.ku...@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@edk2.groups.io; ler...@redhat.com >>> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; >>> Kumar, Chandana C <chandana.c.ku...@intel.com>; Zeng, Star >>> <star.z...@intel.com> >>> 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.z...@intel.com>; devel@edk2.groups.io >>>> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; >>>> Kumar, Chandana C <chandana.c.ku...@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 <ler...@redhat.com> >>>>> Cc: Eric Dong <eric.d...@intel.com> >>>>> Cc: Ruiyu Ni <ruiyu...@intel.com> >>>>> Cc: Chandana Kumar <chandana.c.ku...@intel.com> >>>>> Signed-off-by: Star Zeng <star.z...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
