Users need to set "cpufreq=amd-cppc" in xen cmdline to enable amd-cppc driver,
which selects ACPI Collaborative Performance and Power Control (CPPC) on
supported AMD hardware to provide a finer grained frequency control mechanism.
`verbose` option can also be included to support verbose print.
When users setting "cpufreq=amd-cppc", a new amd-cppc driver
shall be registered and used. All hooks for amd-cppc driver are transiently
missing, and we temporarily make registration fail with -EOPNOTSUPP here. It
will be fixed along with the implementation.
New xen-pm internal flag XEN_PROCESSOR_PM_CPPC is introduced, to stand for
cpufreq driver in CPPC mode. We define XEN_PROCESSOR_PM_CPPC 0x100, as it is
the next value to use after 8-bits wide public xen-pm options. We also add
sanity check on compile time. All XEN_PROCESSOR_PM_xxx checking shall be
updated to consider "XEN_PROCESSOR_PM_CPPC" too.
XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX are firstly set when Xen parsed
relative driver signature from xen cmdline, and will become exclusive after
cpufreq driver registration. It is because that platform could not support
both or mixed mode (CPPC & legacy Px) operations, and only one cpufreq driver
could be registerd in Xen at one time, such as on AMD, it is either amd-cppc
or legacy P-states driver.
Xen rely on XEN_PROCESSOR_PM_CPPC flag to tell current cpufreq driver is in
CPPC mode, and accepts relative hypercall. It will neglect Px request and
yields success.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- Obey to alphabetic sorting and also strict it with CONFIG_AMD
- Remove unnecessary empty comment line
- Use __initconst_cf_clobber for pre-filled structure cpufreq_driver
- Make new switch-case code apply to Hygon CPUs too
- Change ENOSYS with EOPNOTSUPP
- Blanks around binary operator
- Change all amd_/-pstate defined values to amd_/-cppc
---
v2 -> v3
- refactor too long lines
- Make sure XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC incompatible flags
after cpufreq register registrantion
---
v3 -> v4:
- introduce XEN_PROCESSOR_PM_CPPC in xen internal header
- complement "Hygon" in log message
- remove unnecessary if()
- grow cpufreq_xen_opts[] array
---
v4 -> v5:
- remove XEN_PROCESSOR_PM_xxx flag sanitization from individual driver
- prefer ! over "== 0" in purely boolean contexts
- Blank line between non-fall-through case blocks
- add build-time checking between internal and public XEN_PROCESSOR_PM_*
values
- define XEN_PROCESSOR_PM_CPPC with 0x100, as it is the next value to use
after public interface, and public mask SIF_PM_MASK is 8 bits wide.
- as Dom0 will send the CPPC/Px data whenever it could, the return value shall
be 0 instead of -ENOSYS/EOPNOTSUP when platform doesn't require these data.
---
v5 -> v6:
- do not register the driver when all hooks are NULL
- refactor the subject and commit message
- move pruning of xen_processor_pmbits into generic space
- add comment and build-time check for XEN_PROCESSOR_PM_CPPC
---
docs/misc/xen-command-line.pandoc | 7 ++-
xen/arch/x86/acpi/cpufreq/Makefile | 1 +
xen/arch/x86/acpi/cpufreq/amd-cppc.c | 59 +++++++++++++++++++
xen/arch/x86/acpi/cpufreq/cpufreq.c | 72 ++++++++++++++++++++++-
xen/arch/x86/platform_hypercall.c | 14 +++++
xen/drivers/acpi/pm-op.c | 3 +-
xen/drivers/acpi/pmstat.c | 3 +
xen/drivers/cpufreq/cpufreq.c | 11 ++++
xen/include/acpi/cpufreq/cpufreq.h | 6 +-
xen/include/acpi/cpufreq/processor_perf.h | 10 ++++
10 files changed, 180 insertions(+), 6 deletions(-)
create mode 100644 xen/arch/x86/acpi/cpufreq/amd-cppc.c
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 6865a61220..03761d9e3c 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -515,7 +515,7 @@ If set, force use of the performance counters for oprofile, rather than detectin
available support.
### cpufreq
-> `= none | {{ <boolean> | xen } { [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfreq>]]] } [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]]`
+> `= none | {{ <boolean> | xen } { [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfreq>]]] } [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] | amd-cppc[:[verbose]]`
> Default: `xen`
@@ -526,7 +526,7 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels.
* `<maxfreq>` and `<minfreq>` are integers which represent max and min processor frequencies
respectively.
* `verbose` option can be included as a string or also as `verbose=<integer>`
- for `xen`. It is a boolean for `hwp`.
+ for `xen`. It is a boolean for `hwp` and `amd-cppc`.
* `hwp` selects Hardware-Controlled Performance States (HWP) on supported Intel
hardware. HWP is a Skylake+ feature which provides better CPU power
management. The default is disabled. If `hwp` is selected, but hardware
@@ -534,6 +534,9 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels.
* `<hdc>` is a boolean to enable Hardware Duty Cycling (HDC). HDC enables the
processor to autonomously force physical package components into idle state.
The default is enabled, but the option only applies when `hwp` is enabled.
+* `amd-cppc` selects ACPI Collaborative Performance and Power Control (CPPC)
+ on supported AMD hardware to provide finer grained frequency control
+ mechanism. The default is disabled.
There is also support for `;`-separated fallback options:
`cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to `xen` if
diff --git a/xen/arch/x86/acpi/cpufreq/Makefile b/xen/arch/x86/acpi/cpufreq/Makefile
index e7dbe434a8..a2ba34bda0 100644
--- a/xen/arch/x86/acpi/cpufreq/Makefile
+++ b/xen/arch/x86/acpi/cpufreq/Makefile
@@ -1,4 +1,5 @@
obj-$(CONFIG_INTEL) += acpi.o
+obj-$(CONFIG_AMD) += amd-cppc.o
obj-y += cpufreq.o
obj-$(CONFIG_INTEL) += hwp.o
obj-$(CONFIG_AMD) += powernow.o
diff --git a/xen/arch/x86/acpi/cpufreq/amd-cppc.c b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
new file mode 100644
index 0000000000..3377783f7e
--- /dev/null
+++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * amd-cppc.c - AMD Processor CPPC Frequency Driver
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc. All Rights Reserved.
+ *
+ * Author: Penny Zheng <penny.zheng@amd.com>
+ *
+ * AMD CPPC cpufreq driver introduces a new CPU performance scaling design
+ * for AMD processors using the ACPI Collaborative Performance and Power
+ * Control (CPPC) feature which provides finer grained frequency control range.
+ */
+
+#include <xen/domain.h>
+#include <xen/init.h>
+#include <xen/param.h>
+#include <acpi/cpufreq/cpufreq.h>
+
+static bool __init amd_cppc_handle_option(const char *s, const char *end)
+{
+ int ret;
+
+ ret = parse_boolean("verbose", s, end);
+ if ( ret >= 0 )
+ {
+ cpufreq_verbose = ret;
+ return true;
+ }
+
+ return false;
+}
+
+int __init amd_cppc_cmdline_parse(const char *s, const char *e)
+{
+ do {
+ const char *end = strpbrk(s, ",;");
+
+ if ( !amd_cppc_handle_option(s, end) )
+ {
+ printk(XENLOG_WARNING
+ "cpufreq/amd-cppc: option '%.*s' not recognized\n",
+ (int)((end ?: e) - s), s);
+
+ return -EINVAL;
+ }
+
+ s = end ? end + 1 : NULL;
+ } while ( s && s < e );
+
+ return 0;
+}
+
+int __init amd_cppc_register_driver(void)
+{
+ if ( !cpu_has_cppc )
+ return -ENODEV;
+
+ return -EOPNOTSUPP;
+}
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 45f301f354..b33699ef13 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -128,12 +128,14 @@ static int __init cf_check cpufreq_driver_init(void)
if ( cpufreq_controller == FREQCTL_xen )
{
+ unsigned int i = 0;
+
switch ( boot_cpu_data.x86_vendor )
{
case X86_VENDOR_INTEL:
ret = -ENOENT;
- for ( unsigned int i = 0; i < cpufreq_xen_cnt; i++ )
+ for ( i = 0; i < cpufreq_xen_cnt; i++ )
{
switch ( cpufreq_xen_opts[i] )
{
@@ -148,6 +150,11 @@ static int __init cf_check cpufreq_driver_init(void)
case CPUFREQ_none:
ret = 0;
break;
+
+ default:
+ printk(XENLOG_WARNING
+ "Unsupported cpufreq driver for vendor Intel\n");
+ break;
}
if ( !ret || ret == -EBUSY )
@@ -157,9 +164,70 @@ static int __init cf_check cpufreq_driver_init(void)
case X86_VENDOR_AMD:
case X86_VENDOR_HYGON:
- ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
+ if ( !IS_ENABLED(CONFIG_AMD) )
+ {
+ ret = -ENODEV;
+ break;
+ }
+ ret = -ENOENT;
+
+ for ( i = 0; i < cpufreq_xen_cnt; i++ )
+ {
+ switch ( cpufreq_xen_opts[i] )
+ {
+ case CPUFREQ_xen:
+ ret = powernow_register_driver();
+ break;
+
+ case CPUFREQ_amd_cppc:
+ ret = amd_cppc_register_driver();
+ break;
+
+ case CPUFREQ_none:
+ ret = 0;
+ break;
+
+ default:
+ printk(XENLOG_WARNING
+ "Unsupported cpufreq driver for vendor AMD or Hygon\n");
+ break;
+ }
+
+ if ( !ret || ret == -EBUSY )
+ break;
+ }
+
break;
}
+
+ /*
+ * After successful cpufreq driver registeration, XEN_PROCESSOR_PM_CPPC
+ * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
+ */
+ if ( !ret )
+ {
+ ASSERT(i < cpufreq_xen_cnt);
+ switch ( cpufreq_xen_opts[i] )
+ {
+ case CPUFREQ_amd_cppc:
+ xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
+ break;
+
+ case CPUFREQ_hwp:
+ case CPUFREQ_xen:
+ xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
+ break;
+
+ default:
+ break;
+ }
+ } else if ( ret != -EBUSY )
+ /*
+ * No cpufreq driver gets registered, clear both
+ * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
+ */
+ xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
+ XEN_PROCESSOR_PM_PX);
}
return ret;
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 42b3b8b95a..cf64b8a622 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -546,6 +546,8 @@ ret_t do_platform_op(
ret = 0;
break;
}
+ /* Xen doesn't support mixed mode */
+ ASSERT(!(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC));
ret = set_px_pminfo(op->u.set_pminfo.id, &op->u.set_pminfo.u.perf);
break;
@@ -578,6 +580,18 @@ ret_t do_platform_op(
}
case XEN_PM_CPPC:
+ if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) )
+ {
+ /*
+ * Neglect CPPC-info when registered cpufreq driver
+ * isn't in CPPC mode
+ */
+ ret = 0;
+ break;
+ }
+ /* Xen doesn't support mixed mode */
+ ASSERT(!(xen_processor_pmbits & XEN_PROCESSOR_PM_PX));
+
ret = set_cppc_pminfo(op->u.set_pminfo.id,
&op->u.set_pminfo.u.cppc_data);
break;
diff --git a/xen/drivers/acpi/pm-op.c b/xen/drivers/acpi/pm-op.c
index 49b4067dec..d10f6db0e4 100644
--- a/xen/drivers/acpi/pm-op.c
+++ b/xen/drivers/acpi/pm-op.c
@@ -350,7 +350,8 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
switch ( op->cmd & PM_PARA_CATEGORY_MASK )
{
case CPUFREQ_PARA:
- if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
+ if ( !(xen_processor_pmbits & (XEN_PROCESSOR_PM_PX |
+ XEN_PROCESSOR_PM_CPPC)) )
return -ENODEV;
if ( !pmpt || !(pmpt->init & (XEN_PX_INIT | XEN_CPPC_INIT)) )
return -EINVAL;
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index da7a1f81e1..e4e62966de 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -107,6 +107,9 @@ int cpufreq_statistic_init(unsigned int cpu)
if ( !pmpt )
return -EINVAL;
+ if ( !(pmpt->init & XEN_PX_INIT) )
+ return 0;
+
spin_lock(cpufreq_statistic_lock);
pxpt = per_cpu(cpufreq_statistic_data, cpu);
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 065fdf4106..cf1fcf1d22 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -98,6 +98,10 @@ static int __init handle_cpufreq_cmdline(enum cpufreq_xen_opt option)
cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
switch ( option )
{
+ case CPUFREQ_amd_cppc:
+ xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC;
+ break;
+
case CPUFREQ_hwp:
case CPUFREQ_xen:
xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
@@ -166,6 +170,13 @@ static int __init cf_check setup_cpufreq_option(const char *str)
if ( !ret && arg[0] && arg[1] )
ret = hwp_cmdline_parse(arg + 1, end);
}
+ else if ( IS_ENABLED(CONFIG_AMD) && choice < 0 &&
+ !cmdline_strcmp(str, "amd-cppc") )
+ {
+ ret = handle_cpufreq_cmdline(CPUFREQ_amd_cppc);
+ if ( !ret && arg[0] && arg[1] )
+ ret = amd_cppc_cmdline_parse(arg + 1, end);
+ }
else
ret = -EINVAL;
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index 7f26b5653a..32cf905fb8 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -26,8 +26,9 @@ enum cpufreq_xen_opt {
CPUFREQ_none,
CPUFREQ_xen,
CPUFREQ_hwp,
+ CPUFREQ_amd_cppc,
};
-#define NR_CPUFREQ_OPTS 2
+#define NR_CPUFREQ_OPTS 3
extern enum cpufreq_xen_opt cpufreq_xen_opts[NR_CPUFREQ_OPTS];
extern unsigned int cpufreq_xen_cnt;
struct cpufreq_governor;
@@ -273,4 +274,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
int acpi_cpufreq_register(void);
+int amd_cppc_cmdline_parse(const char *s, const char *e);
+int amd_cppc_register_driver(void);
+
#endif /* __XEN_CPUFREQ_PM_H__ */
diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/acpi/cpufreq/processor_perf.h
index f80495fc96..6d8d29d440 100644
--- a/xen/include/acpi/cpufreq/processor_perf.h
+++ b/xen/include/acpi/cpufreq/processor_perf.h
@@ -5,6 +5,16 @@
#include <public/sysctl.h>
#include <xen/acpi.h>
+/*
+ * Internal xen-pm options
+ * They are extension to public xen-pm options (XEN_PROCESSOR_PM_xxx) defined
+ * in public/platform.h, guarded by SIF_PM_MASK
+ */
+#define XEN_PROCESSOR_PM_CPPC 0x100
+#if XEN_PROCESSOR_PM_CPPC & MASK_EXTR(~0, SIF_PM_MASK)
+# error "XEN_PROCESSOR_PM_CPPC shall not occupy bits reserved for public xen-pm options"
+#endif
+
#define XEN_CPPC_INIT 0x40000000U
#define XEN_PX_INIT 0x80000000U
--
2.34.1On 11.07.2025 05:50, Penny Zheng wrote:
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -128,12 +128,14 @@ static int __init cf_check cpufreq_driver_init(void)
>
> if ( cpufreq_controller == FREQCTL_xen )
> {
> + unsigned int i = 0;
Pointless initializer; both for() loops set i to 0. But also see further
down.
> @@ -157,9 +164,70 @@ static int __init cf_check cpufreq_driver_init(void)
>
> case X86_VENDOR_AMD:
> case X86_VENDOR_HYGON:
> - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
> + if ( !IS_ENABLED(CONFIG_AMD) )
> + {
> + ret = -ENODEV;
> + break;
> + }
> + ret = -ENOENT;
The code structure is sufficiently different from the Intel counterpart for
this to perhaps better move ...
> + for ( i = 0; i < cpufreq_xen_cnt; i++ )
> + {
> + switch ( cpufreq_xen_opts[i] )
> + {
> + case CPUFREQ_xen:
> + ret = powernow_register_driver();
> + break;
> +
> + case CPUFREQ_amd_cppc:
> + ret = amd_cppc_register_driver();
> + break;
> +
> + case CPUFREQ_none:
> + ret = 0;
> + break;
> +
> + default:
> + printk(XENLOG_WARNING
> + "Unsupported cpufreq driver for vendor AMD or Hygon\n");
> + break;
... here.
> + }
> +
> + if ( !ret || ret == -EBUSY )
> + break;
> + }
> +
> break;
> }
> +
> + /*
> + * After successful cpufreq driver registeration, XEN_PROCESSOR_PM_CPPC
> + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
> + */
> + if ( !ret )
> + {
> + ASSERT(i < cpufreq_xen_cnt);
> + switch ( cpufreq_xen_opts[i] )
Hmm, this is using the the initializer of i that I commented on. I think there's
another default: case missing, where you simply "return 0" (to retain prior
behavior). But again see also yet further down.
> + {
> + case CPUFREQ_amd_cppc:
> + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
> + break;
> +
> + case CPUFREQ_hwp:
> + case CPUFREQ_xen:
> + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
> + break;
> +
> + default:
> + break;
> + }
> + } else if ( ret != -EBUSY )
Nit (style): Closing brace wants to be on its own line.
> + /*
> + * No cpufreq driver gets registered, clear both
> + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
> + */
> + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
> + XEN_PROCESSOR_PM_PX);
Yet more hmm - this path you want to get through for the case mentioned above.
But only this code; specifically not the "switch ( cpufreq_xen_opts[i] )",
which really is "switch ( cpufreq_xen_opts[0] )" in that case, and that's
pretty clearly wrong to evaluate in then.
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -107,6 +107,9 @@ int cpufreq_statistic_init(unsigned int cpu)
> if ( !pmpt )
> return -EINVAL;
>
> + if ( !(pmpt->init & XEN_PX_INIT) )
> + return 0;
> +
> spin_lock(cpufreq_statistic_lock);
>
> pxpt = per_cpu(cpufreq_statistic_data, cpu);
This change could do with a code comment, I think.
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -98,6 +98,10 @@ static int __init handle_cpufreq_cmdline(enum cpufreq_xen_opt option)
> cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
> switch ( option )
> {
> + case CPUFREQ_amd_cppc:
> + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC;
> + break;
> +
> case CPUFREQ_hwp:
> case CPUFREQ_xen:
> xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
Unless they're clearly "more important" (tm), please can insertions like
this not be done at the top of a switch() (or whatever else it is)? You
don't do so ...
> @@ -166,6 +170,13 @@ static int __init cf_check setup_cpufreq_option(const char *str)
> if ( !ret && arg[0] && arg[1] )
> ret = hwp_cmdline_parse(arg + 1, end);
> }
> + else if ( IS_ENABLED(CONFIG_AMD) && choice < 0 &&
> + !cmdline_strcmp(str, "amd-cppc") )
> + {
> + ret = handle_cpufreq_cmdline(CPUFREQ_amd_cppc);
> + if ( !ret && arg[0] && arg[1] )
> + ret = amd_cppc_cmdline_parse(arg + 1, end);
> + }
> else
> ret = -EINVAL;
... here, for example.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, July 17, 2025 12:00 AM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline
> and amd-cppc driver
>
> On 11.07.2025 05:50, Penny Zheng wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > @@ -128,12 +128,14 @@ static int __init cf_check
> > cpufreq_driver_init(void)
> >
> > if ( cpufreq_controller == FREQCTL_xen )
> > {
> > + unsigned int i = 0;
>
> Pointless initializer; both for() loops set i to 0. But also see further down.
>
> > @@ -157,9 +164,70 @@ static int __init cf_check
> > cpufreq_driver_init(void)
> >
> > case X86_VENDOR_AMD:
> > case X86_VENDOR_HYGON:
> > - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
> ENODEV;
> > + if ( !IS_ENABLED(CONFIG_AMD) )
> > + {
> > + ret = -ENODEV;
> > + break;
> > + }
> > + ret = -ENOENT;
>
> The code structure is sufficiently different from the Intel counterpart for this to
> perhaps better move ...
>
> > + for ( i = 0; i < cpufreq_xen_cnt; i++ )
> > + {
> > + switch ( cpufreq_xen_opts[i] )
> > + {
> > + case CPUFREQ_xen:
> > + ret = powernow_register_driver();
> > + break;
> > +
> > + case CPUFREQ_amd_cppc:
> > + ret = amd_cppc_register_driver();
> > + break;
> > +
> > + case CPUFREQ_none:
> > + ret = 0;
> > + break;
> > +
> > + default:
> > + printk(XENLOG_WARNING
> > + "Unsupported cpufreq driver for vendor AMD or Hygon\n");
> > + break;
>
> ... here.
>
Are we suggesting moving
"
if ( !IS_ENABLED(CONFIG_AMD) )
{
ret = -ENODEV;
break;
}
" here? In which case, When CONFIG_AMD=n and users doesn't provide "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen. powernow_register_driver() hence gets invoked. The thing is that we don't have stub for it and it is compiled under CONFIG_AMD
I suggest to change to use #ifdef CONFIG_AMD code wrapping
> > + }
> > +
> > + if ( !ret || ret == -EBUSY )
> > + break;
> > + }
> > +
> > break;
> > }
> > +
> > + /*
> > + * After successful cpufreq driver registeration,
> XEN_PROCESSOR_PM_CPPC
> > + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
> > + */
> > + if ( !ret )
> > + {
> > + ASSERT(i < cpufreq_xen_cnt);
> > + switch ( cpufreq_xen_opts[i] )
>
> Hmm, this is using the the initializer of i that I commented on. I think there's
> another default: case missing, where you simply "return 0" (to retain prior behavior).
> But again see also yet further down.
>
>
> > + /*
> > + * No cpufreq driver gets registered, clear both
> > + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
> > + */
> > + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
> > + XEN_PROCESSOR_PM_PX);
>
> Yet more hmm - this path you want to get through for the case mentioned above.
> But only this code; specifically not the "switch ( cpufreq_xen_opts[i] )", which really
> is "switch ( cpufreq_xen_opts[0] )" in that case, and that's pretty clearly wrong to
> evaluate in then.
>
Correct me if I understand you wrongly:
The above "case missing" , are we talking about is entering "case CPUFREQ_none" ?
IMO, it may never be entered. If users doesn't provide "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen. That is, we will have px states as default driver. Even if we have failed px-driver initialization, with cpufreq_xen_cnt limited to 1, we will not enter CPUFREQ_none.
CPUFREQ_none only could be set when users explicitly set "cpufreq=disabled/none/0", but in which case, cpufreq_controller will be set with FREQCTL_none. And the whole cpufreq_driver_init() is under " cpufreq_controller == FREQCTL_xen " condition
Or "case missing" is referring entering default case? In which case, we will have -ENOENT errno. As we have ret=-ENOENT in the very beginning
> Jan
On 04.08.2025 10:09, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, July 17, 2025 12:00 AM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
>> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline
>> and amd-cppc driver
>>
>> On 11.07.2025 05:50, Penny Zheng wrote:
>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> @@ -128,12 +128,14 @@ static int __init cf_check
>>> cpufreq_driver_init(void)
>>>
>>> if ( cpufreq_controller == FREQCTL_xen )
>>> {
>>> + unsigned int i = 0;
>>
>> Pointless initializer; both for() loops set i to 0. But also see further down.
>>
>>> @@ -157,9 +164,70 @@ static int __init cf_check
>>> cpufreq_driver_init(void)
>>>
>>> case X86_VENDOR_AMD:
>>> case X86_VENDOR_HYGON:
>>> - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
>> ENODEV;
>>> + if ( !IS_ENABLED(CONFIG_AMD) )
>>> + {
>>> + ret = -ENODEV;
>>> + break;
>>> + }
>>> + ret = -ENOENT;
>>
>> The code structure is sufficiently different from the Intel counterpart for this to
>> perhaps better move ...
>>
>>> + for ( i = 0; i < cpufreq_xen_cnt; i++ )
>>> + {
>>> + switch ( cpufreq_xen_opts[i] )
>>> + {
>>> + case CPUFREQ_xen:
>>> + ret = powernow_register_driver();
>>> + break;
>>> +
>>> + case CPUFREQ_amd_cppc:
>>> + ret = amd_cppc_register_driver();
>>> + break;
>>> +
>>> + case CPUFREQ_none:
>>> + ret = 0;
>>> + break;
>>> +
>>> + default:
>>> + printk(XENLOG_WARNING
>>> + "Unsupported cpufreq driver for vendor AMD or Hygon\n");
>>> + break;
>>
>> ... here.
>>
>
> Are we suggesting moving
> "
> if ( !IS_ENABLED(CONFIG_AMD) )
> {
> ret = -ENODEV;
> break;
> }
> " here?
That's what I said, didn't I?
> In which case, When CONFIG_AMD=n and users doesn't provide "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen. powernow_register_driver() hence gets invoked. The thing is that we don't have stub for it and it is compiled under CONFIG_AMD
> I suggest to change to use #ifdef CONFIG_AMD code wrapping
Perhaps necessary, yes. As you know, we generally prefer IS_ENABLED() where possible,
but when not possible, #ifdef is certainly okay to use.
Jan
On 04.08.2025 10:09, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, July 17, 2025 12:00 AM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
>> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline
>> and amd-cppc driver
>>
>> On 11.07.2025 05:50, Penny Zheng wrote:
>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> @@ -128,12 +128,14 @@ static int __init cf_check
>>> cpufreq_driver_init(void)
>>>
>>> if ( cpufreq_controller == FREQCTL_xen )
>>> {
>>> + unsigned int i = 0;
>>
>> Pointless initializer; both for() loops set i to 0. But also see further down.
>>
>>> @@ -157,9 +164,70 @@ static int __init cf_check
>>> cpufreq_driver_init(void)
>>>
>>> case X86_VENDOR_AMD:
>>> case X86_VENDOR_HYGON:
>>> - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
>> ENODEV;
>>> + if ( !IS_ENABLED(CONFIG_AMD) )
>>> + {
>>> + ret = -ENODEV;
>>> + break;
>>> + }
>>> + ret = -ENOENT;
>>
>> The code structure is sufficiently different from the Intel counterpart for this to
>> perhaps better move ...
>>
>>> + for ( i = 0; i < cpufreq_xen_cnt; i++ )
>>> + {
>>> + switch ( cpufreq_xen_opts[i] )
>>> + {
>>> + case CPUFREQ_xen:
>>> + ret = powernow_register_driver();
>>> + break;
>>> +
>>> + case CPUFREQ_amd_cppc:
>>> + ret = amd_cppc_register_driver();
>>> + break;
>>> +
>>> + case CPUFREQ_none:
>>> + ret = 0;
>>> + break;
>>> +
>>> + default:
>>> + printk(XENLOG_WARNING
>>> + "Unsupported cpufreq driver for vendor AMD or Hygon\n");
>>> + break;
>>
>> ... here.
>>
>
> Are we suggesting moving
> "
> if ( !IS_ENABLED(CONFIG_AMD) )
> {
> ret = -ENODEV;
> break;
> }
> " here? In which case, When CONFIG_AMD=n and users doesn't provide "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen. powernow_register_driver() hence gets invoked. The thing is that we don't have stub for it and it is compiled under CONFIG_AMD
> I suggest to change to use #ifdef CONFIG_AMD code wrapping
>
>>> + }
>>> +
>>> + if ( !ret || ret == -EBUSY )
>>> + break;
>>> + }
>>> +
>>> break;
>>> }
>>> +
>>> + /*
>>> + * After successful cpufreq driver registeration,
>> XEN_PROCESSOR_PM_CPPC
>>> + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
>>> + */
>>> + if ( !ret )
>>> + {
>>> + ASSERT(i < cpufreq_xen_cnt);
>>> + switch ( cpufreq_xen_opts[i] )
>>
>> Hmm, this is using the the initializer of i that I commented on. I think there's
>> another default: case missing, where you simply "return 0" (to retain prior behavior).
>> But again see also yet further down.
>>
>>
>>> + /*
>>> + * No cpufreq driver gets registered, clear both
>>> + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
>>> + */
>>> + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
>>> + XEN_PROCESSOR_PM_PX);
>>
>> Yet more hmm - this path you want to get through for the case mentioned above.
>> But only this code; specifically not the "switch ( cpufreq_xen_opts[i] )", which really
>> is "switch ( cpufreq_xen_opts[0] )" in that case, and that's pretty clearly wrong to
>> evaluate in then.
>
> Correct me if I understand you wrongly:
> The above "case missing" , are we talking about is entering "case CPUFREQ_none" ?
> IMO, it may never be entered. If users doesn't provide "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen. That is, we will have px states as default driver. Even if we have failed px-driver initialization, with cpufreq_xen_cnt limited to 1, we will not enter CPUFREQ_none.
> CPUFREQ_none only could be set when users explicitly set "cpufreq=disabled/none/0", but in which case, cpufreq_controller will be set with FREQCTL_none. And the whole cpufreq_driver_init() is under " cpufreq_controller == FREQCTL_xen " condition
> Or "case missing" is referring entering default case? In which case, we will have -ENOENT errno. As we have ret=-ENOENT in the very beginning
Sorry, this is hard to follow. Plus I think I made the main requirement quite
clear: You want to "retain prior behavior" for all cases you don't deliberately
change to accommodate the new driver. Plus you want to watch out for pre-
existing incorrect behavior: Rather than proliferating any, such would want
adjusting.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, August 4, 2025 4:48 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline
> and amd-cppc driver
>
> On 04.08.2025 10:09, Penny, Zheng wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Thursday, July 17, 2025 12:00 AM
> >> To: Penny, Zheng <penny.zheng@amd.com>
> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Anthony PERARD
> >> <anthony.perard@vates.tech>; Orzel, Michal <Michal.Orzel@amd.com>;
> >> Julien Grall <julien@xen.org>; Roger Pau Monné
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> >> xen- devel@lists.xenproject.org
> >> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc"
> >> xen cmdline and amd-cppc driver
> >>
> >> On 11.07.2025 05:50, Penny Zheng wrote:
> >>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >>> @@ -128,12 +128,14 @@ static int __init cf_check
> >>> cpufreq_driver_init(void)
> >>>
> >>> if ( cpufreq_controller == FREQCTL_xen )
> >>> {
> >>> + unsigned int i = 0;
> >>
> >> Pointless initializer; both for() loops set i to 0. But also see further down.
> >>
> >>> @@ -157,9 +164,70 @@ static int __init cf_check
> >>> cpufreq_driver_init(void)
> >>>
> >>> case X86_VENDOR_AMD:
> >>> case X86_VENDOR_HYGON:
> >>> - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
> >> ENODEV;
> >>> + if ( !IS_ENABLED(CONFIG_AMD) )
> >>> + {
> >>> + ret = -ENODEV;
> >>> + break;
> >>> + }
> >>> + ret = -ENOENT;
> >>
> >> The code structure is sufficiently different from the Intel
> >> counterpart for this to perhaps better move ...
> >>
> >>> + for ( i = 0; i < cpufreq_xen_cnt; i++ )
> >>> + {
> >>> + switch ( cpufreq_xen_opts[i] )
> >>> + {
> >>> + case CPUFREQ_xen:
> >>> + ret = powernow_register_driver();
> >>> + break;
> >>> +
> >>> + case CPUFREQ_amd_cppc:
> >>> + ret = amd_cppc_register_driver();
> >>> + break;
> >>> +
> >>> + case CPUFREQ_none:
> >>> + ret = 0;
> >>> + break;
> >>> +
> >>> + default:
> >>> + printk(XENLOG_WARNING
> >>> + "Unsupported cpufreq driver for vendor AMD or Hygon\n");
> >>> + break;
> >>
> >> ... here.
> >>
> >
> > Are we suggesting moving
> > "
> > if ( !IS_ENABLED(CONFIG_AMD) )
> > {
> > ret = -ENODEV;
> > break;
> > }
> > " here? In which case, When CONFIG_AMD=n and users doesn't provide
> > "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and
> > cpufreq_xen_opts[0] = CPUFREQ_xen. powernow_register_driver() hence
> > gets invoked. The thing is that we don't have stub for it and it is
> > compiled under CONFIG_AMD I suggest to change to use #ifdef CONFIG_AMD
> > code wrapping
> >
> >>> + }
> >>> +
> >>> + if ( !ret || ret == -EBUSY )
> >>> + break;
> >>> + }
> >>> +
> >>> break;
> >>> }
> >>> +
> >>> + /*
> >>> + * After successful cpufreq driver registeration,
> >> XEN_PROCESSOR_PM_CPPC
> >>> + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
> >>> + */
> >>> + if ( !ret )
> >>> + {
> >>> + ASSERT(i < cpufreq_xen_cnt);
> >>> + switch ( cpufreq_xen_opts[i] )
> >>
> >> Hmm, this is using the the initializer of i that I commented on. I
> >> think there's another default: case missing, where you simply "return 0" (to
> retain prior behavior).
> >> But again see also yet further down.
> >>
> >>
> >>> + /*
> >>> + * No cpufreq driver gets registered, clear both
> >>> + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
> >>> + */
> >>> + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
> >>> + XEN_PROCESSOR_PM_PX);
> >>
> >> Yet more hmm - this path you want to get through for the case mentioned above.
> >> But only this code; specifically not the "switch (
> >> cpufreq_xen_opts[i] )", which really is "switch ( cpufreq_xen_opts[0]
> >> )" in that case, and that's pretty clearly wrong to evaluate in then.
> >
> > Correct me if I understand you wrongly:
> > The above "case missing" , are we talking about is entering "case
> CPUFREQ_none" ?
> > IMO, it may never be entered. If users doesn't provide "cpufreq=xxx", we will
> have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen.
> That is, we will have px states as default driver. Even if we have failed px-driver
> initialization, with cpufreq_xen_cnt limited to 1, we will not enter CPUFREQ_none.
> > CPUFREQ_none only could be set when users explicitly set
> > "cpufreq=disabled/none/0", but in which case, cpufreq_controller will
> > be set with FREQCTL_none. And the whole cpufreq_driver_init() is under
> > " cpufreq_controller == FREQCTL_xen " condition Or "case missing" is
> > referring entering default case? In which case, we will have -ENOENT
> > errno. As we have ret=-ENOENT in the very beginning
>
> Sorry, this is hard to follow. Plus I think I made the main requirement quite
> clear: You want to "retain prior behavior" for all cases you don't deliberately change
> to accommodate the new driver. Plus you want to watch out for pre- existing
> incorrect behavior: Rather than proliferating any, such would want adjusting.
>
I was trying to follow "there's another default: case missing, where you simply "return 0" (to retain prior behavior ) ",
The missing "default :" is referring the one for "switch ( boot_cpu_data.x86_vendor )"? (I thought it referred " switch ( cpufreq_xen_opts[i] ) " ....)
It is a pre- existing incorrect behavior which I shall create a new commit to fix it firstly
I'll add an -ENOENTRY initializer for ret at the very beginning , and complement the missing default: entry with "Unsupported vendor..." error log
> Jan
On 05.08.2025 08:31, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, August 4, 2025 4:48 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
>> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline
>> and amd-cppc driver
>>
>> On 04.08.2025 10:09, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Thursday, July 17, 2025 12:00 AM
>>>> To: Penny, Zheng <penny.zheng@amd.com>
>>>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; Anthony PERARD
>>>> <anthony.perard@vates.tech>; Orzel, Michal <Michal.Orzel@amd.com>;
>>>> Julien Grall <julien@xen.org>; Roger Pau Monné
>>>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
>>>> xen- devel@lists.xenproject.org
>>>> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc"
>>>> xen cmdline and amd-cppc driver
>>>>
>>>> On 11.07.2025 05:50, Penny Zheng wrote:
>>>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>>>> @@ -128,12 +128,14 @@ static int __init cf_check
>>>>> cpufreq_driver_init(void)
>>>>>
>>>>> if ( cpufreq_controller == FREQCTL_xen )
>>>>> {
>>>>> + unsigned int i = 0;
>>>>
>>>> Pointless initializer; both for() loops set i to 0. But also see further down.
>>>>
>>>>> @@ -157,9 +164,70 @@ static int __init cf_check
>>>>> cpufreq_driver_init(void)
>>>>>
>>>>> case X86_VENDOR_AMD:
>>>>> case X86_VENDOR_HYGON:
>>>>> - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
>>>> ENODEV;
>>>>> + if ( !IS_ENABLED(CONFIG_AMD) )
>>>>> + {
>>>>> + ret = -ENODEV;
>>>>> + break;
>>>>> + }
>>>>> + ret = -ENOENT;
>>>>
>>>> The code structure is sufficiently different from the Intel
>>>> counterpart for this to perhaps better move ...
>>>>
>>>>> + for ( i = 0; i < cpufreq_xen_cnt; i++ )
>>>>> + {
>>>>> + switch ( cpufreq_xen_opts[i] )
>>>>> + {
>>>>> + case CPUFREQ_xen:
>>>>> + ret = powernow_register_driver();
>>>>> + break;
>>>>> +
>>>>> + case CPUFREQ_amd_cppc:
>>>>> + ret = amd_cppc_register_driver();
>>>>> + break;
>>>>> +
>>>>> + case CPUFREQ_none:
>>>>> + ret = 0;
>>>>> + break;
>>>>> +
>>>>> + default:
>>>>> + printk(XENLOG_WARNING
>>>>> + "Unsupported cpufreq driver for vendor AMD or Hygon\n");
>>>>> + break;
>>>>
>>>> ... here.
>>>>
>>>
>>> Are we suggesting moving
>>> "
>>> if ( !IS_ENABLED(CONFIG_AMD) )
>>> {
>>> ret = -ENODEV;
>>> break;
>>> }
>>> " here? In which case, When CONFIG_AMD=n and users doesn't provide
>>> "cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and
>>> cpufreq_xen_opts[0] = CPUFREQ_xen. powernow_register_driver() hence
>>> gets invoked. The thing is that we don't have stub for it and it is
>>> compiled under CONFIG_AMD I suggest to change to use #ifdef CONFIG_AMD
>>> code wrapping
>>>
>>>>> + }
>>>>> +
>>>>> + if ( !ret || ret == -EBUSY )
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> break;
>>>>> }
>>>>> +
>>>>> + /*
>>>>> + * After successful cpufreq driver registeration,
>>>> XEN_PROCESSOR_PM_CPPC
>>>>> + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
>>>>> + */
>>>>> + if ( !ret )
>>>>> + {
>>>>> + ASSERT(i < cpufreq_xen_cnt);
>>>>> + switch ( cpufreq_xen_opts[i] )
>>>>
>>>> Hmm, this is using the the initializer of i that I commented on. I
>>>> think there's another default: case missing, where you simply "return 0" (to
>> retain prior behavior).
>>>> But again see also yet further down.
>>>>
>>>>
>>>>> + /*
>>>>> + * No cpufreq driver gets registered, clear both
>>>>> + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
>>>>> + */
>>>>> + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
>>>>> + XEN_PROCESSOR_PM_PX);
>>>>
>>>> Yet more hmm - this path you want to get through for the case mentioned above.
>>>> But only this code; specifically not the "switch (
>>>> cpufreq_xen_opts[i] )", which really is "switch ( cpufreq_xen_opts[0]
>>>> )" in that case, and that's pretty clearly wrong to evaluate in then.
>>>
>>> Correct me if I understand you wrongly:
>>> The above "case missing" , are we talking about is entering "case
>> CPUFREQ_none" ?
>>> IMO, it may never be entered. If users doesn't provide "cpufreq=xxx", we will
>> have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen.
>> That is, we will have px states as default driver. Even if we have failed px-driver
>> initialization, with cpufreq_xen_cnt limited to 1, we will not enter CPUFREQ_none.
>>> CPUFREQ_none only could be set when users explicitly set
>>> "cpufreq=disabled/none/0", but in which case, cpufreq_controller will
>>> be set with FREQCTL_none. And the whole cpufreq_driver_init() is under
>>> " cpufreq_controller == FREQCTL_xen " condition Or "case missing" is
>>> referring entering default case? In which case, we will have -ENOENT
>>> errno. As we have ret=-ENOENT in the very beginning
>>
>> Sorry, this is hard to follow. Plus I think I made the main requirement quite
>> clear: You want to "retain prior behavior" for all cases you don't deliberately change
>> to accommodate the new driver. Plus you want to watch out for pre- existing
>> incorrect behavior: Rather than proliferating any, such would want adjusting.
>>
>
> I was trying to follow "there's another default: case missing, where you simply "return 0" (to retain prior behavior ) ",
> The missing "default :" is referring the one for "switch ( boot_cpu_data.x86_vendor )"? (I thought it referred " switch ( cpufreq_xen_opts[i] ) " ....)
> It is a pre- existing incorrect behavior which I shall create a new commit to fix it firstly
> I'll add an -ENOENTRY initializer for ret at the very beginning , and complement the missing default: entry with "Unsupported vendor..." error log
Yes, I was referring to pre-existing code which I think wants adjusting in
order to then accommodate your changes there.
Jan
© 2016 - 2025 Red Hat, Inc.