Re: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2025 08:20, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, March 24, 2025 11:01 PM
>>
>> On 06.03.2025 09:39, Penny Zheng wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported
>> by all Dom0 kernels.
>>>    processor to autonomously force physical package components into idle 
>>> state.
>>>    The default is enabled, but the option only applies when `hwp` is 
>>> enabled.
>>>
>>> -There is also support for `;`-separated fallback options:
>>> +User could use `;`-separated options to support universal options
>>> +which they would like to try on any agnostic platform, *but* under
>>> +priority order, like
>>>  `cpufreq=hwp;xen,verbose`.  This first tries `hwp` and falls back to
>>> `xen` if  unavailable.  Note: The `verbose` suboption is handled
>>> globally.  Setting it  for either the primary or fallback option
>>> applies to both irrespective of where
>>
>> What does "support" here mean? I fear I can't even suggest what else to use, 
>> as I
>> don't follow what additional information you mean to add here. Is a change 
>> here
>> really needed?
> 
> There are two changes I'd like to address:
> 1) ";" is not designed for fallback options anymore, like we discussed 
> before, we would
> like to support something like "cpufreq=hwp;amd-cppc;xen" for users to define 
> all universal options
> they would like to try.

Why would the meaning of ; change? There's no difference between having a single
fallback option from hwp, or two of them from amd-cppc.

> 2) Must under *priority* order. As in cpufreq_driver_init(), we are using 
> loop to decide which driver to
> try firstly. If user defines "cpufreq=xen;amd-cppc", which leads legacy 
> P-state set before amd-cppc in cpufreq_xen_opts[],
> then in the loop, we will try to register legacy P-state firstly, once it 
> gets registered successfully, we will not try to register amd-cppc at all.

This in-order aspect also doesn't change.

Overall I fear I don't feel my question was answered.

>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>> +    if ( arg[0] && arg[1] )
>>> +        ret = cpufreq_cmdline_parse(arg + 1, end);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int __init cpufreq_cmdline_parse_hwp(const char *arg, const
>>> +char *end) {
>>> +    int ret = 0;
>>> +
>>> +    xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
>>> +    cpufreq_controller = FREQCTL_xen;
>>> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
>>> +    if ( arg[0] && arg[1] )
>>> +        ret = hwp_cmdline_parse(arg + 1, end);
>>> +
>>> +    return ret;
>>> +}
>>
>> For both of the helpers may I suggest s/parse/process/ or some such ("handle"
>> might be another possible term to use), as themselves they don't do any 
>> parsing?
>>
> 
> Maybe I mis-understood the previous comment you said
> ```
>         >          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.
> ```
> I thought you suggested to introduce helper function to wrap the conditional 
> codes...
> Or may you were suggesting something like:
> ```
> #ifdef CONFIG_INTEL
> else if ( choice < 0 && !cmdline_strcmp(str, "hwp") )
> {
>     xen_processor_pmbits |= XEN_PROCES
>     ...
> }
> #endif
> ```

Was this reply of yours misplaced? It doesn't fit with the part of my reply in
context above. Or maybe I'm not understanding what you mean to say.

>> In the end I'm also not entirely convinced that we need these two almost 
>> identical
>> helpers (with a 3rd likely appearing in a later patch).

Instead it feels as if this response of yours was to this part of my comment.
Indeed iirc I was suggesting to introduce a helper function. Note, however, the
singular here as well as in your response above.

Jan