Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 07:56, Penny, Zheng wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, April 28, 2025 11:32 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 v4 02/15] xen/cpufreq: extract _PSD info from "struct
>> xen_processor_performance"
>>
>> On 14.04.2025 09:40, Penny Zheng wrote:
>>> --- a/xen/include/public/platform.h
>>> +++ b/xen/include/public/platform.h
>>> @@ -440,6 +440,11 @@ struct xen_psd_package {
>>>      uint64_t num_processors;
>>>  };
>>>
>>> +/* Coordination type value */
>>> +#define XEN_CPUPERF_SHARED_TYPE_HW   1 /* HW does needed
>> coordination */
>>> +#define XEN_CPUPERF_SHARED_TYPE_ALL  2 /* All dependent CPUs
>> should
>>> +set freq */ #define XEN_CPUPERF_SHARED_TYPE_ANY  3 /* Freq can be
>> set
>>> +from any dependent CPU */
>>> +
>>>  struct xen_processor_performance {
>>>      uint32_t flags;     /* flag for Px sub info type */
>>>      uint32_t platform_limit;  /* Platform limitation on freq usage */
>>> @@ -449,10 +454,7 @@ struct xen_processor_performance {
>>>      XEN_GUEST_HANDLE(xen_processor_px_t) states;
>>>      struct xen_psd_package domain_info;
>>>      /* Coordination type of this processor */
>>> -#define XEN_CPUPERF_SHARED_TYPE_HW   1 /* HW does needed
>> coordination */
>>> -#define XEN_CPUPERF_SHARED_TYPE_ALL  2 /* All dependent CPUs
>> should
>>> set freq */ -#define XEN_CPUPERF_SHARED_TYPE_ANY  3 /* Freq can be set
>> from any dependent CPU */
>>> -    uint32_t shared_type;
>>> +    uint32_t shared_type; /* XEN_CPUPERF_SHARED_TYPE_xxx */
>>>  };
>>>  typedef struct xen_processor_performance xen_processor_performance_t;
>>> DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
>>
>> What's this movement about? In the public interface nothing changes?
> 
> As we will have shared type in "struct xen_processor_cppc" too, maybe the 
> definition would like to live
> at more common place, then it could be shared?
> Living inside "struct xen_processor_performance" looks like internal values 
> for internal field.
> If it breaks the public interface some way, I'll change it back and duplicate 
> the definition in "struct xen_processor_cppc" too

I don't think it would break anything, but I also don't see any need for the
movement. And generally we prefer to avoid unnecessary code churn.

Jan