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
