Re: [PATCH v4 09/15] cpufreq: Export HWP parameters to userspace as CPPC

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Jun 19, 2023 at 10:24 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 14.06.2023 20:02, Jason Andryuk wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> > +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> > @@ -537,6 +537,29 @@ static const struct cpufreq_driver __initconstrel 
> > hwp_cpufreq_driver =
> >      .update = hwp_cpufreq_update,
> >  };
> >
> > +int get_hwp_para(const unsigned int cpu,
> > +                 struct xen_cppc_para *cppc_para)
> > +{
> > +    const struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
> > +
> > +    if ( data == NULL )
> > +        return -EINVAL;
>
> Maybe better -ENODATA in this case?

Sounds good.

> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -251,48 +251,54 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op 
> > *op)
> >      else
> >          strlcpy(op->u.get_para.scaling_driver, "Unknown", 
> > CPUFREQ_NAME_LEN);
> >
> > -    if ( !(scaling_available_governors =
> > -           xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> > -        return -ENOMEM;
> > -    if ( (ret = read_scaling_available_governors(
> > -                    scaling_available_governors,
> > -                    gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
> > +    if ( !strncasecmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER,
> > +                      CPUFREQ_NAME_LEN) )
>
> Mind me asking why you think case-insensitive compare is appropriate here?

I'll change to strncmp().  All the other string comparisons on
pmstat.c are strncasecmp, so I followed that pattern.

> > +        ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para);
> > +    else
> >      {
> > +        if ( !(scaling_available_governors =
> > +               xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> > +            return -ENOMEM;
> > +        if ( (ret = read_scaling_available_governors(
> > +                        scaling_available_governors,
> > +                        gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
>
> I realize you only re-indent this, but since you need to touch it anyway,
> may I suggest to also switch to siezof(*scaling_available_governors)?

How about dropping sizeof(*scaling_available_governors)?  This length ...

> > +        {
> > +            xfree(scaling_available_governors);
> > +            return ret;
> > +        }
> > +        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> > +                    scaling_available_governors, gov_num * 
> > CPUFREQ_NAME_LEN);
>
> Similarly here: Please adjust indentation while you touch this code.

... should match here, but this second one lacks the "* sizeof($foo)".
They are strings, so multiplying by sizeof() is unusual.

FTAOD, you want the indenting as:
        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
                            scaling_available_governors,
                            gov_num * CPUFREQ_NAME_LEN);
?

> >          xfree(scaling_available_governors);
> > -        return ret;
> > -    }
> > -    ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> > -                scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);
> > -    xfree(scaling_available_governors);
> > -    if ( ret )
> > -        return ret;
> > +        if ( ret )
> > +            return ret;
> >
> > -    op->u.get_para.u.s.scaling_cur_freq = policy->cur;
> > -    op->u.get_para.u.s.scaling_max_freq = policy->max;
> > -    op->u.get_para.u.s.scaling_min_freq = policy->min;
> > +        op->u.get_para.u.s.scaling_cur_freq = policy->cur;
> > +        op->u.get_para.u.s.scaling_max_freq = policy->max;
> > +        op->u.get_para.u.s.scaling_min_freq = policy->min;
> >
> > -    if ( policy->governor->name[0] )
> > -        strlcpy(op->u.get_para.u.s.scaling_governor,
> > -            policy->governor->name, CPUFREQ_NAME_LEN);
> > -    else
> > -        strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown",
> > -                CPUFREQ_NAME_LEN);
> > +        if ( policy->governor->name[0] )
> > +            strlcpy(op->u.get_para.u.s.scaling_governor,
> > +                policy->governor->name, CPUFREQ_NAME_LEN);
> > +        else
> > +            strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown",
> > +                    CPUFREQ_NAME_LEN);
> >
> > -    /* governor specific para */
> > -    if ( !strncasecmp(op->u.get_para.u.s.scaling_governor,
> > -                      "userspace", CPUFREQ_NAME_LEN) )
> > -    {
> > -        op->u.get_para.u.s.u.userspace.scaling_setspeed = policy->cur;
> > -    }
> > +        /* governor specific para */
> > +        if ( !strncasecmp(op->u.get_para.u.s.scaling_governor,
> > +                          "userspace", CPUFREQ_NAME_LEN) )
> > +        {
> > +            op->u.get_para.u.s.u.userspace.scaling_setspeed = policy->cur;
> > +        }
>
> Would also be nice if you could get rid of the unnecessary braces here
> at this occasion.

Sure

> > --- a/xen/include/acpi/cpufreq/cpufreq.h
> > +++ b/xen/include/acpi/cpufreq/cpufreq.h
> > @@ -248,5 +248,7 @@ void intel_feature_detect(struct cpufreq_policy 
> > *policy);
> >
> >  extern bool __initdata opt_cpufreq_hwp;
> >  int hwp_cmdline_parse(const char *s);
> > +int get_hwp_para(const unsigned int cpu,
>
> I think we generally avoid const when it's not a pointed-to type. It's
> not useful at all in a declaration.

Ok

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -296,6 +296,61 @@ struct xen_ondemand {
> >      uint32_t up_threshold;
> >  };
> >
> > +struct xen_cppc_para {
> > +    /* OUT */
> > +    /* activity_window supported if 1 */
> > +#define XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW  (1 << 0)
>
> I think 1 isn't very helpful, looking forward. Perhaps better "set" or
> "flag set"?

"set" works for me.

> > +    uint32_t features; /* bit flags for features */
> > +    /*
> > +     * See Intel SDM: HWP Performance Range and Dynamic Capabilities
> > +     *
> > +     * These four are 0-255 hardware-provided values.  They "continuous,
> > +     * abstract unit-less, performance" values.  smaller numbers are slower
> > +     * and larger ones are faster.
> > +     */
> > +    uint32_t lowest;
> > +    uint32_t lowest_nonlinear; /* most_efficient */
>
> Why non_linear in the external interface when internally you use
> most_efficient (merely put in the comment here)?
>
> > +    uint32_t nominal; /* guaranteed */
>
> Similar question for the name choice here.

There is a naming mismatch between the HWP fields and the CPPC fields.
The commit message includes:
The HWP most_efficient is mapped to CPPC lowest_nonlinear, and guaranteed is
mapped to nominal.  CPPC has a guaranteed that is optional while nominal
is required.  ACPI spec says "If this register is not implemented, OSPM
assumes guaranteed performance is always equal to nominal performance."

So the comments were to help with the mapping.  Should I prefix the
comments like "HWP: most_efficient"?

> > +    uint32_t highest;
> > +    /*
> > +     * See Intel SDM: IA32_HWP_REQUEST MSR (Address: 774H Logical Processor
> > +     * Scope)
> > +     *
> > +     * These are all hints, and the processor may deviate outside of them.
> > +     * Values below are 0-255.
> > +     *
> > +     * minimum and maximum can be set to the above hardware values to 
> > constrain
> > +     * operation.  The full range 0-255 is accepted and will be clipped by
> > +     * hardware.
> > +     */
> > +    uint32_t minimum;
> > +    uint32_t maximum;
> > +    /*
> > +     * Set an explicit performance hint, disabling hardware selection.
> > +     * 0 lets the hardware decide.
> > +     */
> > +    uint32_t desired;
>
> "Set" kind of conflicts with all fields being marked as OUT above. I think
> the word can simply be dropped?

Sounds good.

Thanks,
Jason