From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 16 May 2019 06:06:32 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0569B3001A62; Thu, 16 May 2019 13:06:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-88.rdu2.redhat.com [10.10.121.88]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9C66E5D97C; Thu, 16 May 2019 13:06:28 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Remove CPU generation check To: Star Zeng , devel@edk2.groups.io Cc: Eric Dong , Ruiyu Ni , Chandana Kumar References: <20190516103335.76628-1-star.zeng@intel.com> From: "Laszlo Ersek" Message-ID: <7eeffb11-79ae-39b6-73a6-8c84434a1855@redhat.com> Date: Thu, 16 May 2019 15:06:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190516103335.76628-1-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Thu, 16 May 2019 13:06:31 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > Cc: Ruiyu Ni > Cc: Chandana Kumar > Signed-off-by: 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. 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