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; Mon, 20 May 2019 07:26:11 -0700 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 CACB83082B3F; Mon, 20 May 2019 14:25:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-154.rdu2.redhat.com [10.10.124.154]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3B1371001F5D; Mon, 20 May 2019 14:25:54 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION To: Star Zeng , devel@edk2.groups.io Cc: Eric Dong , Ruiyu Ni , Chandana Kumar , Kevin Li References: <20190518085827.78084-1-star.zeng@intel.com> From: "Laszlo Ersek" Message-ID: <25855c06-c28b-d3a5-a353-53589bfe9b43@redhat.com> Date: Mon, 20 May 2019 16:25:53 +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: <20190518085827.78084-1-star.zeng@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.45]); Mon, 20 May 2019 14:26:00 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Star, On 05/18/19 10:58, Star Zeng wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1810 > > This patch covers two problems. > > 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. > > 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(). > > 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. > > 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. > > 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) > > With the patch: > One getting (1 AsmReadMsr64 in ClockModulationSupport) > One setting (1 AsmWriteMsr64 for 1 CPU_REGISTER_TABLE_WRITE64) > > 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(-) I'll defer to Eric and Ray on this patch. Thank you for the CC! Laszlo > 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. > > - 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 > > **/ > > #include "CpuCommonFeatures.h" > > +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 = AllocateZeroPool (sizeof (CLOCK_MODULATION_CONFIG_DATA) * NumberOfProcessors); > + ASSERT (ConfigData != NULL); > + return ConfigData; > +} > + > /** > Detects if Clock Modulation feature supported on current processor. > > @@ -32,7 +59,26 @@ ClockModulationSupport ( > IN VOID *ConfigData OPTIONAL > ) > { > - return (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI == 1); > + CLOCK_MODULATION_CONFIG_DATA *ClockModulationConfigData; > + CPUID_THERMAL_POWER_MANAGEMENT_EAX *ThermalPowerManagementEax; > + MSR_IA32_CLOCK_MODULATION_REGISTER *ClockModulation; > + > + if (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI == 1) { > + ClockModulationConfigData = (CLOCK_MODULATION_CONFIG_DATA *) ConfigData; > + ASSERT (ClockModulationConfigData != NULL); > + ThermalPowerManagementEax = &ClockModulationConfigData[ProcessorNumber].ThermalPowerManagementEax; > + ClockModulation = &ClockModulationConfigData[ProcessorNumber].ClockModulation; > + AsmCpuid ( > + CPUID_THERMAL_POWER_MANAGEMENT, > + &ThermalPowerManagementEax->Uint32, > + NULL, > + NULL, > + NULL > + ); > + ClockModulation->Uint64 = AsmReadMsr64 (MSR_IA32_CLOCK_MODULATION); > + return TRUE; > + } > + return FALSE; > } > > /** > @@ -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; > > - 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 == 1) { > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_CLOCK_MODULATION, > - MSR_IA32_CLOCK_MODULATION_REGISTER, > - Bits.ExtendedOnDemandClockModulationDutyCycle, > - PcdGet8 (PcdCpuClockModulationDutyCycle) & BIT0 > - ); > + ClockModulationConfigData = (CLOCK_MODULATION_CONFIG_DATA *) ConfigData; > + ASSERT (ClockModulationConfigData != NULL); > + ThermalPowerManagementEax = &ClockModulationConfigData[ProcessorNumber].ThermalPowerManagementEax; > + ClockModulation = &ClockModulationConfigData[ProcessorNumber].ClockModulation; > + > + if (State) { > + ClockModulation->Bits.OnDemandClockModulationEnable = 1; > + ClockModulation->Bits.OnDemandClockModulationDutyCycle = PcdGet8 (PcdCpuClockModulationDutyCycle) >> 1; > + if (ThermalPowerManagementEax->Bits.ECMD == 1) { > + ClockModulation->Bits.ExtendedOnDemandClockModulationDutyCycle = PcdGet8 (PcdCpuClockModulationDutyCycle) & BIT0; > + } > + } else { > + ClockModulation->Bits.OnDemandClockModulationEnable = 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 > ); > > +/** > + 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. > > 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 = RegisterCpuFeature ( > "ACPI", > - NULL, > + ClockModulationGetConfigData, > ClockModulationSupport, > ClockModulationInitialize, > CPU_FEATURE_ACPI, >