On 12.11.2021 19:51, Andrew Cooper 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.to...@citrix.com>
>> Signed-off-by: <jane.malal...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Ian Jackson <i...@xenproject.org>
>> ---
>>
>> 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.
This is quite confusing in the docs - at least one of the tables calls
the bit "IDA Disable", while other entries at least also refer to the
effect of disabling IDA. I'm afraid I have trouble connecting turbo
mode and IDA disabling, unless both are two different names of the
same thing. Maybe they really are, except that SDM vol 2 uses yet
another slightly different term for CPUID[6].EAX[1]: "Intel Turbo Boost
Technology".
> 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'm afraid public documentation has no mention of a bit of this name.
Considering the above I wonder whether you mean "IDA engage" (bit 32),
albeit this doesn't seem very likely when you're taking about a
"disengage" bit. If it is, we'd still need to cope with it being
unavailable: While as per the doc it exists from Merom onwards, i.e.
just far enough back for all 64-bit capable processors to be covered,
at least there it is attributed "Mobile only".
Jan
> 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
>
