Re: [PATCH] xen/cpufreq: Reset policy after enabling/disabling turbo status

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Fri, Nov 12, 2021 at 1:51 PM Andrew Cooper <amc96@xxxxxxxx> wrote:
>
> On 10/11/2021 09:19, Jane Malalane wrote:
> > Before, user would change turbo status but this had no effect: boolean
> > was set but policy wasn't reevaluated.  Policy must be reevaluated so
> > that CPU frequency is chosen according to the turbo status and the
> > current governor.
> >
> > Therefore, add __cpufreq_governor() in cpufreq_update_turbo().
> >
> > Reported-by: <edvin.torok@xxxxxxxxxx>
> > Signed-off-by: <jane.malalane@xxxxxxxxxx>
> > ---
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> > ---
> >
> > Release rationale:
> > Not taking this patch means that turbo status is misleading.
> >
> > Taking this patch is low-risk as it only uses a function that already
> > exists and is already used for setting the chosen scaling governor.
> > Essentially, this change is equivalent to running 'xenpm
> > en/disable-turbo-mode' and, subsequently, running 'xenpm
> > set-scaling-governor <current governor>'.
> > ---
> >  xen/drivers/cpufreq/utility.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
> > index b93895d4dd..5f200ff3ee 100644
> > --- a/xen/drivers/cpufreq/utility.c
> > +++ b/xen/drivers/cpufreq/utility.c
> > @@ -417,10 +417,14 @@ int cpufreq_update_turbo(int cpuid, int new_state)
> >      {
> >          ret = cpufreq_driver.update(cpuid, policy);
> >          if (ret)
> > +        {
> >              policy->turbo = curr_state;
> > +            return ret;
> > +        }
> >      }
> >
> > -    return ret;
> > +    /* Reevaluate current CPU policy. */
> > +    return __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> >  }
>
> So, having looked through the manual, what the cpufreq_driver does for
> turbo on Intel is bogus according to the SDM.
>
> There is a non-architectrual dance with IA32_MISC_ENABLE bit 38 (per
> package) for firmware to configure turbo, but it manifests as another
> dynamic CPUID bit (which I think we handle correctly).  It has the same
> semantics as XD_DISABLE and CPUID_LIMIT so we might want to consider
> adding it to the set of bits we clear during boot.
>
> However, the correct way to turn off turbo is to set
> IA32_PERF_CTL.TURBO_DISENGAGE bit, which is per logical processor.  As
> such, it *should* behave far more like the AMD CPB path.

I wrote this in my HWP work (which I need to get back to...):
+/*
+ * The SDM reads like turbo should be disabled with MSR_IA32_PERF_CTL and
+ * PERF_CTL_TURBO_DISENGAGE, but that does not seem to actually work, at least
+ * with my HWP testing.  MSR_IA32_MISC_ENABLE and MISC_ENABLE_TURBO_DISENGAGE
+ * is what Linux uses and seems to work.
+ */

Regards,
Jason