[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, March 24, 2025 11:26 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-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v3 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
>
> On 06.03.2025 09:39, Penny Zheng wrote:
> > @@ -514,5 +515,14 @@ 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;
> > +
> > + if ( IS_ENABLED(CONFIG_AMD) )
> > + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
>
> What's the purpose of the if() here?
After cpufreq driver properly registered, I'd like XEN_PROCESSOR_PM_PX and
XEN_PROCESSOR_PM_CPPC
being exclusive value to represent the actual underlying registered driver.
As users could define something like "cpufreq=amd-cppc,xen", which implies both
XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC
got set in parsing logic. With amd-cppc failing to register, we are falling
back to legacy ones. Then XEN_PROCESSOR_PM_CPPC needs to clear.
>
> > @@ -157,7 +161,35 @@ 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 ( unsigned int 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\n");
>
> What about Hygon?
>
> > --- a/xen/include/acpi/cpufreq/cpufreq.h
> > +++ b/xen/include/acpi/cpufreq/cpufreq.h
> > @@ -28,6 +28,7 @@ enum cpufreq_xen_opt {
> > CPUFREQ_none,
> > CPUFREQ_xen,
> > CPUFREQ_hwp,
> > + CPUFREQ_amd_cppc,
> > };
> > extern enum cpufreq_xen_opt cpufreq_xen_opts[2];
>
> I'm pretty sure I pointed out before that this array needs to grow, now that
> you add a
> 3rd kind of handling.
>
Hmmm, but the CPUFREQ_hwp and CPUFREQ_amd_cppc are incompatible options.
I thought cpufreq_xen_opts[] shall reflect available choices on their hardware.
Even if users define "cpufreq=hwp;amd-cppc;xen", in Intel platform,
cpufreq_xen_opts[] shall
contain CPUFREQ_hwp and CPUFREQ_xen, while in amd platform, cpufreq_xen_opts[]
shall
contain CPUFREQ_amd_cppc and CPUFREQ_xen
> Jan
