Re: [PATCH] x86/msr: fix handling of MSR_IA32_PERF_{STATUS/CTL}

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Roger Pau Monne <roger.pau@citrix.com>, <xen-devel@lists.xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>, Wei Liu <wl@xen.org>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH] x86/msr: fix handling of MSR_IA32_PERF_{STATUS/CTL}
Date: Wed, 7 Oct 2020 13:06:08 +0100	[thread overview]
Message-ID: <a98d6cb1-0b1d-8fb8-8718-c65e02e448bb@citrix.com> (raw)
In-Reply-To: <20201006162327.93055-1-roger.pau@citrix.com>

On 06/10/2020 17:23, Roger Pau Monne wrote:
> 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.

This might be how the current logic "works", but its straight up broken.

PERF_CTL is thread scope, so unless dom0 is identity pinned and has one
vcpu for every pcpu, it cannot use the interface correctly.

> 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}.

OTOH, PERF_CTL does have a seemingly architectural "please disable turbo
for me" bit, which is supposed to be for calibration loops.  I wonder if
anyone uses this, and whether we ought to honour it (probably not).

> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index d8ed83f869..41baa3b7a1 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1069,6 +1069,12 @@ extern enum cpufreq_controller {
>      FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen
>  } cpufreq_controller;
>  
> +static inline bool is_cpufreq_controller(const struct domain *d)
> +{
> +    return ((cpufreq_controller == FREQCTL_dom0_kernel) &&
> +            is_hardware_domain(d));

This won't compile on !CONFIG_X86, due to CONFIG_HAS_CPUFREQ

Honestly - I don't see any point to this code.  Its opt-in via the
command line only, and doesn't provide adequate checks for enablement. 
(It's not as if we're lacking complexity or moving parts when it comes
to power/frequency management).

~Andrew

> +}
> +
>  int cpupool_move_domain(struct domain *d, struct cpupool *c);
>  int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>  int cpupool_get_id(const struct domain *d);