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. 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 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 (#40787): https://edk2.groups.io/g/devel/message/40787 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] -=-=-=-=-=-=-=-=-=-=-=-
