Re: [PATCH v5 10/18] xen/cpufreq: introduce a new amd cppc driver for cpufreq scaling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.07.2025 11:49, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, June 17, 2025 12:00 AM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>>
>> On 27.05.2025 10:48, Penny Zheng wrote:
>>> --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c
>>> +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
>>> +        /*
>>> +         * We don't need to convert to kHz for computing offset and can
>>> +         * directly use nominal_mhz and lowest_mhz as the division
>>> +         * will remove the frequency unit.
>>> +         */
>>> +        offset = data->caps.nominal_perf -
>>> +                 (mul * cppc_data->cpc.nominal_mhz) / div;
>>> +    }
>>> +    else
>>> +    {
>>> +        /* Read Processor Max Speed(MHz) as anchor point */
>>> +        mul = data->caps.highest_perf;
>>> +        div = this_cpu(pxfreq_mhz);
>>> +        if ( !div )
>>> +            return -EINVAL;
>>
>> What's wrong about the function arguments in this case? (Same question again 
>> on
>> further uses of EINVAL below.)
> 
> If we could not get processor max frequency, the whole function is useless...
> Maybe -EOPNOTSUPP is more suitable than -EINVAL;

I don't like EOPNOTSUPP very much either for the purpose, but it's surely better
than EINVAL.

>>> +static int cf_check amd_cppc_cpufreq_target(struct cpufreq_policy *policy,
>>> +                                            unsigned int target_freq,
>>> +                                            unsigned int relation) {
>>> +    unsigned int cpu = policy->cpu;
>>> +    const struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu);
>>> +    uint8_t des_perf;
>>> +    int res;
>>> +
>>> +    if ( unlikely(!target_freq) )
>>> +        return 0;
>>> +
>>> +    res = amd_cppc_khz_to_perf(data, target_freq, &des_perf);
>>> +    if ( res )
>>> +        return res;
>>> +
>>> +    /*
>>> +     * Setting with "lowest_nonlinear_perf" to ensure governoring
>>> +     * performance in P-state range.
>>> +     */
>>> +    amd_cppc_write_request(policy->cpu, data->caps.lowest_nonlinear_perf,
>>> +                           des_perf, data->caps.highest_perf);
>>
>> I fear I don't understand the comment, and hence it remains unclear to me why
>> lowest_nonlinear_perf is being used here.
> 
> How about
> ```
> Choose lowest nonlinear performance as the minimum performance level at which 
> the platform may run.
> Lowest nonlinear performance is the lowest performance level at which 
> nonlinear power savings are achieved,
> Above this threshold, lower performance levels should be generally more 
> energy efficient than higher performance levels.
> ```

I finally had to go to the ACPI spec to understand what this is about. There 
looks
to be an implication that lowest <= lowest_nonlinear, and states in that range
would correspond more to T-states than to P-states. With that I think I agree 
with
the use of lowest_nonlinear here. The comment, however, could do with moving
farther away from merely quoting the pretty abstract text in the ACPI spec, as
such quoting doesn't help in clarifying terminology used, when that terminology
also isn't explained anywhere else in the code base.

Jan