[edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
Ni, Ray
ray.ni at intel.com
Tue May 28 05:51:20 UTC 2019
Reviewed-by: Ray Ni <ray.ni at intel.com>
> -----Original Message-----
> From: Zeng, Star <star.zeng at intel.com>
> Sent: Tuesday, May 21, 2019 6:36 PM
> To: devel at edk2.groups.io
> Cc: Zeng, Star <star.zeng at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar,
> Chandana C <chandana.c.kumar at intel.com>; Li, Kevin Y
> <kevin.y.li at intel.com>
> Subject: [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set
> MSR_IA32_CLOCK_MODULATION
>
> 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 (AsmReadMsr64 + AsmWriteMsr64) will be used for
> CPU_REGISTER_TABLE_WRITE_FIELD, and AsmWriteMsr64 will be used for
> CPU_REGISTER_TABLE_WRITE64.
>
> The times of MSR accessing could be reduced with this patch.
> Without the patch:
> 3 CPU_REGISTER_TABLE_WRITE_FIELD (in ClockModulationInitialize)
> ==> 3 AsmMsrBitFieldWrite64
> ==> 3 AsmReadMsr64 + 3 AsmWriteMsr64
>
> With the patch:
> 1 AsmReadMsr64 (in ClockModulationSupport) +
> 1 CPU_REGISTER_TABLE_WRITE64 (in ClockModulationInitialize)
> ==> 1 AsmWriteMsr64
>
> 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 | 87 +++++++++++++------
> .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 ++++
> .../CpuCommonFeaturesLib.c | 2 +-
> 3 files changed, 78 insertions(+), 26 deletions(-)
>
> 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.
>
> - 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,22 @@ ClockModulationSupport (
> IN VOID *ConfigData OPTIONAL
> )
> {
> - return (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI == 1);
> + CLOCK_MODULATION_CONFIG_DATA *CmConfigData;
> +
> + if (CpuInfo->CpuIdVersionInfoEdx.Bits.ACPI == 1) {
> + CmConfigData = (CLOCK_MODULATION_CONFIG_DATA *) ConfigData;
> + ASSERT (CmConfigData != NULL);
> + AsmCpuid (
> + CPUID_THERMAL_POWER_MANAGEMENT,
> +
> &CmConfigData[ProcessorNumber].ThermalPowerManagementEax.Uint32,
> + NULL,
> + NULL,
> + NULL
> + );
> + CmConfigData[ProcessorNumber].ClockModulation.Uint64 =
> AsmReadMsr64 (MSR_IA32_CLOCK_MODULATION);
> + return TRUE;
> + }
> + return FALSE;
> }
>
> /**
> @@ -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;
>
> - 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
> - );
> + CmConfigData = (CLOCK_MODULATION_CONFIG_DATA *) ConfigData;
> ASSERT
> + (CmConfigData != NULL); ClockModulation =
> + &CmConfigData[ProcessorNumber].ClockModulation;
> +
> + if (State) {
> + ClockModulation->Bits.OnDemandClockModulationEnable = 1;
> + ClockModulation->Bits.OnDemandClockModulationDutyCycle = PcdGet8
> (PcdCpuClockModulationDutyCycle) >> 1;
> + if
> (CmConfigData[ProcessorNumber].ThermalPowerManagementEax.Bits.ECM
> D == 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,
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41444): https://edk2.groups.io/g/devel/message/41444
Mute This Topic: https://groups.io/mt/31695247/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