From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E69CD202E53B2 for ; Tue, 12 Feb 2019 19:01:27 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6B3ECC0624CB; Wed, 13 Feb 2019 03:01:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-136.rdu2.redhat.com [10.10.120.136]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5320C101960B; Wed, 13 Feb 2019 03:01:26 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org References: <20190213020405.18800-1-eric.dong@intel.com> <20190213020405.18800-4-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <7831b95d-de56-2bb1-9d3f-99cbf596dcb8@redhat.com> Date: Wed, 13 Feb 2019 04:01:25 +0100 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: <20190213020405.18800-4-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 13 Feb 2019 03:01:27 +0000 (UTC) Subject: Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Feb 2019 03:01:28 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/13/19 03:04, Eric Dong wrote: > PcdCpuFeaturesSupport used to specify the platform policy about > what CPU features this platform supports. This value is decide by > platform owner, not by hardware. After this change, this PCD will > be used in IsCpuFeatureSupported function only. > > Now RegisterCpuFeaturesLib use this PCD as an template to Get the > pcd size. Update the code logic to replace it with > PcdCpuFeaturesSetting. > > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1375 > > Cc: Ray Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 66 +++++++--------------- > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 1 - > .../RegisterCpuFeaturesLib.c | 10 ++-- > 3 files changed, 24 insertions(+), 53 deletions(-) > > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > index 4ebd0025b4..762eaec277 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > @@ -74,27 +74,6 @@ GetSettingPcd ( > return SettingBitMask; > } > > -/** > - Worker function to get PcdCpuFeaturesSupport. > - > - @return The pointer to CPU feature bits mask buffer. > -**/ > -UINT8 * > -GetSupportPcd ( > - VOID > - ) > -{ > - UINT8 *SupportBitMask; > - > - SupportBitMask = AllocateCopyPool ( > - PcdGetSize (PcdCpuFeaturesSupport), > - PcdGetPtr (PcdCpuFeaturesSupport) > - ); > - ASSERT (SupportBitMask != NULL); > - > - return SupportBitMask; > -} > - > /** > Collects CPU type and feature information. > > @@ -282,11 +261,6 @@ CpuInitDataInitialize ( > ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL); > CpuFeaturesData->CpuFlags.PackageSemaphoreCount = AllocateZeroPool (sizeof (UINT32) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount); > ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL); > - > - // > - // Get support and configuration PCDs > - // > - CpuFeaturesData->SupportPcd = GetSupportPcd (); > } > > /** > @@ -306,7 +280,7 @@ SupportedMaskOr ( > UINT8 *Data1; > UINT8 *Data2; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > Data1 = SupportedFeatureMask; > Data2 = OrFeatureBitMask; > for (Index = 0; Index < BitMaskSize; Index++) { > @@ -331,7 +305,7 @@ SupportedMaskAnd ( > UINT8 *Data1; > UINT8 *Data2; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > Data1 = SupportedFeatureMask; > Data2 = AndFeatureBitMask; > for (Index = 0; Index < BitMaskSize; Index++) { > @@ -356,7 +330,7 @@ SupportedMaskCleanBit ( > UINT8 *Data1; > UINT8 *Data2; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > Data1 = SupportedFeatureMask; > Data2 = AndFeatureBitMask; > for (Index = 0; Index < BitMaskSize; Index++) { > @@ -387,7 +361,7 @@ IsBitMaskMatch ( > UINT8 *Data1; > UINT8 *Data2; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > > Data1 = SupportedFeatureMask; > Data2 = ComparedFeatureBitMask; > @@ -426,22 +400,22 @@ CollectProcessorData ( > Entry = GetFirstNode (&CpuFeaturesData->FeatureList); > while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) { > CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry); > - if (IsBitMaskMatch (CpuFeaturesData->SupportPcd, CpuFeature->FeatureMask)) { > - if (CpuFeature->SupportFunc == NULL) { > - // > - // If SupportFunc is NULL, then the feature is supported. > - // > - SupportedMaskOr ( > - CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask, > - CpuFeature->FeatureMask > - ); > - } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, CpuFeature->ConfigData)) { > - SupportedMaskOr ( > - CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask, > - CpuFeature->FeatureMask > - ); > - } > + > + if (CpuFeature->SupportFunc == NULL) { > + // > + // If SupportFunc is NULL, then the feature is supported. > + // > + SupportedMaskOr ( > + CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask, > + CpuFeature->FeatureMask > + ); > + } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, CpuFeature->ConfigData)) { > + SupportedMaskOr ( > + CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask, > + CpuFeature->FeatureMask > + ); > } > + > Entry = Entry->ForwardLink; > } > } The functional effect of the change in CollectProcessorData() is too complex for me to determine, so I'd like to leave the review of this patch to Ray only. (1) FWIW, I've attempted to read the BZ carefully, and it seems the patch doesn't do all things that the BZ suggests: - The IsCpuFeatureSupported() function is not removed. That could be correct, but then I think it should be explained in the BZ (i.e. update the scope). - Is it asserted somewhere that PcdCpuFeaturesSetting and PcdCpuFeaturesCapability have equal size? (2) Independently, please append (or prepend) a cleanup patch to the series. Namely, the comment block on IsCpuFeatureSetInCpuPcd() references "PcdCpuFeaturesSupport". It should reference "CpuBitMask" instead, IMO. Thanks Laszlo > @@ -646,8 +620,6 @@ AnalysisProcessorFeatures ( > DumpCpuFeature (CpuFeature); > Entry = Entry->ForwardLink; > } > - DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n")); > - DumpCpuFeatureMask (CpuFeaturesData->SupportPcd); > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n")); > DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd); > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n")); > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > index 3e0a342fd1..836ed3549c 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > @@ -81,7 +81,6 @@ typedef struct { > LIST_ENTRY FeatureList; > > CPU_FEATURES_INIT_ORDER *InitOrder; > - UINT8 *SupportPcd; > UINT8 *CapabilityPcd; > UINT8 *SettingPcd; > > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > index 3540029079..3e8e899766 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > @@ -31,7 +31,7 @@ IsCpuFeatureMatch ( > { > UINTN BitMaskSize; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) == 0) { > return TRUE; > } else { > @@ -53,7 +53,7 @@ DumpCpuFeatureMask ( > UINT8 *Data8; > UINTN BitMaskSize; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > Data8 = (UINT8 *) FeatureMask; > for (Index = 0; Index < BitMaskSize; Index++) { > DEBUG ((DEBUG_INFO, " %02x ", *Data8++)); > @@ -100,7 +100,7 @@ IsBitMaskMatchCheck ( > UINT8 *Data1; > UINT8 *Data2; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > > Data1 = FeatureMask; > Data2 = DependentBitMask; > @@ -656,7 +656,7 @@ RegisterCpuFeatureWorker ( > UINTN BitMaskSize; > BOOLEAN FeatureExist; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > CpuFeaturesData = GetCpuFeaturesData (); > if (CpuFeaturesData->FeaturesCount == 0) { > InitializeListHead (&CpuFeaturesData->FeatureList); > @@ -870,7 +870,7 @@ RegisterCpuFeature ( > BeforeAll = FALSE; > AfterAll = FALSE; > > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport); > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting); > > VA_START (Marker, InitializeFunc); > Feature = VA_ARG (Marker, UINT32); >