[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, April 28, 2025 11:57 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>;
> Anthony PERARD <anthony.per...@vates.tech>; Orzel, Michal
> <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini
> <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 03/15] xen/x86: introduce new sub-hypercall to
> propagate
> CPPC data
>
> On 14.04.2025 09:40, Penny Zheng wrote:
> > @@ -459,6 +464,26 @@ struct xen_processor_performance { typedef
> > struct xen_processor_performance xen_processor_performance_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
> >
> > +struct xen_processor_cppc {
> > + uint8_t flags; /* flag for CPPC sub info type */
> > + /*
> > + * Subset _CPC fields useful for CPPC-compatible cpufreq
> > + * driver's initialization
> > + */
> > + struct {
> > + uint32_t highest_perf;
> > + uint32_t nominal_perf;
> > + uint32_t lowest_nonlinear_perf;
> > + uint32_t lowest_perf;
> > + uint32_t lowest_mhz;
> > + uint32_t nominal_mhz;
> > + } cpc;
> > + struct xen_psd_package domain_info; /* _PSD */
>
> This being a member of the new type, ...
>
> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -168,6 +168,7 @@
> > ! processor_performance platform.h
> > ! processor_power platform.h
> > ? processor_px platform.h
> > +? processor_cppc platform.h
>
> ... how can it be ? here when it's ...
>
> > ! psd_package platform.h
>
> ... ! here? And with it being ?, you're lacking a place where you invoke the
> resulting
> checking macro (which I assume would cause a build failure).
>
> Also when laying out struct xen_processor_cppc, please avoid unnecessary gaps
> or tail padding - it looks like "shared_type" would better move up. I think
> it would
> also be a good idea to make padding fields explicit, and check them to be
> zero.
> This way they can be assigned meaning later (if need
> be) without breaking backwards compatibility.
>
Understood, I'll re-construct into increasing order and add explicit padding:
```
struct xen_processor_cppc {
uint8_t flags; /* flag for CPPC sub info type */
uint8_t pad[3]; /* padding and must be zero */
/*
* Subset _CPC fields useful for CPPC-compatible cpufreq
* driver's initialization
*/
struct {
uint32_t highest_perf;
uint32_t nominal_perf;
uint32_t lowest_nonlinear_perf;
uint32_t lowest_perf;
uint32_t lowest_mhz;
uint32_t nominal_mhz;
} cpc;
/* Coordination type of this processor */
uint32_t shared_type;
struct xen_psd_package domain_info; /* _PSD */
};
```
> Jan
