On 09.11.2020 18:38, Andrew Cooper wrote:
> From: Roger Pau Monné <roger....@citrix.com>
>
> 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....@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
