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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.04.2022 17:51, Roger Pau Monné wrote:
> 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.

Oh, yes, sorry.

>  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.

I agree, but we can't do anything about this now.

Jan