[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] x86/msr: fix handling of MSR_IA32_PERF_{STATUS/CTL}
On 09.11.2020 18:38, Andrew Cooper wrote: > From: Roger Pau Monné <roger.pau@xxxxxxxxxx> > > Currently a PV hardware domain can also be given control over the CPU > frequency, and such guest is allowed to write to MSR_IA32_PERF_CTL. > However since commit 322ec7c89f6 the default behavior has been changed > to reject accesses to not explicitly handled MSRs, preventing PV > guests that manage CPU frequency from reading > MSR_IA32_PERF_{STATUS/CTL}. > > Additionally some HVM guests (Windows at least) will attempt to read > MSR_IA32_PERF_CTL and will panic if given back a #GP fault: > > vmx.c:3035:d8v0 RDMSR 0x00000199 unimplemented > d8v0 VIRIDIAN CRASH: 3b c0000096 fffff806871c1651 ffffda0253683720 0 > > Move the handling of MSR_IA32_PERF_{STATUS/CTL} to the common MSR > handling shared between HVM and PV guests, and add an explicit case > for reads to MSR_IA32_PERF_{STATUS/CTL}. > > Restore previous behavior and allow PV guests with the required > permissions to read the contents of the mentioned MSRs. Non privileged > guests will get 0 when trying to read those registers, as writes to > MSR_IA32_PERF_CTL by such guest will already be silently dropped. > > Fixes: 322ec7c89f6 ('x86/pv: disallow access to unknown MSRs') > Fixes: 84e848fd7a1 ('x86/hvm: disallow access to unknown MSRs') > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> with a nit, a minor adjustment request, and a question: > @@ -448,6 +467,21 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t > val) > goto gp_fault; > break; > > + /* > + * This MSR are not enumerated in CPUID. It has been around since > the s/are/is/ > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -1069,6 +1069,23 @@ extern enum cpufreq_controller { > FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen > } cpufreq_controller; > > +static always_inline bool is_cpufreq_controller(const struct domain *d) > +{ > + /* > + * A PV dom0 can be nominated as the cpufreq controller, instead of using > + * Xen's cpufreq driver, at which point dom0 gets direct access to > certain > + * MSRs. > + * > + * This interface only works when dom0 is identity pinned and has the > same > + * number of vCPUs as pCPUs on the system. > + * > + * It would be far better to paravirtualise the interface. > + */ > + return (IS_ENABLED(CONFIG_PV) && > + (cpufreq_controller == FREQCTL_dom0_kernel) && > + is_pv_domain(d) && is_hardware_domain(d)); > +} IS_ENABLED(CONFIG_PV) is already part of is_pv_domain() and hence imo shouldn't be used explicitly here. Also, this being an x86 concept, is it really a good idea to put in xen/sched.h? I guess this builds on Arm just because of DCE from the IS_ENABLED(CONFIG_PV) (where afaict the one in is_pv_domain() will still do). (But yes, I do realize that cpufreq_controller itself gets declared in this file, so maybe better to do some subsequent cleanup.) Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |