Re: [PATCH v3 10/14 RESEND] xen: Add SET_CPUFREQ_HWP xen_sysctl_pm_op

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.05.2023 21:30, Jason Andryuk wrote:
> @@ -531,6 +533,100 @@ int get_hwp_para(const struct cpufreq_policy *policy,
>      return 0;
>  }
>  
> +int set_hwp_para(struct cpufreq_policy *policy,
> +                 struct xen_set_hwp_para *set_hwp)

const?

> +{
> +    unsigned int cpu = policy->cpu;
> +    struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
> +
> +    if ( data == NULL )
> +        return -EINVAL;
> +
> +    /* Validate all parameters first */
> +    if ( set_hwp->set_params & ~XEN_SYSCTL_HWP_SET_PARAM_MASK )
> +        return -EINVAL;
> +
> +    if ( set_hwp->activity_window & ~XEN_SYSCTL_HWP_ACT_WINDOW_MASK )
> +        return -EINVAL;

Below you limit checks to when the respective control bit is set. I
think you want the same here.

> +    if ( !feature_hwp_energy_perf &&
> +         (set_hwp->set_params & XEN_SYSCTL_HWP_SET_ENERGY_PERF) &&
> +         set_hwp->energy_perf > IA32_ENERGY_BIAS_MAX_POWERSAVE )
> +        return -EINVAL;
> +
> +    if ( (set_hwp->set_params & XEN_SYSCTL_HWP_SET_DESIRED) &&
> +         set_hwp->desired != 0 &&
> +         (set_hwp->desired < data->hw.lowest ||
> +          set_hwp->desired > data->hw.highest) )
> +        return -EINVAL;
> +
> +    /*
> +     * minimum & maximum are not validated as hardware doesn't seem to care
> +     * and the SDM says CPUs will clip internally.
> +     */
> +
> +    /* Apply presets */
> +    switch ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_PRESET_MASK )
> +    {
> +    case XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE:
> +        data->minimum = data->hw.lowest;
> +        data->maximum = data->hw.lowest;
> +        data->activity_window = 0;
> +        if ( feature_hwp_energy_perf )
> +            data->energy_perf = HWP_ENERGY_PERF_MAX_POWERSAVE;
> +        else
> +            data->energy_perf = IA32_ENERGY_BIAS_MAX_POWERSAVE;
> +        data->desired = 0;
> +        break;
> +
> +    case XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE:
> +        data->minimum = data->hw.highest;
> +        data->maximum = data->hw.highest;
> +        data->activity_window = 0;
> +        data->energy_perf = HWP_ENERGY_PERF_MAX_PERFORMANCE;
> +        data->desired = 0;
> +        break;
> +
> +    case XEN_SYSCTL_HWP_SET_PRESET_BALANCE:
> +        data->minimum = data->hw.lowest;
> +        data->maximum = data->hw.highest;
> +        data->activity_window = 0;
> +        if ( feature_hwp_energy_perf )
> +            data->energy_perf = HWP_ENERGY_PERF_BALANCE;
> +        else
> +            data->energy_perf = IA32_ENERGY_BIAS_BALANCE;
> +        data->desired = 0;
> +        break;
> +
> +    case XEN_SYSCTL_HWP_SET_PRESET_NONE:
> +        break;
> +
> +    default:
> +        return -EINVAL;
> +    }

So presets set all the values for which the individual item control bits
are clear. That's not exactly what I would have expected, and it took me
reading the code several times until I realized that you write life per-
CPU data fields here, not fields of some intermediate variable. I think
this could do with saying explicitly in the public header (if indeed the
intended model).

> +    /* Further customize presets if needed */
> +    if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_MINIMUM )
> +        data->minimum = set_hwp->minimum;
> +
> +    if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_MAXIMUM )
> +        data->maximum = set_hwp->maximum;
> +
> +    if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_ENERGY_PERF )
> +        data->energy_perf = set_hwp->energy_perf;
> +
> +    if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_DESIRED )
> +        data->desired = set_hwp->desired;
> +
> +    if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_ACT_WINDOW )
> +        data->activity_window = set_hwp->activity_window &
> +                                XEN_SYSCTL_HWP_ACT_WINDOW_MASK;
> +
> +    hwp_cpufreq_target(policy, 0, 0);
> +
> +    return 0;

I don't think you should assume here that hwp_cpufreq_target() will
only ever return 0. Plus by returning its return value here you
allow the compiler to tail-call optimize this code.

> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -398,6 +398,20 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>      return ret;
>  }
>  
> +static int set_cpufreq_hwp(struct xen_sysctl_pm_op *op)

const?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -317,6 +317,34 @@ struct xen_hwp_para {
>      uint8_t energy_perf;
>  };
>  
> +/* set multiple values simultaneously when set_args bit is set */

What "set_args bit" does this comment refer to?

> +struct xen_set_hwp_para {
> +#define XEN_SYSCTL_HWP_SET_DESIRED              (1U << 0)
> +#define XEN_SYSCTL_HWP_SET_ENERGY_PERF          (1U << 1)
> +#define XEN_SYSCTL_HWP_SET_ACT_WINDOW           (1U << 2)
> +#define XEN_SYSCTL_HWP_SET_MINIMUM              (1U << 3)
> +#define XEN_SYSCTL_HWP_SET_MAXIMUM              (1U << 4)
> +#define XEN_SYSCTL_HWP_SET_PRESET_MASK          0xf000
> +#define XEN_SYSCTL_HWP_SET_PRESET_NONE          0x0000
> +#define XEN_SYSCTL_HWP_SET_PRESET_BALANCE       0x1000
> +#define XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE     0x2000
> +#define XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE   0x3000
> +#define XEN_SYSCTL_HWP_SET_PARAM_MASK ( \
> +                                  XEN_SYSCTL_HWP_SET_PRESET_MASK | \
> +                                  XEN_SYSCTL_HWP_SET_DESIRED     | \
> +                                  XEN_SYSCTL_HWP_SET_ENERGY_PERF | \
> +                                  XEN_SYSCTL_HWP_SET_ACT_WINDOW  | \
> +                                  XEN_SYSCTL_HWP_SET_MINIMUM     | \
> +                                  XEN_SYSCTL_HWP_SET_MAXIMUM     )
> +    uint16_t set_params; /* bitflags for valid values */
> +#define XEN_SYSCTL_HWP_ACT_WINDOW_MASK          0x03ff
> +    uint16_t activity_window; /* See comment in struct xen_hwp_para */
> +    uint8_t minimum;
> +    uint8_t maximum;
> +    uint8_t desired;
> +    uint8_t energy_perf; /* 0-255 or 0-15 depending on HW support */

Instead of (or in addition to) the "HW support" reference, could this
gain a reference to the "get para" bit determining which range to use?

Jan