Re: [PATCH v4 09/15] xen/x86: introduce a new amd cppc driver for cpufreq scaling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2025 11:19, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, April 29, 2025 10:29 PM
>>
>> On 14.04.2025 09:40, 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 linear function passing by the 2 points:
>>> + *  - (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) {
>>> +    const struct xen_processor_cppc *cppc_data = data->cppc_data;
>>> +    uint64_t mul, div;
>>> +    int offset = 0, res;
>>> +
>>> +    if ( freq == (cppc_data->cpc.nominal_mhz * 1000) )
>>> +    {
>>> +        *perf = data->caps.nominal_perf;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if ( freq == (cppc_data->cpc.lowest_mhz * 1000) )
>>> +    {
>>> +        *perf = data->caps.lowest_perf;
>>> +        return 0;
>>> +    }
>>
>> How likely is it that these two early return paths are taken, when the 
>> incoming
>> "freq" is 25 or 5 MHz granular? IOW - is it relevant to shortcut these two 
>> cases?
>>
> 
> Sorry, I may not understand what you mean here.
> Answering " How likely is it that these two early return paths are taken "
> It's rare ig.... maybe *ondemand* governor will frequently give frequency 
> around nominal frequency,
> but the exact value is rare ig.
> I'm confused about " when the incoming  "freq" is 25 or 5 MHz granular ".
> Are we talking about is it worthy to have these two early return paths 
> considering it is rarely taken

Yes - why have extra code which is expected to be hardly ever used. Things
would be different if such shortcuts covered common cases.

>>> +static int amd_get_max_freq(const struct amd_cppc_drv_data *data,
>>> +                            unsigned int *max_freq) {
>>> +    unsigned int nom_freq = 0, boost_ratio;
>>> +    int res;
>>> +
>>> +    res = amd_get_lowest_or_nominal_freq(data,
>>> +                                         data->cppc_data->cpc.nominal_mhz,
>>> +                                         data->caps.nominal_perf,
>>> +                                         &nom_freq);
>>> +    if ( res )
>>> +        return res;
>>> +
>>> +    boost_ratio = (unsigned int)(data->caps.highest_perf /
>>> +                                 data->caps.nominal_perf);
>>
>> I may have seen logic ensuring nominal_perf isn't 0, so that part may be 
>> fine. What
>> guarantees this division to yield a positive value, though?
>> If it yields zero (say 0xff / 0x80), ...
> 
> I think maybe you were saying 0x80/0xff to yield zero value. For that, we 
> checked that highest_perf
> must not be smaller than nominal_perf during init, see ...

No, I indeed meant 0xff / 0x80, but yes, that will yield 1, not 0. My main
point though was about this calculation being pretty far off the real value
(rather closer to 2), i.e. in the overall calculation (including ...

>>> +    *max_freq = nom_freq * boost_ratio;

... this multiplication) some scaling may want introducing, or the
multiplication may want doing ahead of the division.

Jan