| 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 |
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
> 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 --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;