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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, May 22, 2023 at 9:11 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 22.05.2023 14:45, Jason Andryuk wrote:
> > On Mon, May 8, 2023 at 7:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 01.05.2023 21:30, Jason Andryuk wrote:
> >>> +    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.
> >
> > Not sure if you mean feature_hwp_activity_window or the bit in
> > set_params as control bit.  But, yes, they can both use some
> > additional checking.  IIRC, I wanted to always check
> > ~XEN_SYSCTL_HWP_ACT_WINDOW_MASK, because bits should never be set
> > whether or not the activity window is supported by hardware.
>
> I took ...
>
> >>> +    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;
>
> ... e.g. this for comparison, where you apply the range check only when
> the XEN_SYSCTL_HWP_* bit is set. I think you want to be consistent in
> such checking: Either you always allow the caller to not care about
> fields that aren't going to be consumed when their controlling bit is
> off, or you always check validity. Both approaches have their pros and
> cons, I think.

Ok, good point.  I wrote it inconsistently because the SDM states the
desired limit: "When set to a non-zero value (between the range of
Lowest_Performance and Highest_Performance of IA32_HWP_CAPABILITIES)
conveys an explicit performance request hint to the hardware;
effectively disabling HW Autonomous selection."  And I was trying to
follow that.  But later "The HWP hardware clips and resolves the field
values as necessary to the valid range." seems to override that.  I'll
test to verify that it is correct, and drop the lowest/highest
checking if so.

Thanks,
Jason