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

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, July 4, 2025 2:21 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 10/18] xen/cpufreq: introduce a new amd cppc driver for
> cpufreq scaling
>
> On 04.07.2025 05:40, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Wednesday, July 2, 2025 6:48 PM
> >>
> >> 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:
> >>>>> +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
> >
> > Yes, It doesn't have definitive conclusion about relation between
> > lowest and lowest_nonlinear In our internal FW designed spec, it
> > always shows lowest_nonlinear corresponds to P2
> >
> >> 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.
> >
> >
> > How about we add detailed explanations about each terminology in the
> > beginning declaration , see:
> > ```
> > +/*
> > + * Field highest_perf, nominal_perf, lowest_nonlinear_perf, and
> > +lowest_perf
> > + * contain the values read from CPPC capability MSR.
> > + * Field highest_perf represents highest performance, which is the
> > +absolute
> > + * maximum performance an individual processor may reach, assuming
> > +ideal
> > + * conditions
> > + * Field nominal_perf represents maximum sustained performance level
> > +of the
> > + * processor, assuming ideal operating conditions.
> > + * Field lowest_nonlinear_perf represents Lowest Nonlinear
> > +Performance, which
> > + * 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.
>
> Which is still only the vague statement also found in the spec. This says 
> nothing
> about what happens below that level, or what the relationship to other fields 
> is.
>
> > + * Field lowest_perf represents the absolute lowest performance level
> > +of the
> > + * platform.
> > + *
> > + * Field max_perf, min_perf, des_perf store the values for CPPC request 
> > MSR.
> > + * Field max_perf conveys the maximum performance level at which the
> > +platform
> > + * may run. And it may be set to any performance value in the range
> > + * [lowest_perf, highest_perf], inclusive.
> > + * Field min_perf conveys the minimum performance level at which the
> > +platform
> > + * may run. And it may be set to any performance value in the range
> > + * [lowest_perf, highest_perf], inclusive but must be less than or
> > +equal to
> > + * max_perf.
> > + * Field des_perf conveys performance level Xen is requesting. And it
> > +may be
> > + * set to any performance value in the range [min_perf, max_perf], 
> > inclusive.
> > + */
> > +struct amd_cppc_drv_data
> > +{
> > +    const struct xen_processor_cppc *cppc_data;
> > +    union {
> > +        uint64_t raw;
> > +        struct {
> > +            unsigned int lowest_perf:8;
> > +            unsigned int lowest_nonlinear_perf:8;
> > +            unsigned int nominal_perf:8;
> > +            unsigned int highest_perf:8;
> > +            unsigned int :32;
> > +        };
> > +    } caps;
> > +    union {
> > +        uint64_t raw;
> > +        struct {
> > +            unsigned int max_perf:8;
> > +            unsigned int min_perf:8;
> > +            unsigned int des_perf:8;
> > +            unsigned int epp:8;
> > +            unsigned int :32;
> > +        };
> > +    } req;
> > +
> > +    int err;
> > +};
> > ``
> > Then here, we could elaborate the reason why we choose lowest_nonlinear_perf
> over lowest_perf:
> > ```
> > +    /*
> > +     * Having a performance level lower than the lowest nonlinear
> > +     * performance level, such as, lowest_perf <= perf <= 
> > lowest_nonliner_perf,
> > +     * may actually cause an efficiency penalty, So when deciding the 
> > min_perf
> > +     * value, we prefer lowest nonlinear performance over lowest 
> > performance
> > +     */
> > +    amd_cppc_write_request(policy->cpu, data->caps.lowest_nonlinear_perf,
> > +                           des_perf, data->caps.highest_perf);
> > ```
>
> This reads fine to me.
>
> Question then is though: Is setting lowest_perf as the low boundary a good 
> idea in
> any of the places? (Iirc it is used in one or two places. Or am I
> misremembering?)

Yes, in active mode, I choose lowest_perf as min_perf to try to extend the 
limitation for active(autonomous) mode
Maybe it is not a good choice. Maybe cpufreq driver is limited to do 
performance tuning in P-states range.

>
> Jan