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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, March 27, 2025 3:48 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
>
> 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.
>

Thanks for the detailed explanation, understood!

> Jan