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

Understood, I see it automatically generates CHECK_psd_package. I shall change 
psd_package with ? too
In order to avoid causing build failure, I add "typedef struct xen_psd_package 
xen_psd_package_t;"
I'm not familiar with the compat framework, if it isn't the right way to fix, 
plz let me know

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