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.88, mailfrom: ray.ni@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Mon, 20 May 2019 19:28:45 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 May 2019 19:28:45 -0700 X-ExtLoop1: 1 Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga007.jf.intel.com with ESMTP; 20 May 2019 19:28:45 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 20 May 2019 19:28:23 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.33]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.213]) with mapi id 14.03.0415.000; Tue, 21 May 2019 10:26:10 +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] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Thread-Topic: [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Thread-Index: AQHVDVfgJ6wcTSpM30Cbhg3olLatoKZ02+JA Date: Tue, 21 May 2019 02:26:09 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C153F62@SHSMSX104.ccr.corp.intel.com> References: <20190518085827.78084-1-star.zeng@intel.com> In-Reply-To: <20190518085827.78084-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 Star, The patch looks good. Just 2 minor comments below. > -----Original Message----- > From: Zeng, Star > Sent: Saturday, May 18, 2019 4:58 PM > To: devel@edk2.groups.io > Cc: Zeng, Star ; Laszlo Ersek ; > Dong, Eric ; Ni, Ray ; Kumar, > Chandana C ; Li, Kevin Y > > Subject: [PATCH] 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 bit is just for BSP. > It may have no functionality issue as all processors may have same > bit value 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 (read then write) will be used for > CPU_REGISTER_TABLE_WRITE_FIELD, and AsmWriteMsr64 (write) will be > used > for CPU_REGISTER_TABLE_WRITE64. >=20 > The times of MSR_IA32_CLOCK_MODULATION getting & setting for one > thread: > Without the patch: > 3 getting (3 AsmMsrBitFieldWrite64 for 3 > CPU_REGISTER_TABLE_WRITE_FIELD) > 3 setting (3 AsmMsrBitFieldWrite64 for 3 > CPU_REGISTER_TABLE_WRITE_FIELD) >=20 > With the patch: > One getting (1 AsmReadMsr64 in ClockModulationSupport) > One setting (1 AsmWriteMsr64 for 1 CPU_REGISTER_TABLE_WRITE64) 1. Could be re-wording to: MSR access is 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 With the patch: 1 AsmReadMsr64 (in ClockModulationSupport) + 1 AsmWriteMsr64 (in ClockModulationInitialize) >=20 > Cc: Laszlo Ersek > Cc: Eric Dong > Cc: Ruiyu Ni > Cc: Chandana Kumar > Cc: Kevin Li > Signed-off-by: Star Zeng > --- > .../CpuCommonFeaturesLib/ClockModulation.c | 93 ++++++++++++++----- > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 +++ > .../CpuCommonFeaturesLib.c | 2 +- > 3 files changed, 84 insertions(+), 26 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ClockModulation.c > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ClockModulation.c > index 614768587501..15a4396b6b15 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,26 @@ ClockModulationSupport ( > IN VOID *ConfigData OPTIONAL > ) > { > - return (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI =3D=3D 1); > + CLOCK_MODULATION_CONFIG_DATA *ClockModulationConfigData; > + CPUID_THERMAL_POWER_MANAGEMENT_EAX > *ThermalPowerManagementEax; > + MSR_IA32_CLOCK_MODULATION_REGISTER *ClockModulation; > + > + if (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI =3D=3D 1) { > + ClockModulationConfigData =3D (CLOCK_MODULATION_CONFIG_DATA *) > ConfigData; > + ASSERT (ClockModulationConfigData !=3D NULL); > + ThermalPowerManagementEax =3D > &ClockModulationConfigData[ProcessorNumber].ThermalPowerManageme > ntEax; > + ClockModulation =3D > &ClockModulationConfigData[ProcessorNumber].ClockModulation; > + AsmCpuid ( > + CPUID_THERMAL_POWER_MANAGEMENT, > + &ThermalPowerManagementEax->Uint32, 2. Can we eliminate the local variable ClockModulationConfigData and ThermalPowerManagementEax? The code looks a bit complex: ThremalPowerManagementEax is assigned to the pointer of a UINT32. Then when AsmCpuid() is called, &ThermalPowerManagementEax->Uint32 is passed in. In fact &ThermalPowerManagementEax->Uint32 equals to &ThermalPowerManagementEax. Without the ThremalPowerManagementEax, we can directly use: AsmCpuid ( CPUID_THERMAL_POWER_MANAGEMENT, &ClockModulationConfigData[ProcessorNumber].ThermalPowerManagementEax= .Uint32 > + NULL, > + NULL, > + NULL > + ); > + ClockModulation->Uint64 =3D AsmReadMsr64 > (MSR_IA32_CLOCK_MODULATION); > + return TRUE; > + } > + return FALSE; > } >=20 > /** > @@ -61,34 +107,31 @@ ClockModulationInitialize ( > IN BOOLEAN State > ) > { > - CPUID_THERMAL_POWER_MANAGEMENT_EAX > ThermalPowerManagementEax; > - AsmCpuid (CPUID_THERMAL_POWER_MANAGEMENT, > &ThermalPowerManagementEax.Uint32, NULL, NULL, NULL); > + CLOCK_MODULATION_CONFIG_DATA *ClockModulationConfigData; > + CPUID_THERMAL_POWER_MANAGEMENT_EAX > *ThermalPowerManagementEax; > + 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 > - ); > + ClockModulationConfigData =3D (CLOCK_MODULATION_CONFIG_DATA *) > ConfigData; > + ASSERT (ClockModulationConfigData !=3D NULL); > + ThermalPowerManagementEax =3D > &ClockModulationConfigData[ProcessorNumber].ThermalPowerManageme > ntEax; > + ClockModulation =3D > &ClockModulationConfigData[ProcessorNumber].ClockModulation; > + > + if (State) { > + ClockModulation->Bits.OnDemandClockModulationEnable =3D 1; > + ClockModulation->Bits.OnDemandClockModulationDutyCycle =3D PcdGet8 > (PcdCpuClockModulationDutyCycle) >> 1; > + if (ThermalPowerManagementEax->Bits.ECMD =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