1. Patchew
  2. Xen
  3. [XEN PATCH v2 0/2] x86: separate powernow/hwp/acpi cpufreq code
  4. View patch

[XEN PATCH v2 2/2] x86/cpufreq: separate powernow/hwp/acpi cpufreq code

Sergiy Kibrik posted 2 patches 2 months, 2 weeks ago
There is a newer version of this series
[XEN PATCH v2 2/2] x86/cpufreq: separate powernow/hwp/acpi cpufreq code
Posted by Sergiy Kibrik 2 months, 2 weeks ago 
Build AMD Architectural P-state driver when CONFIG_AMD is on, and
Intel Hardware P-States driver together with ACPI Processor P-States driver
when CONFIG_INTEL is on respectively, allowing for a platform-specific build.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
CC: Jason Andryuk <jason.andryuk@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
 - disable acpi-cpufreq driver as well when !INTEL
 - leave a stub for hwp_active() only when !INTEL
 - updated patch description
---
 xen/arch/x86/acpi/cpufreq/Makefile  | 6 +++---
 xen/arch/x86/acpi/cpufreq/cpufreq.c | 8 +++++---
 xen/drivers/acpi/pmstat.c           | 2 +-
 xen/drivers/cpufreq/cpufreq.c       | 3 ++-
 xen/drivers/cpufreq/utility.c       | 2 +-
 xen/include/acpi/cpufreq/cpufreq.h  | 9 +++++++++
 6 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/Makefile b/xen/arch/x86/acpi/cpufreq/Makefile
index 44d4c0b497..e7dbe434a8 100644
--- a/xen/arch/x86/acpi/cpufreq/Makefile
+++ b/xen/arch/x86/acpi/cpufreq/Makefile
@@ -1,4 +1,4 @@
-obj-y += acpi.o
+obj-$(CONFIG_INTEL) += acpi.o
 obj-y += cpufreq.o
-obj-y += hwp.o
-obj-y += powernow.o
+obj-$(CONFIG_INTEL) += hwp.o
+obj-$(CONFIG_AMD) += powernow.o
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index c1a842e959..59ea1f41d4 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -50,10 +50,12 @@ static int __init cf_check cpufreq_driver_init(void)
                 switch ( cpufreq_xen_opts[i] )
                 {
                 case CPUFREQ_xen:
-                    ret = acpi_register_driver();
+                    ret = IS_ENABLED(CONFIG_INTEL) ?
+                          acpi_register_driver() : -ENODEV;
                     break;
                 case CPUFREQ_hwp:
-                    ret = hwp_register_driver();
+                    ret = IS_ENABLED(CONFIG_INTEL) ?
+                          hwp_register_driver() : -ENODEV;
                     break;
                 case CPUFREQ_none:
                     ret = 0;
@@ -67,7 +69,7 @@ static int __init cf_check cpufreq_driver_init(void)
 
         case X86_VENDOR_AMD:
         case X86_VENDOR_HYGON:
-            ret = powernow_register_driver();
+            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
             break;
         }
     }
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index 998d2e3c65..ff6b930794 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -255,7 +255,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
         strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
 
     if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
-                      CPUFREQ_NAME_LEN) )
+                      CPUFREQ_NAME_LEN) && IS_ENABLED(CONFIG_INTEL) )
         ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para);
     else
     {
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 8659ad3aee..e61482e5b0 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -120,7 +120,8 @@ static int __init cf_check setup_cpufreq_option(const char *str)
             if ( arg[0] && arg[1] )
                 ret = cpufreq_cmdline_parse(arg + 1, end);
         }
-        else if ( choice < 0 && !cmdline_strcmp(str, "hwp") )
+        else if ( choice < 0 && !cmdline_strcmp(str, "hwp") &&
+                  IS_ENABLED(CONFIG_INTEL) )
         {
             xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
             cpufreq_controller = FREQCTL_xen;
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index e690a484f1..aab0b78454 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -379,7 +379,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag)
     if (!cpu_online(cpu) || !(policy = per_cpu(cpufreq_cpu_policy, cpu)))
         return 0;
 
-    freq_avg = get_measured_perf(cpu, flag);
+    freq_avg = IS_ENABLED(CONFIG_INTEL) ? get_measured_perf(cpu, flag) : 0;
     if ( freq_avg > 0 )
         return freq_avg;
 
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index 1a8ba3756c..b2e7a592df 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -254,11 +254,20 @@ void intel_feature_detect(struct cpufreq_policy *policy);
 
 int hwp_cmdline_parse(const char *s, const char *e);
 int hwp_register_driver(void);
