[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, March 27, 2025 3:48 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger
> Pau Monné <roger....@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>;
> xen-devel@lists.xenproject.org
> 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 <jbeul...@suse.com>
> >> Sent: Wednesday, March 26, 2025 6:55 PM
> >>
> >> On 26.03.2025 09:35, Penny, Zheng wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeul...@suse.com>
> >>>> 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
