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

[Penny, Zheng]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, May 20, 2025 5:16 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-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
>
> On 20.05.2025 10:28, Penny, Zheng wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: Monday, May 19, 2025 9:19 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- de...@lists.xenproject.org
> >> Subject: Re: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc"
> >> xen cmdline
> >>
> >> On 19.05.2025 10:36, Penny, Zheng wrote:
> >>> [Public]
> >>>
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeul...@suse.com>
> >>>> Sent: Tuesday, April 29, 2025 8:52 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- de...@lists.xenproject.org
> >>>> Subject: Re: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc"
> >>>> xen cmdline
> >>>>
> >>>> On 14.04.2025 09:40, Penny Zheng wrote:
> >>>>> --- a/xen/include/acpi/cpufreq/processor_perf.h
> >>>>> +++ b/xen/include/acpi/cpufreq/processor_perf.h
> >>>>> @@ -5,6 +5,9 @@
> >>>>>  #include <public/sysctl.h>
> >>>>>  #include <xen/acpi.h>
> >>>>>
> >>>>> +/* ability bits */
> >>>>> +#define XEN_PROCESSOR_PM_CPPC   8
> >>>>
> >>>> This needs correlating (at least via commentary, better by
> >>>> build-time
> >>>> checking) with the other XEN_PROCESSOR_PM_* values. Otherwise
> >> someone
> >>>> adding a new #define in the public header may not (easily) notice a
> >>>> possible conflict. With that in mind I also question whether 8 is
> >>>> actually a good choice: That's the obvious next value to use in the
> >>>> public interface. SIF_PM_MASK is 8 bits wide, so a sensible value
> >>>> to use here
> >> would by e.g. 0x100.
> >>>>
> >>>
> >>> I've added a public flag anchor "XEN_PROCESSOR_PM_PUBLIC_END" in
> >> public header:
> >>>          #define XEN_PROCESSOR_PM_PUBLIC_END
> >> XEN_PROCESSOR_PM_TX
> >>> and will do the following build-time checking:
> >>>         BUILD_BUG_ON(XEN_PROCESSOR_PM_CPPC <=
> >>> XEN_PROCESSOR_PM_PUBLIC_END);
> >>
> >> I don't really see why anything would need to be added to the public
> >> header (and we really should strive to avoid unnecessary additions).
> >
> > If I put such definition
> > "#define XEN_PROCESSOR_PM_PUBLIC_END XEN_PROCESSOR_PM_TX"
> > in internal header, I'm afraid it won't be updated if new ones added in the 
> > public in
> the future.
> > Or any other suggestions to provide build-time checking?
>
> Imo it's sufficient to check that the new #define doesn't overlap with
> SIF_PM_MASK.

Understood!

>
> Jan