RE: [PATCH v5 06/18] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, June 12, 2025 6:42 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 v5 06/18] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
>
> On 27.05.2025 10:48, Penny Zheng wrote:
> > 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 will be implemented in the ongoing commits.
> >
> > New xen-pm internal flag XEN_PROCESSOR_PM_CPPC is introduced, to be
> > differentiated with legacy XEN_PROCESSOR_PM_PX. We define
> > XEN_PROCESSOR_PM_CPPC 0x100, as it is the next value to use after
> > 8-bits wide public xen-pm options. All xen-pm flag checking involving
> > XEN_PROCESSOR_PM_PX shall also be updated to consider
> XEN_PROCESSOR_PM_CPPC now.
> >
> > Xen is not expected to support both or mixed mode (CPPC & legacy PSS,
> > _PCT,
> > _PPC) operations, so only one cpufreq driver gets registerd, either
> > amd-cppc or legacy P-states driver, which is reflected and asserted by
> > the incompatible flags XEN_PROCESSOR_PM_PX and
> XEN_PROCESSOR_PM_CPPC.
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> > ---
> > 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.
> > ---
> >  docs/misc/xen-command-line.pandoc         |  7 ++-
> >  xen/arch/x86/acpi/cpufreq/Makefile        |  1 +
> >  xen/arch/x86/acpi/cpufreq/amd-cppc.c      | 68 +++++++++++++++++++++++
> >  xen/arch/x86/acpi/cpufreq/cpufreq.c       | 63 ++++++++++++++++++++-
> >  xen/arch/x86/platform_hypercall.c         | 13 ++++-
> >  xen/drivers/acpi/pmstat.c                 |  3 +-
> >  xen/drivers/cpufreq/cpufreq.c             | 18 +++++-
> >  xen/include/acpi/cpufreq/cpufreq.h        |  6 +-
> >  xen/include/acpi/cpufreq/processor_perf.h |  3 +
> >  xen/include/public/sysctl.h               |  1 +
> >  10 files changed, 175 insertions(+), 8 deletions(-)  create mode
> > 100644 xen/arch/x86/acpi/cpufreq/amd-cppc.c
>
> > @@ -157,7 +162,63 @@ 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;
> > +            unsigned int i;
> > +
> > +            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 != -ENODEV )
> > +                    break;
>
> This, I think, needs some commenting. It's not quite clear why we shouldn't 
> try the
> next option if registration failed with other than -ENODEV.
>

I followed the original logic.
Now, I'm trying to understand the reason. I read the related code, there are 
two code path erroring out other than -ENODEV
In cpufreq_register_driver(), either the driver itself is broken, like missing 
mandatory hooks, etc, yet in which case, IMO we shall try the fallback option,
or repeated registration, TBH, which seems unlikely to me. 
cpufreq_driver_init() is a presmp call, so repeated registration doesn't come 
from racing.
Then if we successfully registered a driver, we will immediately exit the loop. 
How come we will register twice?
Or am I missing something for this error path:
```
        if ( cpufreq_driver.init )
                return -EBUSY;
```

>
> > +                    break;
> > +                }
> > +            }
>
> Why does this pruning of xen_processor_pmbits sit in the AMD-only code path?
> Iirc earlier on you had it in the *_register_driver() themselves, and my 
> request was
> to put it in a central, generic place. It's central now, but not generic (and 
> this way it
> could as well have remained in *_register_driver()).
>

True, before, I was wrongly assumed that XEN_PROCESSOR_PM_CPPC could only be 
initialized in AMD.
We could also provide "cpufreq=amd-cppc" in INTEL platform, yet under which 
scenario, we shall set ret with -ENODEV errno (right now is -ENOENT, need to 
fix) to keep looping and try the next option...
I’ll move it out of the AMD-only patch, and put it after the whole switch-case

>
> Jan