[Public]
HI
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, April 28, 2025 11:57 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 v4 03/15] xen/x86: introduce new sub-hypercall to
> propagate
> CPPC data
>
> 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
> > + 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.
What we actually use is the following optional lowest_mhz and nominal_mhz. We
read
these two values for perf_to_khz transition.
There are two ways to read CPPC capability info, one is from MSR register, and
the other is from
_CPC table. Only in very few hardware, MSR is not supported, and we must switch
to use ACPI _CPC
Such scenario is not covered in this patch serie. I've mentioned it in the
cover letter.
The difficulty actually is that when we try to use ACPI _CPC to do CPPC
performance monitoring,
some control registers are probably implemented in the Platform Communications
Channel (PCC) interface, which
is not supported in Xen.
> > +
> > + 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?
> Jan
[1]
https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html?highlight=cppc#lowest-performance
