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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2025 04:12, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, March 26, 2025 6:55 PM
>>
>> 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:
>>>>> --- 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"
> 
> My bad. In my platform, I haven't enabled the CONFIG_INTEL. I previously 
> assumed that
> CONFIG_INTEL and CONFIG_AMD are incompatible options, which leads to the 
> following code
> ```
> else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 &&
>           !cmdline_strcmp(str, "hwp") )
> {
>     xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
>     cpufreq_controller = FREQCTL_xen;
> ```
> shall not be working in AMD platform...
> May I ask why not make them incompatible pair? I assumed it each wraps 
> vendor-specific feature, like vmx vs svm...

I'm sorry to say this, but that seems like a pretty odd question to ask. Distros
quite clearly want to build one single hypervisor which can be used on both
Intel and AMD hardware. CONFIG_* are build-time constants after all, not runtime
values. We use them in if() where possible (instead of in #if / #ifdef) simply
to expose as much code as possible to at least syntax and alike checking by the
compiler, irrespective of configuration used by a particular individual. This
way we limit the risk of bit-rotting and unexpected build failures at least 
some.

Jan