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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

HI

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, April 28, 2025 11:57 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 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