RE: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline and amd-cppc driver

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, July 17, 2025 12:00 AM
> 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 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