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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 11:11, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, April 28, 2025 11:57 PM
>>
>> On 14.04.2025 09:40, Penny Zheng wrote:
>>> +    if ( cppc_data->flags & XEN_CPPC_CPC )
>>> +    {
>>> +        if ( cppc_data->cpc.highest_perf == 0 ||
>>> +             cppc_data->cpc.highest_perf > UINT8_MAX ||
>>> +             cppc_data->cpc.nominal_perf == 0 ||
>>> +             cppc_data->cpc.nominal_perf > UINT8_MAX ||
>>> +             cppc_data->cpc.lowest_nonlinear_perf == 0 ||
>>> +             cppc_data->cpc.lowest_nonlinear_perf > UINT8_MAX ||
>>> +             cppc_data->cpc.lowest_perf == 0 ||
>>> +             cppc_data->cpc.lowest_perf > UINT8_MAX ||
>>> +             cppc_data->cpc.lowest_perf >
>>> +                cppc_data->cpc.lowest_nonlinear_perf ||
>>
>> Where's this ordering spelled out in the spec?
>>
> 
> Clip a snippet from description on lowest performance[1], we may deduce that
> ```
> Selecting a performance level lower than the lowest nonlinear performance 
> level may actually cause an efficiency penalty,
> but should reduce the instantaneous power consumption of the processor
> ```
> lowest is smaller than lowest nonlinear

I can't imply that from the quoted sentence. It describes what happens in that
situation, but it doesn't exclude the opposite relationship (in which case the
described situation simply can't occur).

>>> +             cppc_data->cpc.lowest_nonlinear_perf >
>>> +                cppc_data->cpc.nominal_perf ||
>>> +             cppc_data->cpc.nominal_perf > cppc_data->cpc.highest_perf )
>>> +            /*
>>> +             * Right now, Xen doesn't actually use perf values
>>> +             * in ACPI _CPC table, warning is enough.
>>> +             */
>>> +            printk(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);
>>
>> If this warning was to ever surface, it would likely surface for every CPU.
>> That's unnecessarily verbose, I guess. Please consider using printk_once() 
>> here.
>>
> 
> Understood
> 
>> Also, is "right now" (as the comment says) still going to be true by the end 
>> of the
>> series? Didn't I see you use the values in earlier versions?
>>
> 
> The reason why I added this comment is that in current implementation, we 
> actually
> don't use values read from ACPI _CPC table for lowest_perf, 
> lowest_nonlinear_perf,
> nominal_perf, and highest_perf.
> We read CPPC capability MSR to get these four values.

Oh, okay. Could you slightly extend that comment to include this detail?

>>> +    if ( cppc_data->flags == (XEN_CPPC_PSD | XEN_CPPC_CPC) )
>>
>> If either flag may be clear, ...
>>
>>> +    {
>>> +        pm_info->cppc_data = *cppc_data;
>>> +        if ( cpufreq_verbose )
>>> +        {
>>> +            print_PSD(&pm_info->cppc_data.domain_info);
>>> +            print_CPPC(&pm_info->cppc_data);
>>
>> ... why unconditionally loog both?
>>
>>> +        }
>>> +
>>> +        pm_info->init = XEN_CPPC_INIT;
>>
>> Plus is it correct to set this flag if either of the incoming flags was 
>> clear?
> 
> Hmm, I may not understand what you mean here...
> I log and set this flag only when both flags are set (cppc_data->flags == 
> (XEN_CPPC_PSD | XEN_CPPC_CPC))
> _PSD entry and _CPC entry are both mandatory
> Did you suggest that we shall give warning message when either flag is clear?

Oh, sorry - I read & where you have == actually. Hence why I thought only
one of the flags may be set. Please disregard those comments of mine.

Jan