* [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
@ 2019-05-18 8:58 Zeng, Star
2019-05-20 14:25 ` Laszlo Ersek
2019-05-21 2:26 ` Ni, Ray
0 siblings, 2 replies; 3+ messages in thread
From: Zeng, Star @ 2019-05-18 8:58 UTC (permalink / raw)
To: devel
Cc: Star Zeng, Laszlo Ersek, Eric Dong, Ruiyu Ni, Chandana Kumar,
Kevin Li
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@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Kevin Li <kevin.y.li@intel.com>
Signed-off-by: Star Zeng <star.zeng@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].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,
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
2019-05-18 8:58 [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Zeng, Star
@ 2019-05-20 14:25 ` Laszlo Ersek
2019-05-21 2:26 ` Ni, Ray
1 sibling, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2019-05-20 14:25 UTC (permalink / raw)
To: Star Zeng, devel; +Cc: Eric Dong, Ruiyu Ni, Chandana Kumar, Kevin Li
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@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Kevin Li <kevin.y.li@intel.com>
> Signed-off-by: Star Zeng <star.zeng@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,
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
2019-05-18 8:58 [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Zeng, Star
2019-05-20 14:25 ` Laszlo Ersek
@ 2019-05-21 2:26 ` Ni, Ray
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ray @ 2019-05-21 2:26 UTC (permalink / raw)
To: Zeng, Star, devel@edk2.groups.io
Cc: Laszlo Ersek, Dong, Eric, Kumar, Chandana C, Li, Kevin Y
Star,
The patch looks good.
Just 2 minor comments below.
> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Saturday, May 18, 2019 4:58 PM
> To: devel@edk2.groups.io
> Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Li, Kevin Y
> <kevin.y.li@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 <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Kevin Li <kevin.y.li@intel.com>
> Signed-off-by: Star Zeng <star.zeng@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-21 2:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-18 8:58 [PATCH] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION Zeng, Star
2019-05-20 14:25 ` Laszlo Ersek
2019-05-21 2:26 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox