Star,
The patch looks good.
Just 2 minor comments below.
> -----Original Message-----
> From: Zeng, Star <star.z...@intel.com>
> Sent: Saturday, May 18, 2019 4:58 PM
> To: devel@edk2.groups.io
> Cc: Zeng, Star <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com>;
> Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Kumar,
> Chandana C <chandana.c.ku...@intel.com>; Li, Kevin Y
> <kevin.y...@intel.com>
> Subject: [PATCH] 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 (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)
1. Could be re-wording to:
MSR access is 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 AsmWriteMsr64 (in ClockModulationInitialize)
>
> 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(-)
>
> 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].ThermalPowerManageme
> ntEax;
> + ClockModulation =
> &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 = 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].ThermalPowerManageme
> ntEax;
> + 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,
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41101): https://edk2.groups.io/g/devel/message/41101
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]
-=-=-=-=-=-=-=-=-=-=-=-
