[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, March 25, 2025 5:58 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>; xen-
> de...@lists.xenproject.org; Jason Andryuk <jandr...@gmail.com>
> Subject: Re: [PATCH v3 09/15] xen/x86: introduce a new amd cppc driver for
> cpufreq scaling
>
> > + * directly use nominal_mhz and lowest_mhz as the division
> > + * will remove the frequency unit.
> > + */
> > + div = div ?: 1;
>
> Imo the cppc_data->lowest_mhz >= cppc_data->nominal_mhz case better
> wouldn't make it here, but use the fallback path below. Or special- case
> cppc_data->lowest_mhz == cppc_data->nominal_mhz: mul would
> (hopefully) be zero (i.e. there would be the expectation that
> data->caps.nominal_perf == data->caps.lowest_perf, yet no guarantee
> without checking), and hence ...
Okay, I'll drop the " div = div ?: 1", to strict the if() to
```
if ( cppc_data->cpc.lowest_mhz && cppc_data->cpc.nominal_mhz &&
(cppc_data->cpc.lowest_mhz != cppc_data->cpc.nominal_mhz) )
```
>
> > + offset = data->caps.nominal_perf -
> > + (mul * cppc_data->nominal_mhz) / div;
>
> ... offset = data->caps.nominal_perf regardless of "div" (as long as that's
> not zero).
> I.e. the "equal" case may still be fine to take this path.
>
> Or is there a check somewhere that lowest_mhz <= nominal_mhz and lowest_perf
> <= nominal_perf, which I'm simply overlooking?
>
Yes. I overlooked the scenario that lowest_mhz > nominal_mhz and lowest_perf >
nominal_perf
and I'll add the check on first read
> > +#define amd_get_freq(name)
> > \
>
> The macro parameter is used just ...
>
> > + static int amd_get_##name##_freq(const struct amd_cppc_drv_data
> > + *data, \
>
> ... here, ...
>
> > + unsigned int *freq)
> > \
> > + {
> > \
> > + const struct xen_processor_cppc *cppc_data = data->cppc_data;
> > \
> > + uint64_t mul, div, res;
> > \
> > +
> > \
> > + if ( cppc_data->name##_mhz )
> > \
> > + {
> > \
> > + /* Switch to khz */
> > \
> > + *freq = cppc_data->name##_mhz * 1000;
> > \
>
> ... twice here forthe MHz value, and ...
>
> > + return 0;
> > \
> > + }
> > \
> > +
> > \
> > + /* Read Processor Max Speed(mhz) as anchor point */
> > \
> > + mul = this_cpu(amd_max_freq_mhz);
> > \
> > + if ( !mul )
> > \
> > + return -EINVAL;
> > \
> > + div = data->caps.highest_perf;
> > \
> > + res = (mul * data->caps.name##_perf * 1000) / div;
> > \
>
> ... here for the respective perf indicator. Why does it take ...
>
> > + if ( res > UINT_MAX )
> > \
> > + {
> > \
> > + printk(XENLOG_ERR
> > \
> > + "Frequeny exceeds maximum value UINT_MAX: %lu\n", res);
> > \
> > + return -EINVAL;
> > \
> > + }
> > \
> > + *freq = (unsigned int)res;
> > \
> > +
> > \
> > + return 0;
> > \
> > + }
> > \
> > +
> > +amd_get_freq(lowest);
> > +amd_get_freq(nominal);
>
> ... two almost identical functions, when one (with two extra input
> parameters) would
> suffice?
>
I had a draft fix here, If it doesn't what you hope for, plz let me know
```
static int amd_get_lowest_and_nominal_freq(const struct amd_cppc_drv_data *data,
unsigned int *lowest_freq,
unsigned int *nominal_freq)
{
const struct xen_processor_cppc *cppc_data = data->cppc_data;
uint64_t mul, div, res;
uint8_t perf;
if ( !lowest_freq && !nominal_freq )
return -EINVAL;
if ( lowest_freq && cppc_data->cpc.lowest_mhz )
/* Switch to khz */
*lowest_freq = cppc_data->cpc.lowest_mhz * 1000;
if ( nominal_freq && cppc_data->cpc.nominal_mhz )
/* Switch to khz */
*nominal_freq = cppc_data->cpc.nominal_mhz * 1000;
/* Still have unresolved frequency */
if ( (lowest_freq && !(*lowest_freq)) ||
(nominal_freq && !(*nominal_freq)) )
{
do {
/* Calculate lowest frequency firstly if need */
if ( lowest_freq && !(*lowest_freq) )
perf = data->caps.lowest_perf;
else
perf = data->caps.nominal_perf;
/* Read Processor Max Speed(MHz) as anchor point */
mul = this_cpu(amd_max_pxfreq_mhz);
if ( mul == INVAL_FREQ_MHZ || !mul )
{
printk(XENLOG_ERR
"Failed to read valid processor max frequency as anchor
point: %lu\n",
mul);
return -EINVAL;
}
div = data->caps.highest_perf;
res = (mul * perf * 1000) / div;
if ( res > UINT_MAX || !res )
{
printk(XENLOG_ERR
"Frequeny exceeds maximum value UINT_MAX or being zero
value: %lu\n",
res);
return -EINVAL;
}
if ( lowest_freq && !(*lowest_freq) )
*lowest_freq = (unsigned int)res;
else
*nominal_freq = (unsigned int)res;
} while ( nominal_freq && !(*nominal_freq) );
}
return 0;
}
```
> In amd_cppc_khz_to_perf() you have a check to avoid division by zero. Why not
> the same safeguarding here?
>
div = data->caps.highest_perf; For highest_perf non-zero check, it is already
added
in amd_cppc_init_msrs()
> > +static int amd_get_max_freq(const struct amd_cppc_drv_data *data,
> > + unsigned int *max_freq) {
> > + unsigned int nom_freq, boost_ratio;
> > + int res;
> > +
> > + res = amd_get_nominal_freq(data, &nom_freq);
> > + if ( res )
> > + return res;
> > +
> > + boost_ratio = (unsigned int)(data->caps.highest_perf /
> > + data->caps.nominal_perf);
>
> Similarly here - I can't spot what would prevent division by zero.
>
In amd_cppc_init_msrs(), before calculating the frequency, we have checked
all caps.xxx_perf info shall not be zero.
I'll complement check to avoid "highest_perf < nominal_perf", to ensure that
the calculation result of boost_ratio must not be zero.
```
if ( data->caps.highest_perf == 0 || data->caps.lowest_perf == 0 ||
data->caps.nominal_perf == 0 || data->caps.lowest_nonlinear_perf == 0
||
data->caps.lowest_perf > data->caps.lowest_nonlinear_perf ||
data->caps.lowest_nonlinear_perf > data->caps.nominal_perf ||
data->caps.nominal_perf > data->caps.highest_perf )
```
>
> Jan
