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; Fri, 17 May 2019 05:14:11 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6DC477FDF9; Fri, 17 May 2019 12:13:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-94.rdu2.redhat.com [10.10.120.94]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7DE6A614E3; Fri, 17 May 2019 12:13:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Remove CPU generation check To: "Zeng, Star" , "Ni, Ray" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Kumar, Chandana C" , "Li, Kevin Y" References: <20190516103335.76628-1-star.zeng@intel.com> <7eeffb11-79ae-39b6-73a6-8c84434a1855@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB048310402E357B@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C145D7D@SHSMSX104.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB048310402E3AD1@shsmsx102.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 17 May 2019 14:13:48 +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: <0C09AFA07DD0434D9E2A0C6AEB048310402E3AD1@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 17 May 2019 12:13:59 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ; devel@edk2.groups.io; >> lersek@redhat.com >> Cc: Dong, Eric ; Kumar, Chandana C >> >> 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@redhat.com >>> Cc: Dong, Eric ; Ni, Ray ; >>> Kumar, Chandana C ; Zeng, Star >>> >>> 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 ; devel@edk2.groups.io >>>> Cc: Dong, Eric ; Ni, Ray ; >>>> Kumar, Chandana C >>>> 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 >>>>> 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. >>> >>> 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 >>>> >>>> >