RE: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, February 17, 2025 6:34 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> <Jason.Andryuk@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 v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen 
> cmdline
>
> On 17.02.2025 11:17, Penny, Zheng wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Tuesday, February 11, 2025 8:09 PM
> >> To: Penny, Zheng <penny.zheng@xxxxxxx>
> >> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> >> <Jason.Andryuk@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 v2 03/11] xen/x86: introduce "cpufreq=amd-cppc"
> >> xen cmdline
> >>
> >> On 06.02.2025 09:32, Penny Zheng wrote:
> >>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> >>> @@ -148,6 +148,9 @@ static int __init cf_check cpufreq_driver_init(void)
> >>>                  case CPUFREQ_none:
> >>>                      ret = 0;
> >>>                      break;
> >>> +                default:
> >>> +                    printk(XENLOG_WARNING "Unsupported cpufreq
> >>> + driver for vendor Intel\n");
> >>
> >> Same here. The string along overruning line length is fine. But this
> >> cal then still be wrapped as
> >>
> >>                     printk(XENLOG_WARNING
> >>                            "Unsupported cpufreq driver for vendor
> >> Intel\n");
> >>
> >> to respect the line length limit as much as possible.
> >>
> >>> @@ -131,6 +131,15 @@ static int __init cf_check
> >>> setup_cpufreq_option(const
> >> char *str)
> >>>              if ( arg[0] && arg[1] )
> >>>                  ret = hwp_cmdline_parse(arg + 1, end);
> >>>          }
> >>> +        else if ( choice < 0 && !cmdline_strcmp(str, "amd-cppc") )
> >>> +        {
> >>> +            xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC;
> >>> +            cpufreq_controller = FREQCTL_xen;
> >>> +            cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_amd_cppc;
> >>
> >> While apparently again a pre-existing problem, the risk of array
> >> overrun will become more manifest with this addition: People may
> >> plausibly want to pass a universal option to Xen on all their instances:
> >> "cpufreq=hwp,amd-cppc,xen". I think this wants taking care of in a prereq 
> >> patch,
> i.e.
> >> before you further extend it. Here you will then further want to bump
> >> cpufreq_xen_opts[]'es dimension, to account for the now sensible three-fold
> option.
> >>
> >
> > Correct me if I'm wrong, We are missing dealing the scenario which looks 
> > like the
> following:
> > "cpufreq=amd-cppc,hwp,verbose".
>
> Not so much this one (can it even overflow). It's "cpufreq=amd-cppc,hwp,xen"
> that I'm concerned about (or, prior to your change something redundant like
> "cpufreq=hwp,hwp,xen").

I misunderstood before, sorry
What is the appropriate behavior when user passes an option to Xen, like 
"cpufreq=amd-cppc,hwp,xen" ?
FWIT, amd-cppc and hwp are incompatible options.
Send the error info to tell them you shall choose either of them, amd-cppc, or 
hwp, or xen?
If user wants to add fall back scheme, when amd-cppc is hardware unavailable, 
we fall back to xen. user shall
use ";", not "," to add, like "cpufreq=amd-cppc;xen"

>
> > `verbose` is an overrun flag when parsing amd-cppc.
> > I've written the following fix:
> > ```
> > diff --git a/xen/drivers/cpufreq/cpufreq.c
> > b/xen/drivers/cpufreq/cpufreq.c index 860ae32c8a..0fe633d4bc 100644
> > --- a/xen/drivers/cpufreq/cpufreq.c
> > +++ b/xen/drivers/cpufreq/cpufreq.c
> > @@ -71,6 +71,22 @@ unsigned int __initdata cpufreq_xen_cnt = 1;
> >
> >  static int __init cpufreq_cmdline_parse(const char *s, const char
> > *e);
> >
> > +static int __initdata nr_cpufreq_avail_opts = 3; static const char *
> > +__initdata cpufreq_avail_opts[nr_cpufreq_avail_opts] = { "xen",
> > +"hwp", "amd-cppc" };
>
> Does this even build? If it does, it likely won't be what you mean. You want a
> constant array dimension (which could actually be omitted, given the 
> initializer),
> as ...
>
> > +static void __init cpufreq_cmdline_trim(const char *s) {
> > +    unsigned int i = 0;
> > +
> > +    do
> > +    {
> > +        if ( strncmp(s, cpufreq_avail_opts[i], 
> > strlen(cpufreq_avail_opts[i] - 1) == 0 )
> > +            return false;
> > +    } while ( ++i < nr_cpufreq_avail_opts )
>
> ... you can use ARRAY_SIZE() here. (Style note: "do {" goes on a single line.)
>
> > +
> > +    return true;
> > +}
> > +
> >  static int __init cf_check setup_cpufreq_option(const char *str)  {
> >      const char *arg = strpbrk(str, ",:;"); @@ -118,7 +134,7 @@ static
> > int __init cf_check setup_cpufreq_option(const char *str)
> >              cpufreq_controller = FREQCTL_xen;
> >              cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
> >              ret = 0;
> > -            if ( arg[0] && arg[1] )
> > +            if ( arg[0] && arg[1] && cpufreq_cmdline_trim(arg + 1) )
> >                  ret = cpufreq_cmdline_parse(arg + 1, end);
> >          }
> >          else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 && ```
>
> For the rest of this, I guess I'd prefer to see this in context. Also with 
> regard to the
> helper function's name.
>
> Jan