On 04.08.2025 08:47, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Wednesday, July 16, 2025 11:39 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 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
