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: Tuesday, February 18, 2025 10:56 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 18.02.2025 05:24, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Monday, February 17, 2025 6:34 PM
> >>
> >> On 17.02.2025 11:17, Penny, Zheng wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> Sent: Tuesday, February 11, 2025 8:09 PM
> >>>>
> >>>> On 06.02.2025 09:32, Penny Zheng wrote:
> >>>>> @@ -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.
>
> Sure, but as said people may want to use something like this uniformly on all 
> their
> systems, be them AMD or Intel ones. IOW ...
>
> > Send the error info to tell them you shall choose either of them, amd-cppc, 
> > or hwp,
> or xen?
>
> ... no, I don't think this should be an error.
>
> > 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"
>
> Well, I didn't closely check whether the separator is to be semicolon or 
> comma.
> Things is that people may want to use one single command line option across 
> all
> their systems, old or new, Intel or AMD. Hence they may want to ask to use 
> HWP is
> available, CPPC is available, or fall back to what we have had for ages. Yet 
> they will
> also need to have a way to express that they want only HWP and CPPC to be 
> tried,
> without falling back to the legacy driver. Hence we may not automatically 
> fall back to
> that if "amp-cppc" was passed, but is unavailable.
>

Then I suggest we use the semicolon to separate all options user would like to 
try, but in the
priority order, like "cpufreq=hwp;amd-cppc;xen", if hwp and amd-cppc are both 
tried and found
not supported,legacy xen will be considered.
If it's only "cpufreq=hwp;amd-cppc", and when hwp and amd-cppc are both not 
supported, we
will not automatically fall back to any.

For specific driver, like "amd-cppc", sub-features like active mode will be 
separated by ",", like
"cpufreq=hwp;amd-cppc,active;xen"

> Jan