Re: [PATCH v3 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2025 09:35, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, March 24, 2025 11:26 PM
>>
>> On 06.03.2025 09:39, Penny Zheng wrote:
>>> @@ -514,5 +515,14 @@ acpi_cpufreq_driver = {
>>>
>>>  int __init acpi_cpufreq_register(void)  {
>>> -    return cpufreq_register_driver(&acpi_cpufreq_driver);
>>> +    int ret;
>>> +
>>> +    ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>>> +    if ( ret )
>>> +        return ret;
>>> +
>>> +    if ( IS_ENABLED(CONFIG_AMD) )
>>> +        xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
>>
>> What's the purpose of the if() here?
> 
> After cpufreq driver properly registered, I'd like XEN_PROCESSOR_PM_PX and 
> XEN_PROCESSOR_PM_CPPC
> being exclusive value to represent the actual underlying registered driver.
> As users could define something like "cpufreq=amd-cppc,xen", which implies 
> both XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC
> got set in parsing logic. With amd-cppc failing to register, we are falling 
> back to legacy ones. Then XEN_PROCESSOR_PM_CPPC needs to clear.

Looks like you try to explain the &= when my question was about the if().
I understand the purpose of the &=. What I don't understand is why it needs
to be conditional.

>>> --- a/xen/include/acpi/cpufreq/cpufreq.h
>>> +++ b/xen/include/acpi/cpufreq/cpufreq.h
>>> @@ -28,6 +28,7 @@ enum cpufreq_xen_opt {
>>>      CPUFREQ_none,
>>>      CPUFREQ_xen,
>>>      CPUFREQ_hwp,
>>> +    CPUFREQ_amd_cppc,
>>>  };
>>>  extern enum cpufreq_xen_opt cpufreq_xen_opts[2];
>>
>> I'm pretty sure I pointed out before that this array needs to grow, now that 
>> you add a
>> 3rd kind of handling.
>>
> 
> Hmmm, but the CPUFREQ_hwp and CPUFREQ_amd_cppc are incompatible options.
> I thought cpufreq_xen_opts[] shall reflect available choices on their 
> hardware.
> Even if users define "cpufreq=hwp;amd-cppc;xen", in Intel platform, 
> cpufreq_xen_opts[] shall
> contain  CPUFREQ_hwp and CPUFREQ_xen, while in amd platform, 
> cpufreq_xen_opts[] shall
> contain CPUFREQ_amd_cppc and CPUFREQ_xen

Maybe I misread the code, but the impression I got was that 
"cpufreq=hwp;amd-cppc;xen"
would populate 3 slots of the array (with one of "hwp" and "amd-cppc" 
necessarily not
working, leading to the next one to be tried).

Jan