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: Monday, August 4, 2025 4:48 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 v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen 
> cmdline
> and amd-cppc driver
>
> On 04.08.2025 10:09, Penny, Zheng wrote:
> > [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
>
> Sorry, this is hard to follow. Plus I think I made the main requirement quite
> clear: You want to "retain prior behavior" for all cases you don't 
> deliberately change
> to accommodate the new driver. Plus you want to watch out for pre- existing
> incorrect behavior: Rather than proliferating any, such would want adjusting.
>

I was trying to follow "there's another default: case missing, where you simply 
"return 0" (to retain prior behavior ) ",
The missing "default :" is referring the one for "switch ( 
boot_cpu_data.x86_vendor )"? (I thought it referred " switch ( 
cpufreq_xen_opts[i] ) " ....)
It is a pre- existing incorrect behavior which I shall create a new commit to 
fix it firstly
I'll add an -ENOENTRY initializer for ret at the very beginning , and 
complement the missing default: entry with "Unsupported vendor..." error log

> Jan