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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- 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?

> +        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)?

> +        {
> +            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.

>          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.

> --- 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.

> --- 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"?

> +    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.

> +    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?

Jan