[edk2-devel] [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
Laszlo Ersek
lersek at redhat.com
Mon May 20 14:25:53 UTC 2019
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 <lersek at redhat.com>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Ruiyu Ni <ruiyu.ni at intel.com>
> Cc: Chandana Kumar <chandana.c.kumar at intel.com>
> Cc: Kevin Li <kevin.y.li at intel.com>
> Signed-off-by: Star Zeng <star.zeng at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list