From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.93; helo=mga11.intel.com; envelope-from=ray.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 AAD34201B0403 for ; Thu, 14 Feb 2019 00:47:47 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Feb 2019 00:47:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,368,1544515200"; d="scan'208";a="143370544" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 14 Feb 2019 00:47:46 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 14 Feb 2019 00:47:46 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 14 Feb 2019 00:47:46 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.102]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.36]) with mapi id 14.03.0415.000; Thu, 14 Feb 2019 16:47:44 +0800 From: "Ni, Ray" To: "Dong, Eric" , "edk2-devel@lists.01.org" CC: Laszlo Ersek Thread-Topic: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration. Thread-Index: AQHUw0Bxe/ywLtImCkW2CW6YkB8ey6Xe+ekQ Date: Thu, 14 Feb 2019 08:47:44 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C01997F@SHSMSX104.ccr.corp.intel.com> References: <20190213020405.18800-1-eric.dong@intel.com> <20190213020405.18800-3-eric.dong@intel.com> In-Reply-To: <20190213020405.18800-3-eric.dong@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration. 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: Thu, 14 Feb 2019 08:47:47 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, February 13, 2019 10:04 AM > To: edk2-devel@lists.01.org > Cc: Ni, Ray ; Laszlo Ersek > Subject: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD > PcdCpuFeaturesUserConfiguration. >=20 > In current implementation, PCD PcdCpuFeaturesUserConfiguration used as > user enabled CPU features list. It is initialzied in platform driver and = as an > input for CpuFeatures driver. PCD PcdCpuFeaturesSetting used as an output > for the final enabled CPU features list. For now, > PcdCpuFeaturesUserConfiguration is only used as an input and > PcdCpuFeaturesSetting only used as an output. >=20 > This change merge PcdCpuFeaturesUserConfiguration into > PcdCpuFeaturesSetting. > Use PcdCpuFeaturesSetting as input for the user input feature setting Use > PcdCpuFeaturesSetting as output for the final CPU feature setting >=20 > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1375 >=20 > Cc: Ray Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 37 ++++++++++++----= -- > ---- > .../DxeRegisterCpuFeaturesLib.inf | 1 - > .../PeiRegisterCpuFeaturesLib.inf | 1 - > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 1 - > UefiCpuPkg/UefiCpuPkg.dec | 7 ++-- > 5 files changed, 22 insertions(+), 25 deletions(-) >=20 > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > index bae92b89a6..4ebd0025b4 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > @@ -54,41 +54,41 @@ SetSettingPcd ( > } >=20 > /** > - Worker function to get PcdCpuFeaturesSupport. > + Worker function to get PcdCpuFeaturesSetting. >=20 > @return The pointer to CPU feature bits mask buffer. > **/ > UINT8 * > -GetSupportPcd ( > +GetSettingPcd ( > VOID > ) > { > - UINT8 *SupportBitMask; > + UINT8 *SettingBitMask; >=20 > - SupportBitMask =3D AllocateCopyPool ( > - PcdGetSize (PcdCpuFeaturesSupport), > - PcdGetPtr (PcdCpuFeaturesSupport) > + SettingBitMask =3D AllocateCopyPool ( > + PcdGetSize (PcdCpuFeaturesSetting), > + PcdGetPtr (PcdCpuFeaturesSetting) > ); > - ASSERT (SupportBitMask !=3D NULL); > + ASSERT (SettingBitMask !=3D NULL); >=20 > - return SupportBitMask; > + return SettingBitMask; > } >=20 > /** > - Worker function to get PcdCpuFeaturesUserConfiguration. > + Worker function to get PcdCpuFeaturesSupport. >=20 > @return The pointer to CPU feature bits mask buffer. > **/ > UINT8 * > -GetConfigurationPcd ( > +GetSupportPcd ( > VOID > ) > { > UINT8 *SupportBitMask; >=20 > SupportBitMask =3D AllocateCopyPool ( > - PcdGetSize (PcdCpuFeaturesUserConfiguration), > - PcdGetPtr (PcdCpuFeaturesUserConfiguration) > + PcdGetSize (PcdCpuFeaturesSupport), > + PcdGetPtr (PcdCpuFeaturesSupport) > ); > ASSERT (SupportBitMask !=3D NULL); >=20 > @@ -287,7 +287,6 @@ CpuInitDataInitialize ( > // Get support and configuration PCDs > // > CpuFeaturesData->SupportPcd =3D GetSupportPcd (); > - CpuFeaturesData->ConfigurationPcd =3D GetConfigurationPcd (); } >=20 > /** > @@ -595,6 +594,9 @@ AnalysisProcessorFeatures ( > CPU_FEATURE_DEPENDENCE_TYPE AfterDep; > CPU_FEATURE_DEPENDENCE_TYPE NoneNeibBeforeDep; > CPU_FEATURE_DEPENDENCE_TYPE NoneNeibAfterDep; > + UINT8 *ConfigurationPcd; 1. Can we use the name "SettingPcd"? To use the same term. > + > + ConfigurationPcd =3D NULL; >=20 > CpuFeaturesData =3D GetCpuFeaturesData (); > CpuFeaturesData->CapabilityPcd =3D AllocatePool (CpuFeaturesData- > >BitMaskSize); @@ -610,10 +612,13 @@ AnalysisProcessorFeatures ( > // > // Calculate the last setting > // > - > CpuFeaturesData->SettingPcd =3D AllocateCopyPool (CpuFeaturesData- > >BitMaskSize, CpuFeaturesData->CapabilityPcd); > ASSERT (CpuFeaturesData->SettingPcd !=3D NULL); > - SupportedMaskAnd (CpuFeaturesData->SettingPcd, CpuFeaturesData- > >ConfigurationPcd); > + ConfigurationPcd =3D GetSettingPcd (); 2. SuportedMaskAnd() function doesn't change the parameter 2. So how about we pass PcdGetPtr (PcdCpuFeaturesSetting) directly to SupportedMaskAnd()? > + SupportedMaskAnd (CpuFeaturesData->SettingPcd, ConfigurationPcd); if > + (ConfigurationPcd !=3D NULL) { > + FreePool (ConfigurationPcd); > + } >=20 > // > // Save PCDs and display CPU PCDs > @@ -643,8 +648,6 @@ AnalysisProcessorFeatures ( > } > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n")); > DumpCpuFeatureMask (CpuFeaturesData->SupportPcd); > - DEBUG ((DEBUG_INFO, "PcdCpuFeaturesUserConfiguration:\n")); > - DumpCpuFeatureMask (CpuFeaturesData->ConfigurationPcd); 3. The debug message cannot be just deleted. We still need to dump the original PcdCpuFeaturesSetting. > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n")); > DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd); > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n")); diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.i > nf > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.i > nf > index 362e0c6cd1..b7dc70808f 100644 > --- > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.i > nf > +++ > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLi > +++ b.inf > @@ -56,7 +56,6 @@ > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress #= # > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport #= # > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration #= # > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability #= # > PRODUCES > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting #= # > PRODUCES 4. For PcdCpuFeaturesSetting, "CONSUMES" should be mentioned in the comment= s. >=20 > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.in > f > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.in > f > index f3907e25d3..cd69721a2d 100644 > --- > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.in > f > +++ > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLi > +++ b.inf > @@ -57,7 +57,6 @@ > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress #= # > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport #= # > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration #= # > CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability #= # > PRODUCES > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting #= # > PRODUCES 5. Similar as #4. >=20 > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > index 21dd5773a6..3e0a342fd1 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > @@ -83,7 +83,6 @@ typedef struct { > CPU_FEATURES_INIT_ORDER *InitOrder; > UINT8 *SupportPcd; > UINT8 *CapabilityPcd; > - UINT8 *ConfigurationPcd; > UINT8 *SettingPcd; >=20 > UINT32 NumberOfCpus; > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index cb05fa2660..793490872f 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -261,10 +261,6 @@ > # @Prompt SMM CPU Synchronization Method. >=20 > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000 > 014 >=20 > - ## Specifies user's desired settings for enabling/disabling processor > features. > - # @Prompt User settings for enabling/disabling processor features. > - gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration|{0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000017 > - > ## Specifies the On-demand clock modulation duty cycle when ACPI > feature is enabled. > # @Prompt The encoded values for target duty cycle modulation. > # @ValidRange 0x80000001 | 0 - 15 > @@ -292,7 +288,8 @@ > # @ValidList 0x80000001 | 0 > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability|{0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000018 >=20 > - ## Specifies actual settings for processor features, each bit correspo= nding > to a specific feature. > + ## As input, specifies user's desired settings for enabling/disabling > processor features. > + ## As output, specifies actual settings for processor features, each b= it > corresponding to a specific feature. > # @Prompt Actual processor feature settings. > # @ValidList 0x80000001 | 0 > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019 > -- > 2.15.0.windows.1