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 <ler...@redhat.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ruiyu Ni <ruiyu...@intel.com> > Cc: Chandana Kumar <chandana.c.ku...@intel.com> > Cc: Kevin Li <kevin.y...@intel.com> > Signed-off-by: Star Zeng <star.z...@intel.com> > --- > .../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.<BR> > + Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR> > 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, > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41056): https://edk2.groups.io/g/devel/message/41056 Mute This Topic: https://groups.io/mt/31663527/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-