RE: [PATCH v4 03/15] xen/x86: introduce new sub-hypercall to propagate CPPC data

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, April 28, 2025 11:57 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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