RE: [PATCH v2 05/11] xen/x86: introduce a new amd cppc driver for cpufreq scaling

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, February 12, 2025 12:46 AM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> <Jason.Andryuk@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 05/11] xen/x86: introduce a new amd cppc driver for
> cpufreq scaling
>
> On 06.02.2025 09:32, Penny Zheng wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> >
> > +/*
> > + * If CPPC lowest_freq and nominal_freq registers are exposed then we
> > +can
> > + * use them to convert perf to freq and vice versa. The conversion is
> > + * extrapolated as an affine function passing by the 2 points:
>
> Having studied maths, "affine function" isn't a term I know. There are affine
> transformations, but don't you simply mean "linear function" here? If so, is 
> it said
> anywhere in the spec that perf values grow linearly with freq ones?
>

Yes, "linear mapping" is better. And the spec reference is as follows:
```
The OS should use Lowest Frequency/Performance and Nominal Frequency/Performance
as anchor points to create a linear mapping of CPPC abstract performance to CPU 
frequency
```
See 8.4.6.1.1.7. Lowest Frequency and Nominal Frequency
 
(https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html?highlight=cppc#lowest-frequency-and-nominal-frequency
 )

> > + *  - (Low perf, Low freq)
> > + *  - (Nominal perf, Nominal freq)
> > + */
> > +static int amd_cppc_khz_to_perf(const struct amd_cppc_drv_data *data,
> > +unsigned int freq, uint8_t *perf)
>
> Overlong line again. Please sort throughout the series.
>
> > +{
> > +    const struct xen_processor_cppc *cppc_data = data->cppc_data;
> > +    uint64_t mul, div, offset = 0, res;
> > +
> > +    if ( freq == (cppc_data->nominal_freq * 1000) )
>
> There's no comment anywhere what the units of the values are. Therefore the
> multiplication by 1000 here leaves me wondering why consistent units aren't 
> used in
> the first place. (From the name of the function I might guess that "freq" is 
> in kHz,
> and then perhaps ->{min,max,nominal}_freq are in MHz.
> Then for the foreseeable future we're hopefully safe here wrt overflow.)
>

These conversion functions are designed in the first place for *ondemand* 
governor, which
reports performance as CPU frequencies. In generic governor->target() 
functions, we are always
take freq in KHz, but in CPPC ACPI spec, the frequency is read in Mhz from 
register...

> > +    {
> > +        *perf = data->caps.nominal_perf;
> > +        return 0;
> > +    }
> > +
> > +    if ( freq == (cppc_data->lowest_freq * 1000) )
> > +    {
> > +        *perf = data->caps.lowest_perf;
> > +        return 0;
> > +    }
> > +
> > +    if ( (cppc_data->lowest_freq) && (cppc_data->nominal_freq) )
>
> Why the inner parentheses?
>
> > +    {
> > +        mul = data->caps.nominal_perf - data->caps.lowest_perf;
> > +        div = cppc_data->nominal_freq - cppc_data->lowest_freq;
> > +        /*
> > +         * We don't need to convert to kHz for computing offset and can
> > +         * directly use nominal_freq and lowest_freq as the division
> > +         * will remove the frequency unit.
> > +         */
> > +        div = div ?: 1;
> > +        offset = data->caps.nominal_perf - (mul *
> > + cppc_data->nominal_freq) / div;
>
> I fear I can't convince myself that the subtraction can't ever underflow.
> With
>
> O = offset
> Pn = data->caps.nominal_perf
> Pl = data->caps.lowest_perf
> Fn = cppc_data->nominal_freq
> Fl = cppc_data->lowest_freq
>
> the above becomes
>
> O = Pn - ((Pn - Pl) * Fn) / (Fn - Fl)
>
> and your assumption is O >= 0 (and for inputs: Fn >= Fl and Pn >= Pl). That 
> for me
> transforms to
>
> (Pn - Pl) * Fn <= Pn * (Fn - Fl)
>
> and further
>
> -(Pl * Fn) <= -(Pn * Fl)
>
> or
>
> Pn * Fl <= Pl * Fn
>
> and I don't see why this would always hold. Yet if there can be underflow, I 
> wonder
> whether the calculation is actually correct. Or, ...
>

Because we are assuming that in normal circumstances, when F==0, P is the 
offset value, and
It shall be an non-smaller-than-zero value, tbh, ==0 is more logical fwit
So if it is underflow, I might think the hardware itself is malfunctional.

> > +    }
> > +    else
> > +    {
> > +        /* Read Processor Max Speed(mhz) as anchor point */
> > +        mul = data->caps.highest_perf;
> > +        div = this_cpu(max_freq_mhz);
> > +        if ( !div )
> > +            return -EINVAL;
> > +    }
> > +
> > +    res = offset + (mul * freq) / (div * 1000);
>
> ... considering that a negative offset here isn't really an issue, as long as 
> the rhs of
> the addition is large enough, is offset perhaps meant to be a signed quantity 
> (and
> considering it's in principle an [abstract] perf value, it doesn't even need 
> to be a 64-
> bit one, i.e. perhaps one of the cases where plain int is appropriate to use)?
>
> > +    if ( res > UINT8_MAX )
> > +    {
> > +        printk(XENLOG_ERR "Perf value exceeds maximum value 255:
> > + %lu\n", res);
>
> If this was to ever trigger, it would then likely trigger often. Imo such log 
> messages
> want to be printk_once(), if they're needed at all.
>
> > +        return -EINVAL;
>
> Can't we instead clip to 0xff?
>

True, clip it to 0xff maybe better

>
> Jan