+#ifdef CONFIG_INTEL
 bool hwp_active(void);
+#else
+static inline bool hwp_active(void)
+{
+    return false;
+}
+#endif
+
 int get_hwp_para(unsigned int cpu,
                  struct xen_cppc_para *cppc_para);
 int set_hwp_para(struct cpufreq_policy *policy,
                  struct xen_set_cppc_para *set_cppc);
 
 int acpi_register_driver(void);
+
 #endif /* __XEN_CPUFREQ_PM_H__ */
-- 
2.25.1
Re: [XEN PATCH v2 2/2] x86/cpufreq: separate powernow/hwp/acpi cpufreq code
Posted by Jan Beulich 2 months, 2 weeks ago 
On 01.07.2024 14:19, Sergiy Kibrik wrote:
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -255,7 +255,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>          strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
>  
>      if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
> -                      CPUFREQ_NAME_LEN) )
> +                      CPUFREQ_NAME_LEN) && IS_ENABLED(CONFIG_INTEL) )

Wrapping like this is confusing, not just because of the flawed indentation.
Please can this be

    if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
                  CPUFREQ_NAME_LEN) &&
         IS_ENABLED(CONFIG_INTEL) )

? Perhaps the IS_ENABLED() would also better be first (not just here).

> --- a/xen/drivers/cpufreq/utility.c
> +++ b/xen/drivers/cpufreq/utility.c
> @@ -379,7 +379,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag)
>      if (!cpu_online(cpu) || !(policy = per_cpu(cpufreq_cpu_policy, cpu)))
>          return 0;
>  
> -    freq_avg = get_measured_perf(cpu, flag);
> +    freq_avg = IS_ENABLED(CONFIG_INTEL) ? get_measured_perf(cpu, flag) : 0;
>      if ( freq_avg > 0 )
>          return freq_avg;

Why is this? APERF/MPERF aren't Intel-only MSRs.

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -254,11 +254,20 @@ void intel_feature_detect(struct cpufreq_policy *policy);
>  
>  int hwp_cmdline_parse(const char *s, const char *e);
>  int hwp_register_driver(void);
> +#ifdef CONFIG_INTEL
>  bool hwp_active(void);
> +#else
> +static inline bool hwp_active(void)
> +{
> +    return false;
> +}
> +#endif
> +
>  int get_hwp_para(unsigned int cpu,
>                   struct xen_cppc_para *cppc_para);
>  int set_hwp_para(struct cpufreq_policy *policy,
>                   struct xen_set_cppc_para *set_cppc);
>  
>  int acpi_register_driver(void);
> +
>  #endif /* __XEN_CPUFREQ_PM_H__ */

Nit: This adding of a blank line should be part of the earlier patch.

Jan
Re: [XEN PATCH v2 2/2] x86/cpufreq: separate powernow/hwp/acpi cpufreq code
Posted by Sergiy Kibrik 2 months, 1 week ago 
03.07.24 18:14, Jan Beulich:
> On 01.07.2024 14:19, Sergiy Kibrik wrote:
>> --- a/xen/drivers/acpi/pmstat.c
>> +++ b/xen/drivers/acpi/pmstat.c
>> @@ -255,7 +255,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>>           strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
>>   
>>       if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
>> -                      CPUFREQ_NAME_LEN) )
>> +                      CPUFREQ_NAME_LEN) && IS_ENABLED(CONFIG_INTEL) )
> Wrapping like this is confusing, not just because of the flawed indentation.
> Please can this be
> 
>      if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
>                    CPUFREQ_NAME_LEN) &&
>           IS_ENABLED(CONFIG_INTEL) )
> 
> ? Perhaps the IS_ENABLED() would also better be first (not just here).

sure, will fix that

> 
>> --- a/xen/drivers/cpufreq/utility.c
>> +++ b/xen/drivers/cpufreq/utility.c
>> @@ -379,7 +379,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag)
>>       if (!cpu_online(cpu) || !(policy = per_cpu(cpufreq_cpu_policy, cpu)))
>>           return 0;
>>   
>> -    freq_avg = get_measured_perf(cpu, flag);
>> +    freq_avg = IS_ENABLED(CONFIG_INTEL) ? get_measured_perf(cpu, flag) : 0;
>>       if ( freq_avg > 0 )
>>           return freq_avg;
> Why is this? APERF/MPERF aren't Intel-only MSRs.

yes, it seems to be a mistake..

  -Sergiy

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.