Re: [PATCH] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 06, 2022 at 05:31:22PM +0200, Jan Beulich wrote:
> On 06.04.2022 17:16, Roger Pau Monne wrote:
> > The values set in the shared_type field of xen_processor_performance
> > have so far relied on Xen and Linux having the same
> > CPUFREQ_SHARED_TYPE_ defines, as those have never been part of the
> > public interface.
> > 
> > Formalize by adding the defines for the allowed values in the public
> > header, while renaming them to use the XEN_PERF_SHARED_TYPE_ prefix
> > for clarity.
> > 
> > Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > I wonder if we want to keep the CPUFREQ_SHARED_TYPE_ defines for
> > internal usage (and define them based on XEN_PERF_SHARED_TYPE_) in
> > case we need to pick up changes from Linux.
> 
> I think that would be desirable, also to limit code churn by this change.
> 
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -465,7 +465,11 @@ struct xen_processor_performance {
> >      uint32_t state_count;     /* total available performance states */
> >      XEN_GUEST_HANDLE(xen_processor_px_t) states;
> >      struct xen_psd_package domain_info;
> > -    uint32_t shared_type;     /* coordination type of this processor */
> > +    /* Coordination type of this processor */
> > +#define XEN_PERF_SHARED_TYPE_HW   1 /* HW does needed coordination */
> > +#define XEN_PERF_SHARED_TYPE_ALL  2 /* All dependent CPUs should set freq 
> > */
> > +#define XEN_PERF_SHARED_TYPE_ANY  3 /* Freq can be set from any dependent 
> > CPU */
> 
> While the names may then become a little long, I think it would be
> relevant to have "processor" (or maybe "CPU") in the names, to have
> a better match with struct xen_processor_performance. Also I'm not
> sure you want to define these inside the struct - they're
> supposedly suitable for Px, Cx, and Tx aiui (just like the underlying
> ACPI constants are).

But those defines are specific to CPUFREQ code, the raw values from
the ACPI tables for the different 'coordination' fields found in the
Cx and Px states use different values, ie:

#define ACPI_DOMAIN_COORD_TYPE_SW_ALL 0xfc
#define ACPI_DOMAIN_COORD_TYPE_SW_ANY 0xfd
#define ACPI_DOMAIN_COORD_TYPE_HW_ALL 0xfe

And hence the values exposed here should be limited to the existing
usage by the xen_processor_performance struct.  Otherwise it would be
preferred to use the native ACPI values, which don't need exposing in
the Xen headers then because are already part of a different
specification. IOW I think it was a mistake for the shared_type field
to use the CPUFREQ defines instead of the ACPI values.

Note the coord_type field in xen_psd_package and xen_processor_csd do
expect the ACPI values instead of the CPUFREQ ones.

Thanks, Roger.