On 06.05.2025 11:11, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> 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
