diff mbox series

[RFC,01/22] x86/msr: MSR_PLATFORM_INFO shouldn't claim that turbo is programmable

Message ID 17a99e1da838a2edeeffa5a988e22c6fcb31406b.1698261255.git.edwin.torok@cloud.com (mailing list archive)
State New, archived
Headers show
Series vPMU bugfixes and support for PMUv5 | expand

Commit Message

Edwin Torok Oct. 25, 2023, 7:29 p.m. UTC
From: Edwin Török <edvin.torok@citrix.com>

Xen forbids writes to the various turbo control MSRs, however MSR_PLATFORM_INFO claims that these MSRs are writable.
Override MSR_PLATFORM_INFO bits to indicate lack of support.

See Intel SDM Volume 4, 2.17.6 "MSRs Introduced in the Intel Xeon Scaslable Processor Family",
which describes that MSR_PLATFORM_INFO.[28] = 1 implies that MSR_TURBO_RATIO_LIMIT is R/W,
and similarly bit 29 for TDP control, and bit 30 for MSR_TEMPERATURE_TARGET.

These bits were not all present on earlier processors, however where missing the bits were reserved,
and when present they are always present in the same bits.

(Curiously bit 31 that Xen uses is not documented anywhere in this manual but a separate one).

Backport: 4.0+

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 xen/arch/x86/cpu-policy.c            | 8 ++++++++
 xen/include/xen/lib/x86/cpu-policy.h | 5 ++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Andrew Cooper Oct. 25, 2023, 8:33 p.m. UTC | #1
On 25/10/2023 8:29 pm, Edwin Török wrote:
> From: Edwin Török <edvin.torok@citrix.com>
>
> Xen forbids writes to the various turbo control MSRs, however MSR_PLATFORM_INFO claims that these MSRs are writable.
> Override MSR_PLATFORM_INFO bits to indicate lack of support.
>
> See Intel SDM Volume 4, 2.17.6 "MSRs Introduced in the Intel Xeon Scaslable Processor Family",
> which describes that MSR_PLATFORM_INFO.[28] = 1 implies that MSR_TURBO_RATIO_LIMIT is R/W,
> and similarly bit 29 for TDP control, and bit 30 for MSR_TEMPERATURE_TARGET.
>
> These bits were not all present on earlier processors, however where missing the bits were reserved,
> and when present they are always present in the same bits.
>
> (Curiously bit 31 that Xen uses is not documented anywhere in this manual but a separate one).
>
> Backport: 4.0+
>
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>

p->platform_info never has any bit other than cpuid_faulting set in it. 
We still don't even report the proper raw value, because we don't (yet)
have clean MSR derivation logic.

I'm confused as to how you managed to find these set.  Even back in Xen
4.13, PLATFORM_INFO was covered by the msr_policy (later merged into
cpu_policy).  Furthermore, even patch 3 oughtn't to have such an effect.

Sadly, the whole of this MSR is model specific.  Vol4 2.17 is only for
one SKX/CLX/ICX/SPR.  Technically its wrong to treat the cpuid_faulting
in the way we do, but it is enumerated separately, and we intentionally
don't have an Intel check because we need to emulate CPUID faulting on
AMD hardware to make PVShim work.

~Andrew
Edwin Torok Oct. 26, 2023, 4:48 p.m. UTC | #2
> On 25 Oct 2023, at 21:33, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 25/10/2023 8:29 pm, Edwin Török wrote:
>> From: Edwin Török <edvin.torok@citrix.com>
>> 
>> Xen forbids writes to the various turbo control MSRs, however MSR_PLATFORM_INFO claims that these MSRs are writable.
>> Override MSR_PLATFORM_INFO bits to indicate lack of support.
>> 
>> See Intel SDM Volume 4, 2.17.6 "MSRs Introduced in the Intel Xeon Scaslable Processor Family",
>> which describes that MSR_PLATFORM_INFO.[28] = 1 implies that MSR_TURBO_RATIO_LIMIT is R/W,
>> and similarly bit 29 for TDP control, and bit 30 for MSR_TEMPERATURE_TARGET.
>> 
>> These bits were not all present on earlier processors, however where missing the bits were reserved,
>> and when present they are always present in the same bits.
>> 
>> (Curiously bit 31 that Xen uses is not documented anywhere in this manual but a separate one).
>> 
>> Backport: 4.0+
>> 
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> 
> p->platform_info never has any bit other than cpuid_faulting set in it. 
> We still don't even report the proper raw value, because we don't (yet)
> have clean MSR derivation logic.
> 
> I'm confused as to how you managed to find these set.  Even back in Xen
> 4.13, PLATFORM_INFO was covered by the msr_policy (later merged into
> cpu_policy).  Furthermore, even patch 3 oughtn't to have such an effect.
> 
> Sadly, the whole of this MSR is model specific.  Vol4 2.17 is only for
> one SKX/CLX/ICX/SPR.  Technically its wrong to treat the cpuid_faulting
> in the way we do, but it is enumerated separately, and we intentionally
> don't have an Intel check because we need to emulate CPUID faulting on
> AMD hardware to make PVShim work.
> 

xen/lib/x86/msr.c:    COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw);

This code made me believe that the underlying MSR value was copied, and only specific values from it were overwritten, I didn't spot any zeroing.
Although running rdmsr now (on 4.13.5) does show that all the other bits are zero, so the zeroing must happen somewhere else in the code, making this patch obsolete.
I'll drop it.

Thanks,
--Edwin

> ~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 81e574390f..64c8857a61 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -407,6 +407,14 @@  static void __init calculate_host_policy(void)
     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
     /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
     p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
+
+    /* Xen denies write access to turbo control MSRs, however natively the CPU may support them
+       and advertise those MSRs as writable by having bits 28 to 30 set to 1 in MSR_PLATFORM_INFO.
+       Set these bits to 0 to avoid confusing guests on the availability of turbo controls.
+    */
+    p->platform_info.programmable_ratio_turbo = 0;
+    p->platform_info.programmable_tdp_turbo = 0;
+    p->platform_info.programmable_tj_offset = 0;
 }
 
 static void __init guest_common_max_feature_adjustments(uint32_t *fs)
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index bab3eecda6..70479689f2 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -339,7 +339,10 @@  struct cpu_policy
     union {
         uint32_t raw;
         struct {
-            uint32_t :31;
+            uint32_t :28;
+            bool programmable_ratio_turbo:1;
+            bool programmable_tdp_turbo:1;
+            bool programmable_tj_offset:1;
             bool cpuid_faulting:1;
         };
     } platform_info;