[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, April 29, 2025 8:52 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger
> Pau Monné <roger....@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>;
> xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
>
> On 14.04.2025 09:40, Penny Zheng wrote:
> > @@ -514,5 +515,16 @@ acpi_cpufreq_driver = {
> >
> > int __init acpi_cpufreq_register(void) {
> > - return cpufreq_register_driver(&acpi_cpufreq_driver);
> > + int ret;
> > +
> > + ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > + if ( ret )
> > + return ret;
> > + /*
> > + * After cpufreq driver registeration, XEN_PROCESSOR_PM_CPPC
> > + * and XEN_PROCESSOR_PM_PX shall become exclusive flags
> > + */
> > + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
> > +
> > + return ret;
> > }
>
> Why is no similar adjustment needed in powernow_register_driver()? In
> principle I
> would have expected that it's not each individual driver which needs to care
> about
> this aspect, but that the framework is taking care of this.
>
Then maybe we shall add this here, to extract the codes from each specific
driver:
```
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c
b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index eac1c125a3..9276241291 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -190,6 +190,25 @@ static int __init cf_check cpufreq_driver_init(void)
if ( ret != -ENODEV )
break;
}
+
+ if ( !ret && i < cpufreq_xen_cnt )
+ {
+ switch ( cpufreq_xen_opts[i] )
+ {
+ case CPUFREQ_amd_cppc:
+ xen_processor_pmbits &= ~XEN_PROCESSOR_PM_XEN;
+ break;
+
+ case CPUFREQ_xen:
+ xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
+ break;
+
+ case CPUFREQ_none:
+ default:
+ break;
+ }
+ }
+
break;
}
}
```
> > @@ -573,6 +576,14 @@ ret_t do_platform_op(
> > }
> >
> > case XEN_PM_CPPC:
> > + if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) )
> > + {
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
>
> While at least you no longer use -ENOSYS here, I question this behavior,
> including
> that for the pre-existing cases: How is the caller supposed to know whether to
> invoke this sub-op? Ignoring errors is generally not a good idea, so it would
> be
> better if the caller could blindly issue this request, getting back success
> unless
> there really was an issue with the data provided.
>
Understood.
I will change it to ret = 0. Do you think we shall add warning info here?
Dom0 will send the CPPC data whenever it could. XEN_PROCESSOR_PM_CPPC
is not set could largely be users choosing not to. In such case, silently
getting back success
shall be enough.
> Jan
