RE: [PATCH v3 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, March 24, 2025 11:26 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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