[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

Therefore, I propose that the update hook gets renamed to update_turbo()
to more clearly state it's purpose, and that we use the TURBO_DISENGAGE
bit as documented.

If we're going this route, I'd also like to make this hook consistent
with others, where we IPI directly, rather than having an intermediate
function pointer just to send an IPI.

~Andrew




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.