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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Jul 13, 2023 at 9:02 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Yes.  ENOENT seems good here since a NULL data is comparable to not existing.

> > +    /* 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 &.

Sure

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

With all the earlier parameter checking, I think it would be fine to
return success here for a no-op.

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

-EOPNOTSUPP seems good for the !hwp_active() case.  Maybe ENOENT for
the policy one.

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

Sure.

Thanks,
Jason