All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] x86/cpufreq: Various bits of cleanup
@ 2021-11-12 18:28 Andrew Cooper
  2021-11-12 18:28 ` [PATCH 1/3] x86/cpufreq: Clean up powernow registration Andrew Cooper
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Andrew Cooper @ 2021-11-12 18:28 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

Varios bits of cleanup uncovered while looking in to the Intel Turbo issues.

Andrew Cooper (3):
  x86/cpufreq: Clean up powernow registration
  x86/cpufreq: Rework APERF/MPERF handling
  x86/cpufreq: Drop opencoded CPUID handling from powernow

 tools/misc/xen-cpuid.c                      |  3 +-
 xen/arch/x86/acpi/cpufreq/cpufreq.c         | 28 +++++++-------
 xen/arch/x86/acpi/cpufreq/powernow.c        | 58 +++++------------------------
 xen/drivers/cpufreq/utility.c               |  9 ++---
 xen/include/acpi/cpufreq/cpufreq.h          |  2 -
 xen/include/asm-x86/cpufeature.h            |  1 +
 xen/include/public/arch-x86/cpufeatureset.h |  2 +
 7 files changed, 31 insertions(+), 72 deletions(-)

-- 
2.11.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] x86/cpufreq: Clean up powernow registration
  2021-11-12 18:28 [PATCH 0/3] x86/cpufreq: Various bits of cleanup Andrew Cooper
@ 2021-11-12 18:28 ` Andrew Cooper
  2021-11-15  9:27   ` Roger Pau Monné
  2021-11-15 11:17   ` Jan Beulich
  2021-11-12 18:28 ` [PATCH 2/3] x86/cpufreq: Rework APERF/MPERF handling Andrew Cooper
  2021-11-12 18:28 ` [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow Andrew Cooper
  2 siblings, 2 replies; 11+ messages in thread
From: Andrew Cooper @ 2021-11-12 18:28 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

powernow_register_driver() is currently written with a K&R type definition;
I'm surprised that compilers don't object to a mismatch with its declaration,
which is written in an ANSI-C compatible way.

Furthermore, its sole caller is cpufreq_driver_init() which is a pre-smp
initcall.  There are no other online CPUs, and even if there were, checking
the BSP's CPUID data $N times is pointless.  Simplify registration to only
look at the BSP.

While at it, drop obviously unused includes.  Also rewrite the expression in
cpufreq_driver_init() for clarity.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/acpi/cpufreq/cpufreq.c  | 20 +++++++++++++-------
 xen/arch/x86/acpi/cpufreq/powernow.c | 28 ++++++----------------------
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index f1f3c6923fb3..2251c87f9e42 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -640,13 +640,19 @@ static int __init cpufreq_driver_init(void)
 {
     int ret = 0;
 
-    if ((cpufreq_controller == FREQCTL_xen) &&
-        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
-        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
-    else if ((cpufreq_controller == FREQCTL_xen) &&
-        (boot_cpu_data.x86_vendor &
-         (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
-        ret = powernow_register_driver();
+    if ( cpufreq_controller == FREQCTL_xen )
+    {
+        switch ( boot_cpu_data.x86_vendor )
+        {
+        case X86_VENDOR_INTEL:
+            ret = cpufreq_register_driver(&acpi_cpufreq_driver);
+            break;
+
+        case X86_VENDOR_AMD | X86_VENDOR_HYGON:
+            ret = powernow_register_driver();
+            break;
+        }
+    }
 
     return ret;
 }
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index f620bebc7e91..80095dfd14b4 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -24,13 +24,9 @@
 #include <xen/types.h>
 #include <xen/errno.h>
 #include <xen/init.h>
-#include <xen/delay.h>
 #include <xen/cpumask.h>
-#include <xen/timer.h>
 #include <xen/xmalloc.h>
-#include <asm/bug.h>
 #include <asm/msr.h>
-#include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/cpufeature.h>
 #include <acpi/acpi.h>
@@ -353,25 +349,13 @@ static const struct cpufreq_driver __initconstrel powernow_cpufreq_driver = {
     .update = powernow_cpufreq_update
 };
 
-unsigned int __init powernow_register_driver()
+unsigned int __init powernow_register_driver(void)
 {
-    unsigned int i, ret = 0;
+    if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
+        return -ENODEV;
 
-    for_each_online_cpu(i) {
-        struct cpuinfo_x86 *c = &cpu_data[i];
-        if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
-            ret = -ENODEV;
-        else
-        {
-            u32 eax, ebx, ecx, edx;
-            cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
-            if ((edx & USE_HW_PSTATE) != USE_HW_PSTATE)
-                ret = -ENODEV;
-        }
-        if (ret)
-            return ret;
-    }
+    if ( !(cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES) & USE_HW_PSTATE) )
+        return -ENODEV;
 
-    ret = cpufreq_register_driver(&powernow_cpufreq_driver);
-    return ret;
+    return cpufreq_register_driver(&powernow_cpufreq_driver);
 }
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] x86/cpufreq: Rework APERF/MPERF handling
  2021-11-12 18:28 [PATCH 0/3] x86/cpufreq: Various bits of cleanup Andrew Cooper
  2021-11-12 18:28 ` [PATCH 1/3] x86/cpufreq: Clean up powernow registration Andrew Cooper
@ 2021-11-12 18:28 ` Andrew Cooper
  2021-11-15 11:23   ` Jan Beulich
  2021-11-12 18:28 ` [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow Andrew Cooper
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2021-11-12 18:28 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

Currently, each feature_detect() (called on CPU add) hook for both cpufreq
drivers duplicates cpu_has_aperfmperf in a per-cpu datastructure, and edits
cpufreq_driver.getavg to point at get_measured_perf().

As all parts of this are vendor-neutral, drop the function pointer and
duplicated boolean, and call get_measured_perf() directly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

Slightly RFC.  This does introduce an arch-specific call into a nominally
arch-neutral driver, but struct cpufreq_policy already had x86-specifics in it
so this is apparently ok.

A (less desirable) alternative would be to keep the function pointer, and
patch it in the BSP path, not every AP based on BSP data.

Either way, with patching moved out of feature_detect(), the main
cpufreq_driver object can become __ro_after_init (in principle), and all calls
optimised with alternative_call().
---
 xen/arch/x86/acpi/cpufreq/cpufreq.c  | 8 +-------
 xen/arch/x86/acpi/cpufreq/powernow.c | 6 ------
 xen/drivers/cpufreq/utility.c        | 9 +++------
 xen/include/acpi/cpufreq/cpufreq.h   | 2 --
 4 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 2251c87f9e42..f26cd6649e7a 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -275,7 +275,7 @@ unsigned int get_measured_perf(unsigned int cpu, unsigned int flag)
         return 0;
 
     policy = per_cpu(cpufreq_cpu_policy, cpu);
-    if (!policy || !policy->aperf_mperf)
+    if ( !policy || !cpu_has_aperfmperf )
         return 0;
 
     switch (flag)
@@ -345,12 +345,6 @@ static void feature_detect(void *info)
     struct cpufreq_policy *policy = info;
     unsigned int eax;
 
-    if ( cpu_has_aperfmperf )
-    {
-        policy->aperf_mperf = 1;
-        cpufreq_driver.getavg = get_measured_perf;
-    }
-
     eax = cpuid_eax(6);
     if (eax & 0x2) {
         policy->turbo = CPUFREQ_TURBO_ENABLED;
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index 80095dfd14b4..82d7827e17c1 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -205,12 +205,6 @@ static void feature_detect(void *info)
     struct cpufreq_policy *policy = info;
     unsigned int edx;
 
-    if ( cpu_has_aperfmperf )
-    {
-        policy->aperf_mperf = 1;
-        cpufreq_driver.getavg = get_measured_perf;
-    }
-
     edx = cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES);
     if ((edx & CPB_CAPABLE) == CPB_CAPABLE) {
         policy->turbo = CPUFREQ_TURBO_ENABLED;
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index b93895d4dddc..9eb7ecedcd29 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -381,12 +381,9 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag)
     if (!cpu_online(cpu) || !(policy = per_cpu(cpufreq_cpu_policy, cpu)))
         return 0;
 
-    if (cpufreq_driver.getavg)
-    {
-        freq_avg = cpufreq_driver.getavg(cpu, flag);
-        if (freq_avg > 0)
-            return freq_avg;
-    }
+    freq_avg = get_measured_perf(cpu, flag);
+    if ( freq_avg > 0 )
+        return freq_avg;
 
     return policy->cur;
 }
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index e88b20bfed4f..4958d3f7d315 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -72,7 +72,6 @@ struct cpufreq_policy {
     s8                  turbo;  /* tristate flag: 0 for unsupported
                                  * -1 for disable, 1 for enabled
                                  * See CPUFREQ_TURBO_* below for defines */
-    bool_t              aperf_mperf; /* CPU has APERF/MPERF MSRs */
 };
 DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy);
 
@@ -162,7 +161,6 @@ struct cpufreq_driver {
                      unsigned int target_freq,
                      unsigned int relation);
     unsigned int    (*get)(unsigned int cpu);
-    unsigned int    (*getavg)(unsigned int cpu, unsigned int flag);
     int    (*exit)(struct cpufreq_policy *policy);
 };
 
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow
  2021-11-12 18:28 [PATCH 0/3] x86/cpufreq: Various bits of cleanup Andrew Cooper
  2021-11-12 18:28 ` [PATCH 1/3] x86/cpufreq: Clean up powernow registration Andrew Cooper
  2021-11-12 18:28 ` [PATCH 2/3] x86/cpufreq: Rework APERF/MPERF handling Andrew Cooper
@ 2021-11-12 18:28 ` Andrew Cooper
  2021-11-15 11:25   ` Jan Beulich
  2021-11-15 11:26   ` Andrew Cooper
  2 siblings, 2 replies; 11+ messages in thread
From: Andrew Cooper @ 2021-11-12 18:28 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

Xen already collects CPUID.0x80000007.edx by default, meaning that we can
refer to per-cpu data directly.  This also avoids the need IPI the onlining
CPU to identify whether Core Performance Boost is available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

I don't think the printk() is very helpful.  I could be talked into retaining
it as a prink_once(), but (irrespective of cpufreq_verbose), it doesn't want
repeating on each CPU because it will either be all or none of them.
---
 tools/misc/xen-cpuid.c                      |  3 ++-
 xen/arch/x86/acpi/cpufreq/powernow.c        | 26 ++++----------------------
 xen/include/asm-x86/cpufeature.h            |  1 +
 xen/include/public/arch-x86/cpufeatureset.h |  2 ++
 4 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 37989e4a12f0..9b59fec26371 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -139,7 +139,8 @@ static const char *const str_7c0[32] =
 
 static const char *const str_e7d[32] =
 {
-    [ 8] = "itsc",
+    /* 6 */                    [ 7] = "hw-pstate",
+    [ 8] = "itsc",             [ 9] = "cpb",
     [10] = "efro",
 };
 
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index 82d7827e17c1..c39af2e23d1a 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -32,9 +32,6 @@
 #include <acpi/acpi.h>
 #include <acpi/cpufreq/cpufreq.h>
 
-#define CPUID_FREQ_VOLT_CAPABILITIES    0x80000007
-#define CPB_CAPABLE             0x00000200
-#define USE_HW_PSTATE           0x00000080
 #define HW_PSTATE_MASK          0x00000007
 #define HW_PSTATE_VALID_MASK    0x80000000
 #define HW_PSTATE_MAX_MASK      0x000000f0
@@ -200,21 +197,6 @@ static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
     return cpufreq_frequency_table_verify(policy, data->freq_table);
 }
 
-static void feature_detect(void *info)
-{
-    struct cpufreq_policy *policy = info;
-    unsigned int edx;
-
-    edx = cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES);
-    if ((edx & CPB_CAPABLE) == CPB_CAPABLE) {
-        policy->turbo = CPUFREQ_TURBO_ENABLED;
-        if (cpufreq_verbose)
-            printk(XENLOG_INFO
-                   "CPU%u: Core Boost/Turbo detected and enabled\n",
-                   smp_processor_id());
-    }
-}
-
 static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
     unsigned int i;
@@ -300,9 +282,9 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy)
     if (result)
         goto err_freqfree;
 
-    if (c->cpuid_level >= 6)
-        on_selected_cpus(cpumask_of(cpu), feature_detect, policy, 1);
-      
+    if ( cpu_has(c, X86_FEATURE_CPB) )
+        policy->turbo = CPUFREQ_TURBO_ENABLED;
+
     /*
      * the first call to ->target() should result in us actually
      * writing something to the appropriate registers.
@@ -348,7 +330,7 @@ unsigned int __init powernow_register_driver(void)
     if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
         return -ENODEV;
 
-    if ( !(cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES) & USE_HW_PSTATE) )
+    if ( !cpu_has_hw_pstate )
         return -ENODEV;
 
     return cpufreq_register_driver(&powernow_cpufreq_driver);
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index ba0fe7c0aa5f..4754940e23f3 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -122,6 +122,7 @@
 #define cpu_has_enqcmd          boot_cpu_has(X86_FEATURE_ENQCMD)
 
 /* CPUID level 0x80000007.edx */
+#define cpu_has_hw_pstate       boot_cpu_has(X86_FEATURE_HW_PSTATE)
 #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
 
 /* CPUID level 0x80000008.ebx */
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index f11d5439aef7..d6260c801ab5 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -247,7 +247,9 @@ XEN_CPUFEATURE(MOVDIR64B,     6*32+28) /*a  MOVDIR64B instruction */
 XEN_CPUFEATURE(ENQCMD,        6*32+29) /*   ENQCMD{,S} instructions */
 
 /* AMD-defined CPU features, CPUID level 0x80000007.edx, word 7 */
+XEN_CPUFEATURE(HW_PSTATE,     7*32+ 7) /*   Hardware Pstates */
 XEN_CPUFEATURE(ITSC,          7*32+ 8) /*a  Invariant TSC */
+XEN_CPUFEATURE(CPB,           7*32+ 9) /*   Core Performance Boost (Turbo) */
 XEN_CPUFEATURE(EFRO,          7*32+10) /*   APERF/MPERF Read Only interface */
 
 /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpufreq: Clean up powernow registration
  2021-11-12 18:28 ` [PATCH 1/3] x86/cpufreq: Clean up powernow registration Andrew Cooper
@ 2021-11-15  9:27   ` Roger Pau Monné
  2021-11-15 11:09     ` Jan Beulich
  2021-11-15 14:03     ` Andrew Cooper
  2021-11-15 11:17   ` Jan Beulich
  1 sibling, 2 replies; 11+ messages in thread
From: Roger Pau Monné @ 2021-11-15  9:27 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu

On Fri, Nov 12, 2021 at 06:28:16PM +0000, Andrew Cooper wrote:
> powernow_register_driver() is currently written with a K&R type definition;
> I'm surprised that compilers don't object to a mismatch with its declaration,
> which is written in an ANSI-C compatible way.
> 
> Furthermore, its sole caller is cpufreq_driver_init() which is a pre-smp
> initcall.  There are no other online CPUs, and even if there were, checking
> the BSP's CPUID data $N times is pointless.  Simplify registration to only
> look at the BSP.

I guess an extra safety would be to add some check for the cpuid bit
in the AP boot path if the cpufreq driver is enabled.

> 
> While at it, drop obviously unused includes.  Also rewrite the expression in
> cpufreq_driver_init() for clarity.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>  xen/arch/x86/acpi/cpufreq/cpufreq.c  | 20 +++++++++++++-------
>  xen/arch/x86/acpi/cpufreq/powernow.c | 28 ++++++----------------------
>  2 files changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> index f1f3c6923fb3..2251c87f9e42 100644
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -640,13 +640,19 @@ static int __init cpufreq_driver_init(void)
>  {
>      int ret = 0;
>  
> -    if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> -    else if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor &
> -         (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
> -        ret = powernow_register_driver();
> +    if ( cpufreq_controller == FREQCTL_xen )
> +    {
> +        switch ( boot_cpu_data.x86_vendor )
> +        {
> +        case X86_VENDOR_INTEL:
> +            ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> +            break;
> +
> +        case X86_VENDOR_AMD | X86_VENDOR_HYGON:

This should be two separate case statements.

With this fixed:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

One rant below about getting rid of powernow_register_driver.

> +            ret = powernow_register_driver();
> +            break;
> +        }
> +    }
>  
>      return ret;
>  }
> diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
> index f620bebc7e91..80095dfd14b4 100644
> --- a/xen/arch/x86/acpi/cpufreq/powernow.c
> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c
> @@ -24,13 +24,9 @@
>  #include <xen/types.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
> -#include <xen/delay.h>
>  #include <xen/cpumask.h>
> -#include <xen/timer.h>
>  #include <xen/xmalloc.h>
> -#include <asm/bug.h>
>  #include <asm/msr.h>
> -#include <asm/io.h>
>  #include <asm/processor.h>
>  #include <asm/cpufeature.h>
>  #include <acpi/acpi.h>
> @@ -353,25 +349,13 @@ static const struct cpufreq_driver __initconstrel powernow_cpufreq_driver = {
>      .update = powernow_cpufreq_update
>  };
>  
> -unsigned int __init powernow_register_driver()
> +unsigned int __init powernow_register_driver(void)
>  {
> -    unsigned int i, ret = 0;
> +    if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
> +        return -ENODEV;
>  
> -    for_each_online_cpu(i) {
> -        struct cpuinfo_x86 *c = &cpu_data[i];
> -        if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
> -            ret = -ENODEV;
> -        else
> -        {
> -            u32 eax, ebx, ecx, edx;
> -            cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
> -            if ((edx & USE_HW_PSTATE) != USE_HW_PSTATE)
> -                ret = -ENODEV;
> -        }
> -        if (ret)
> -            return ret;
> -    }
> +    if ( !(cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES) & USE_HW_PSTATE) )
> +        return -ENODEV;

I wonder if we could move this check into cpufreq_driver_init and get
rid of powernow_register_driver.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpufreq: Clean up powernow registration
  2021-11-15  9:27   ` Roger Pau Monné
@ 2021-11-15 11:09     ` Jan Beulich
  2021-11-15 14:03     ` Andrew Cooper
  1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2021-11-15 11:09 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: Xen-devel, Wei Liu, Andrew Cooper

On 15.11.2021 10:27, Roger Pau Monné wrote:
> On Fri, Nov 12, 2021 at 06:28:16PM +0000, Andrew Cooper wrote:
>> @@ -353,25 +349,13 @@ static const struct cpufreq_driver __initconstrel powernow_cpufreq_driver = {
>>      .update = powernow_cpufreq_update
>>  };
>>  
>> -unsigned int __init powernow_register_driver()
>> +unsigned int __init powernow_register_driver(void)
>>  {
>> -    unsigned int i, ret = 0;
>> +    if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
>> +        return -ENODEV;
>>  
>> -    for_each_online_cpu(i) {
>> -        struct cpuinfo_x86 *c = &cpu_data[i];
>> -        if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
>> -            ret = -ENODEV;
>> -        else
>> -        {
>> -            u32 eax, ebx, ecx, edx;
>> -            cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
>> -            if ((edx & USE_HW_PSTATE) != USE_HW_PSTATE)
>> -                ret = -ENODEV;
>> -        }
>> -        if (ret)
>> -            return ret;
>> -    }
>> +    if ( !(cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES) & USE_HW_PSTATE) )
>> +        return -ENODEV;
> 
> I wonder if we could move this check into cpufreq_driver_init and get
> rid of powernow_register_driver.

That's a vendor specific leaf, so I'd prefer to keep checking it in
vendor-specific code. Dropping the vendor check from here would seem
reasonable, considering it's already done in the caller.

Jan



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpufreq: Clean up powernow registration
  2021-11-12 18:28 ` [PATCH 1/3] x86/cpufreq: Clean up powernow registration Andrew Cooper
  2021-11-15  9:27   ` Roger Pau Monné
@ 2021-11-15 11:17   ` Jan Beulich
  1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2021-11-15 11:17 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel

On 12.11.2021 19:28, Andrew Cooper wrote:
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -640,13 +640,19 @@ static int __init cpufreq_driver_init(void)
>  {
>      int ret = 0;
>  
> -    if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> -    else if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor &
> -         (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
> -        ret = powernow_register_driver();
> +    if ( cpufreq_controller == FREQCTL_xen )
> +    {
> +        switch ( boot_cpu_data.x86_vendor )
> +        {
> +        case X86_VENDOR_INTEL:
> +            ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> +            break;

I've been wondering why we continue to tie this to Intel. I don't
think there's much Intel specific in the ACPI driver, so I wonder
whether this shouldn't use "default:" instead. But I can agree
that's likely better to be done in a separate change.

> @@ -353,25 +349,13 @@ static const struct cpufreq_driver __initconstrel powernow_cpufreq_driver = {
>      .update = powernow_cpufreq_update
>  };
>  
> -unsigned int __init powernow_register_driver()
> +unsigned int __init powernow_register_driver(void)
>  {
> -    unsigned int i, ret = 0;
> +    if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
> +        return -ENODEV;

Ideally with this dropped (and of course with the issue pointed
out by Roger taken care of)

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] x86/cpufreq: Rework APERF/MPERF handling
  2021-11-12 18:28 ` [PATCH 2/3] x86/cpufreq: Rework APERF/MPERF handling Andrew Cooper
@ 2021-11-15 11:23   ` Jan Beulich
  0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2021-11-15 11:23 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel

On 12.11.2021 19:28, Andrew Cooper wrote:
> Currently, each feature_detect() (called on CPU add) hook for both cpufreq
> drivers duplicates cpu_has_aperfmperf in a per-cpu datastructure, and edits
> cpufreq_driver.getavg to point at get_measured_perf().
> 
> As all parts of this are vendor-neutral, drop the function pointer and
> duplicated boolean, and call get_measured_perf() directly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> Slightly RFC.  This does introduce an arch-specific call into a nominally
> arch-neutral driver, but struct cpufreq_policy already had x86-specifics in it
> so this is apparently ok.

That's fine for the time being; disentangling for another arch to use the
driver would be quite a bit of work anyway.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow
  2021-11-12 18:28 ` [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow Andrew Cooper
@ 2021-11-15 11:25   ` Jan Beulich
  2021-11-15 11:26   ` Andrew Cooper
  1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2021-11-15 11:25 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel

On 12.11.2021 19:28, Andrew Cooper wrote:
> Xen already collects CPUID.0x80000007.edx by default, meaning that we can
> refer to per-cpu data directly.  This also avoids the need IPI the onlining
> CPU to identify whether Core Performance Boost is available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow
  2021-11-12 18:28 ` [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow Andrew Cooper
  2021-11-15 11:25   ` Jan Beulich
@ 2021-11-15 11:26   ` Andrew Cooper
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2021-11-15 11:26 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Wei Liu

On 12/11/2021 18:28, Andrew Cooper wrote:
> Xen already collects CPUID.0x80000007.edx by default, meaning that we can
> refer to per-cpu data directly.  This also avoids the need IPI the onlining
> CPU to identify whether Core Performance Boost is available.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
>
> I don't think the printk() is very helpful.  I could be talked into retaining
> it as a prink_once(), but (irrespective of cpufreq_verbose), it doesn't want
> repeating on each CPU because it will either be all or none of them.

I should note that the printk() is also false.  Nothing AFAICT
synchronises the software turbo setting with hardware, until the next
time set-turbo is called.

~Andrew


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpufreq: Clean up powernow registration
  2021-11-15  9:27   ` Roger Pau Monné
  2021-11-15 11:09     ` Jan Beulich
@ 2021-11-15 14:03     ` Andrew Cooper
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2021-11-15 14:03 UTC (permalink / raw)
  To: Roger Pau Monné, Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu

On 15/11/2021 09:27, Roger Pau Monné wrote:
> On Fri, Nov 12, 2021 at 06:28:16PM +0000, Andrew Cooper wrote:
>> powernow_register_driver() is currently written with a K&R type definition;
>> I'm surprised that compilers don't object to a mismatch with its declaration,
>> which is written in an ANSI-C compatible way.
>>
>> Furthermore, its sole caller is cpufreq_driver_init() which is a pre-smp
>> initcall.  There are no other online CPUs, and even if there were, checking
>> the BSP's CPUID data $N times is pointless.  Simplify registration to only
>> look at the BSP.
> I guess an extra safety would be to add some check for the cpuid bit
> in the AP boot path if the cpufreq driver is enabled.

We already have a number of cases where we expect the system to be
reasonably homogeneous, microcode most notably.

Given that we don't currently check this, I don't think it is worth
changing things.
>> While at it, drop obviously unused includes.  Also rewrite the expression in
>> cpufreq_driver_init() for clarity.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> ---
>>  xen/arch/x86/acpi/cpufreq/cpufreq.c  | 20 +++++++++++++-------
>>  xen/arch/x86/acpi/cpufreq/powernow.c | 28 ++++++----------------------
>>  2 files changed, 19 insertions(+), 29 deletions(-)
>>
>> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> index f1f3c6923fb3..2251c87f9e42 100644
>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> @@ -640,13 +640,19 @@ static int __init cpufreq_driver_init(void)
>>  {
>>      int ret = 0;
>>  
>> -    if ((cpufreq_controller == FREQCTL_xen) &&
>> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
>> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>> -    else if ((cpufreq_controller == FREQCTL_xen) &&
>> -        (boot_cpu_data.x86_vendor &
>> -         (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
>> -        ret = powernow_register_driver();
>> +    if ( cpufreq_controller == FREQCTL_xen )
>> +    {
>> +        switch ( boot_cpu_data.x86_vendor )
>> +        {
>> +        case X86_VENDOR_INTEL:
>> +            ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>> +            break;
>> +
>> +        case X86_VENDOR_AMD | X86_VENDOR_HYGON:
> This should be two separate case statements.
>
> With this fixed:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.  I'd actually already found and fixed that bug - clearly I sent
out an old version of the patch.

~Andrew



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-11-15 14:03 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-12 18:28 [PATCH 0/3] x86/cpufreq: Various bits of cleanup Andrew Cooper
2021-11-12 18:28 ` [PATCH 1/3] x86/cpufreq: Clean up powernow registration Andrew Cooper
2021-11-15  9:27   ` Roger Pau Monné
2021-11-15 11:09     ` Jan Beulich
2021-11-15 14:03     ` Andrew Cooper
2021-11-15 11:17   ` Jan Beulich
2021-11-12 18:28 ` [PATCH 2/3] x86/cpufreq: Rework APERF/MPERF handling Andrew Cooper
2021-11-15 11:23   ` Jan Beulich
2021-11-12 18:28 ` [PATCH 3/3] x86/cpufreq: Drop opencoded CPUID handling from powernow Andrew Cooper
2021-11-15 11:25   ` Jan Beulich
2021-11-15 11:26   ` Andrew Cooper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.