1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION

Zeng, Star posted 1 patch 4 years, 11 months ago
Failed in applying to current master (apply log)
.../CpuCommonFeaturesLib/ClockModulation.c    | 87 +++++++++++++------
.../CpuCommonFeaturesLib/CpuCommonFeatures.h  | 15 ++++
.../CpuCommonFeaturesLib.c                    |  2 +-
3 files changed, 78 insertions(+), 26 deletions(-)
[edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
Posted by Zeng, Star 4 years, 11 months ago 
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@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    | 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.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 (#41119): https://edk2.groups.io/g/devel/message/41119
Mute This Topic: https://groups.io/mt/31695247/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Reduce to set MSR_IA32_CLOCK_MODULATION
Posted by Ni, Ray 4 years, 10 months ago 
Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Tuesday, May 21, 2019 6:36 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 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@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    | 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.