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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2025 08:47, Penny, Zheng wrote:
> [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.

Hmm, right. The next legitimate XEN_CPPC_* value to use would be 4, not 3.
That's not visible from how things are defined, though. May I suggest that
you use

/* CPPC sub info type */
#define XEN_CPPC_PSD   (1U << 0)
#define XEN_CPPC_CPC   (1U << 1)

instead then?

Jan