From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: ray.ni@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Mon, 27 May 2019 22:52:11 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 May 2019 22:52:10 -0700 X-ExtLoop1: 1 Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 27 May 2019 22:52:10 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 27 May 2019 22:52:03 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 27 May 2019 22:52:03 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.33]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.70]) with mapi id 14.03.0415.000; Tue, 28 May 2019 13:52:01 +0800 From: "Ni, Ray" To: "Zeng, Star" , "devel@edk2.groups.io" CC: Laszlo Ersek , "Dong, Eric" , "Kumar, Chandana C" , "Li, Kevin Y" Subject: Re: [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Thread-Topic: [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Thread-Index: AQHVD8D/si/xIMR/TkSW5ESOau3946aAEzmQ Date: Tue, 28 May 2019 05:51:20 +0000 Deferred-Delivery: Tue, 28 May 2019 05:52:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C169BC7@SHSMSX104.ccr.corp.intel.com> References: <20190521103552.90492-1-star.zeng@intel.com> In-Reply-To: <20190521103552.90492-1-star.zeng@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 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ray Ni > -----Original Message----- > From: Zeng, Star > Sent: Tuesday, May 21, 2019 6:36 PM > To: devel@edk2.groups.io > Cc: Zeng, Star ; Laszlo Ersek ; > Dong, Eric ; Ni, Ray ; Kumar, > Chandana C ; Li, Kevin Y > > Subject: [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set > MSR_IA32_CLOCK_MODULATION >=20 > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1810 >=20 > This patch covers two problems. >=20 > 1. Current code gets CPUID_THERMAL_POWER_MANAGEMENT in > ClockModulationInitialize() and uses its ECMD bit for all processors. > But ClockModulationInitialize() is only executed by BSP, that means the b= it is > just for BSP. > It may have no functionality issue as all processors may have same bit va= lue > in a great possibility. But for good practice, the code should get > CPUID_THERMAL_POWER_MANAGEMENT in ClockModulationSupport > (executed by all processors), and then use them in > ClockModulationInitialize() for all processors. > We can see that Aesni.c (and others) have used this good practice. >=20 > 2. Current code uses 3 CPU_REGISTER_TABLE_WRITE_FIELD for > MSR_IA32_CLOCK_MODULATION in ClockModulationInitialize(), they can be > reduced to 1 CPU_REGISTER_TABLE_WRITE64 by getting > MSR_IA32_CLOCK_MODULATION for all processors in > ClockModulationSupport() and then update fields for register table write = in > ClockModulationInitialize(). >=20 > We may argue that there may be more times of > MSR_IA32_CLOCK_MODULATION getting. But actually the times of > MSR_IA32_CLOCK_MODULATION getting could be also reduced. >=20 > The reason is in ProgramProcessorRegister() of CpuFeaturesInitialize.c, > AsmMsrBitFieldWrite64 (AsmReadMsr64 + AsmWriteMsr64) will be used for > CPU_REGISTER_TABLE_WRITE_FIELD, and AsmWriteMsr64 will be used for > CPU_REGISTER_TABLE_WRITE64. >=20 > The times of MSR accessing could be reduced with this patch. > Without the patch: > 3 CPU_REGISTER_TABLE_WRITE_FIELD (in ClockModulationInitialize) > =3D=3D> 3 AsmMsrBitFieldWrite64 > =3D=3D> 3 AsmReadMsr64 + 3 AsmWriteMsr64 >=20 > With the patch: > 1 AsmReadMsr64 (in ClockModulationSupport) + > 1 CPU_REGISTER_TABLE_WRITE64 (in ClockModulationInitialize) > =3D=3D> 1 AsmWriteMsr64 >=20 > Cc: Laszlo Ersek > Cc: Eric Dong > Cc: Ruiyu Ni > Cc: Chandana Kumar > Cc: Kevin Li > Signed-off-by: Star Zeng > --- > .../CpuCommonFeaturesLib/ClockModulation.c | 87 +++++++++++++------ > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 ++++ > .../CpuCommonFeaturesLib.c | 2 +- > 3 files changed, 78 insertions(+), 26 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ClockModulation.c > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ClockModulation.c > index 614768587501..b1c6bf6148f3 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ClockModulation.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ClockModulation.c > @@ -1,13 +1,40 @@ > /** @file > Clock Modulation feature. >=20 > - Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2017 - 2019, Intel Corporation. All rights > + reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ >=20 > #include "CpuCommonFeatures.h" >=20 > +typedef struct { > + CPUID_THERMAL_POWER_MANAGEMENT_EAX > ThermalPowerManagementEax; > + MSR_IA32_CLOCK_MODULATION_REGISTER ClockModulation; } > +CLOCK_MODULATION_CONFIG_DATA; > + > +/** > + Prepares for the data used by CPU feature detection and initialization= . > + > + @param[in] NumberOfProcessors The number of CPUs in the platform. > + > + @return Pointer to a buffer of CPU related configuration data. > + > + @note This service could be called by BSP only. > +**/ > +VOID * > +EFIAPI > +ClockModulationGetConfigData ( > + IN UINTN NumberOfProcessors > + ) > +{ > + UINT32 *ConfigData; > + > + ConfigData =3D AllocateZeroPool (sizeof > (CLOCK_MODULATION_CONFIG_DATA) > +* NumberOfProcessors); > + ASSERT (ConfigData !=3D NULL); > + return ConfigData; > +} > + > /** > Detects if Clock Modulation feature supported on current processor. >=20 > @@ -32,7 +59,22 @@ ClockModulationSupport ( > IN VOID *ConfigData OPTIONAL > ) > { > - return (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI =3D=3D 1); > + CLOCK_MODULATION_CONFIG_DATA *CmConfigData; > + > + if (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI =3D=3D 1) { > + CmConfigData =3D (CLOCK_MODULATION_CONFIG_DATA *) ConfigData; > + ASSERT (CmConfigData !=3D NULL); > + AsmCpuid ( > + CPUID_THERMAL_POWER_MANAGEMENT, > + > &CmConfigData[ProcessorNumber].ThermalPowerManagementEax.Uint32, > + NULL, > + NULL, > + NULL > + ); > + CmConfigData[ProcessorNumber].ClockModulation.Uint64 =3D > AsmReadMsr64 (MSR_IA32_CLOCK_MODULATION); > + return TRUE; > + } > + return FALSE; > } >=20 > /** > @@ -61,34 +103,29 @@ ClockModulationInitialize ( > IN BOOLEAN State > ) > { > - CPUID_THERMAL_POWER_MANAGEMENT_EAX > ThermalPowerManagementEax; > - AsmCpuid (CPUID_THERMAL_POWER_MANAGEMENT, > &ThermalPowerManagementEax.Uint32, NULL, NULL, NULL); > + CLOCK_MODULATION_CONFIG_DATA *CmConfigData; > + MSR_IA32_CLOCK_MODULATION_REGISTER *ClockModulation; >=20 > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_CLOCK_MODULATION, > - MSR_IA32_CLOCK_MODULATION_REGISTER, > - Bits.OnDemandClockModulationDutyCycle, > - PcdGet8 (PcdCpuClockModulationDutyCycle) >> 1 > - ); > - if (ThermalPowerManagementEax.Bits.ECMD =3D=3D 1) { > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_CLOCK_MODULATION, > - MSR_IA32_CLOCK_MODULATION_REGISTER, > - Bits.ExtendedOnDemandClockModulationDutyCycle, > - PcdGet8 (PcdCpuClockModulationDutyCycle) & BIT0 > - ); > + CmConfigData =3D (CLOCK_MODULATION_CONFIG_DATA *) ConfigData; > ASSERT > + (CmConfigData !=3D NULL); ClockModulation =3D > + &CmConfigData[ProcessorNumber].ClockModulation; > + > + if (State) { > + ClockModulation->Bits.OnDemandClockModulationEnable =3D 1; > + ClockModulation->Bits.OnDemandClockModulationDutyCycle =3D PcdGet8 > (PcdCpuClockModulationDutyCycle) >> 1; > + if > (CmConfigData[ProcessorNumber].ThermalPowerManagementEax.Bits.ECM > D =3D=3D 1) { > + ClockModulation->Bits.ExtendedOnDemandClockModulationDutyCycle > =3D PcdGet8 (PcdCpuClockModulationDutyCycle) & BIT0; > + } > + } else { > + ClockModulation->Bits.OnDemandClockModulationEnable =3D 0; > } > - CPU_REGISTER_TABLE_WRITE_FIELD ( > + > + CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_CLOCK_MODULATION, > - MSR_IA32_CLOCK_MODULATION_REGISTER, > - Bits.OnDemandClockModulationEnable, > - (State) ? 1 : 0 > + ClockModulation->Uint64 > ); > + > return RETURN_SUCCESS; > } > diff --git > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > index af2fc41f759a..9e784e916a85 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > @@ -87,6 +87,21 @@ AesniInitialize ( > IN BOOLEAN State > ); >=20 > +/** > + Prepares for the data used by CPU feature detection and initialization= . > + > + @param[in] NumberOfProcessors The number of CPUs in the platform. > + > + @return Pointer to a buffer of CPU related configuration data. > + > + @note This service could be called by BSP only. > +**/ > +VOID * > +EFIAPI > +ClockModulationGetConfigData ( > + IN UINTN NumberOfProcessors > + ); > + > /** > Detects if Clock Modulation feature supported on current processor. >=20 > diff --git > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > index 738b57dc87f9..b93b898cc959 100644 > --- > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > +++ > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > @@ -47,7 +47,7 @@ CpuCommonFeaturesLibConstructor ( > if (IsCpuFeatureSupported (CPU_FEATURE_ACPI)) { > Status =3D RegisterCpuFeature ( > "ACPI", > - NULL, > + ClockModulationGetConfigData, > ClockModulationSupport, > ClockModulationInitialize, > CPU_FEATURE_ACPI, > -- > 2.21.0.windows.1