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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2023 14:58, Jason Andryuk wrote:
> Add SET_CPUFREQ_HWP xen_sysctl_pm_op to set HWP parameters.  The sysctl
> supports setting multiple values simultaneously as indicated by the
> set_params bits.  This allows atomically applying new HWP configuration
> via a single wrmsr.
> 
> XEN_SYSCTL_HWP_SET_PRESET_BALANCE/PERFORMANCE/POWERSAVE provide three
> common presets.  Setting them depends on hardware limits which the
> hypervisor is already caching.  So using them allows skipping a
> hypercall to query the limits (lowest/highest) to then set those same
> values.  The code is organized to allow a preset to be refined with
> additional parameters if desired.
> 
> "most_efficient" and "guaranteed" could be additional presets in the
> future, but the are not added now.  Those levels can change at runtime,
> but we don't have code in place to monitor and update for those events.
> 
> Since activity window may not be supported by all hardware, omit writing
> it when not supported, and return that fact to userspace by updating
> set_params.
> 
> CPPC parameter checking disallows setting reserved bytes and ensure
> values are only non-zero when the corresponding set_params bit is set.
> There is no range checking (0-255 is allowed) since hardware is
> documented to clip internally.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one last nit taken care of:

> @@ -539,6 +543,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 -ENOENT;
> +
> +    /* 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) )
> +        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 ) {

Yet another misplaced brace.

Jan