RE: [PATCH v6 10/19] xen/cpufreq: introduce new sub-hypercall to propagate CPPC data

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, July 16, 2025 11:39 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 v6 10/19] xen/cpufreq: introduce new sub-hypercall to
> propagate CPPC data
>
> On 11.07.2025 05:50, Penny Zheng wrote:
> > +             cppc_data->cpc.nominal_perf > cppc_data->cpc.highest_perf )
> > +            /*
> > +             * Right now, Xen doesn't actually use 
> > highest_perf/nominal_perf/
> > +             * lowest_nonlinear_perf/lowest_perf values read from ACPI _CPC
> > +             * table. Xen reads CPPC capability MSR to get these four 
> > values.
> > +             * So warning is enough.
> > +             */
> > +            printk_once(XENLOG_WARNING
> > +                        "Broken CPPC perf values: lowest(%u), 
> > nonlinear_lowest(%u),
> nominal(%u), highest(%u)\n",
> > +                        cppc_data->cpc.lowest_perf,
> > +                        cppc_data->cpc.lowest_nonlinear_perf,
> > +                        cppc_data->cpc.nominal_perf,
> > +                        cppc_data->cpc.highest_perf);
> > +
> > +        /* lowest_mhz and nominal_mhz are optional value */
> > +        if ( cppc_data->cpc.lowest_mhz > cppc_data->cpc.nominal_mhz )
>
> If they're optional, what if lowest_mhz is provided but nominal_mhz isn't?
> Wouldn't the warning needlessly trigger in that case?
>

Yes, only both are provided, this check is meaningful
+        if ( cppc_data->cpc.nominal_mhz &&
+             cppc_data->cpc.lowest_mhz > cppc_data->cpc.nominal_mhz )

> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -363,6 +363,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
> >  #define XEN_PM_PX   1
> >  #define XEN_PM_TX   2
> >  #define XEN_PM_PDC  3
> > +#define XEN_PM_CPPC 4
> >
> >  /* Px sub info type */
> >  #define XEN_PX_PCT   1
> > @@ -370,6 +371,10 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
> >  #define XEN_PX_PPC   4
> >  #define XEN_PX_PSD   8
> >
> > +/* CPPC sub info type */
> > +#define XEN_CPPC_PSD   1
> > +#define XEN_CPPC_CPC   2
>
> As per this, ...
>
> > @@ -457,6 +462,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; /* IN: XEN_CPPC_xxx */
>
> ... it's a type that's living here, not a collection of flags. Any reason the 
> field isn't
> named "type"?
>

It is a collection of flags. Only when both XEN_CPPC_PSD and XEN_CPPC_CPC are 
set, we could run cpufreq_cpu_init() to initialize cpufreq core.

> > +    uint8_t pad[3];
> > +    /*
> > +     * IN: 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;
>
> What, again, was the reason to wrap these into a sub-struct?

I want to make these fields differentiated from the other two (shared_type and 
domain_info), as sub-struct cpc contains _CPC field info, and the other two 
contains _PSD info

>
> Jan