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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan