On 26.03.2025 08:20, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> 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
