Re: [PATCH v5 12/15] xen: Add SET_CPUFREQ_HWP xen_sysctl_pm_op

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2023 20:54, Jason Andryuk wrote:
> @@ -531,6 +535,103 @@ int get_hwp_para(unsigned int cpu,
>      return 0;
>  }
>  
> +int set_hwp_para(struct cpufreq_policy *policy,
> +                 struct xen_set_cppc_para *set_cppc)
> +{
> +    unsigned int cpu = policy->cpu;
> +    struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
> +    bool cleared_act_window = false;
> +
> +    if ( data == NULL )
> +        return -EINVAL;

I don't think EINVAL is appropriate here. EOPNOTSUPP might be, or ENOENT,
or EIO, or perhaps a few others.

> +    /* Validate all parameters - Disallow reserved bits. */
> +    if ( set_cppc->minimum > 255 ||
> +         set_cppc->maximum > 255 ||
> +         set_cppc->desired > 255 ||
> +         set_cppc->energy_perf > 255 ||
> +         set_cppc->set_params & ~XEN_SYSCTL_CPPC_SET_PARAM_MASK ||
> +         set_cppc->activity_window & ~XEN_SYSCTL_CPPC_ACT_WINDOW_MASK )

Nit: Parentheses again please around the operands of &.

> +        return -EINVAL;
> +
> +    /* Only allow values if params bit is set. */
> +    if ( (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_DESIRED) &&
> +          set_cppc->desired) ||
> +         (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MINIMUM) &&
> +          set_cppc->minimum) ||
> +         (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_MAXIMUM) &&
> +          set_cppc->maximum) ||
> +         (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ENERGY_PERF) &&
> +          set_cppc->energy_perf) ||
> +         (!(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
> +          set_cppc->activity_window) )
> +        return -EINVAL;
> +
> +    /* Clear out activity window if lacking HW supported. */
> +    if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
> +         !feature_hwp_activity_window ) {
> +        set_cppc->set_params &= ~XEN_SYSCTL_CPPC_SET_ACT_WINDOW;
> +        cleared_act_window = true;
> +    }
> +
> +    /* Return if there is nothing to do. */
> +    if ( set_cppc->set_params == 0 )
> +        return cleared_act_window ? 0 : -EINVAL;

Is it really necessary to return an error when there's nothing to do?
We have various hypercalls which can degenerate to no-ops under
certain conditions, and which simply return success then.

> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -400,6 +400,19 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>      return ret;
>  }
>  
> +static int set_cpufreq_cppc(struct xen_sysctl_pm_op *op)
> +{
> +    struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
> +
> +    if ( !policy || !policy->governor )
> +        return -EINVAL;
> +
> +    if ( !hwp_active() )
> +        return -EINVAL;

In both cases I again wonder in how far EINVAL is really appropriate.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -351,6 +351,68 @@ struct xen_cppc_para {
>      uint32_t activity_window;
>  };
>  
> +/*
> + * Set CPPC values.
> + *
> + * Configure the parameters for CPPC.  Set bits in set_params control which
> + * values are applied.  If a bit is not set in set_params, the field must be
> + * zero.
> + *
> + * For HWP specifically, values must be limited to 0-255 or within
> + * XEN_SYSCTL_CPPC_ACT_WINDOW_MASK for activity window.  Set bits outside the
> + * range will be returned as -EINVAL.
> + *
> + * Activity Window may not be supported by the hardware.  In that case, the
> + * returned set_params will clear XEN_SYSCTL_CPPC_SET_ACT_WINDOW to indicate
> + * that it was not applied - though the rest of the values will be applied.
> + *
> + * There are a set of presets along with individual fields.  Presets are
> + * applied first, and then individual fields.  This allows customizing
> + * a preset without having to specify every value.
> + *
> + * The preset options values are as follows:
> + *
> + * preset      | minimum | maxium  | energy_perf
> + * ------------+---------+---------+----------------
> + * powersave   | lowest  | lowest  | powersave (255)
> + * ------------+---------+---------+----------------
> + * balance     | lowest  | highest | balance (128)
> + * ------------+---------+---------+----------------
> + * performance | highest | highest | performance (0)
> + *
> + * desired and activity_window are set to 0, hardware selected.
> + */
> +struct xen_set_cppc_para {
> +#define XEN_SYSCTL_CPPC_SET_MINIMUM              (1U << 0)
> +#define XEN_SYSCTL_CPPC_SET_MAXIMUM              (1U << 1)
> +#define XEN_SYSCTL_CPPC_SET_DESIRED              (1U << 2)
> +#define XEN_SYSCTL_CPPC_SET_ENERGY_PERF          (1U << 3)
> +#define XEN_SYSCTL_CPPC_SET_ACT_WINDOW           (1U << 4)
> +#define XEN_SYSCTL_CPPC_SET_PRESET_MASK          0xf0000000
> +#define XEN_SYSCTL_CPPC_SET_PRESET_NONE          0x00000000
> +#define XEN_SYSCTL_CPPC_SET_PRESET_BALANCE       0x10000000
> +#define XEN_SYSCTL_CPPC_SET_PRESET_POWERSAVE     0x20000000
> +#define XEN_SYSCTL_CPPC_SET_PRESET_PERFORMANCE   0x30000000

As corrections for the respective Misra rule are in the process of
being merged, please add U suffixes here (at the very least on the
_MASK).

Jan