linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] KVM: VMX: x2APIC + APICV MSR fix and cleanup
@ 2020-10-05 19:55 Sean Christopherson
  2020-10-05 19:55 ` [PATCH 1/2] KVM: VMX: Fix x2APIC MSR intercept handling on !APICV platforms Sean Christopherson
  2020-10-05 19:55 ` [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled Sean Christopherson
  0 siblings, 2 replies; 6+ messages in thread
From: Sean Christopherson @ 2020-10-05 19:55 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Alexander Graf, Aaron Lewis,
	Peter Xu

Patch 1 is an unofficial patch from Peter to fix x2APIC MSR interception
on non-APICV systems.  As Peter suggested, it really should be squashed
with commit 3eb900173c71 ("KVM: x86: VMX: Prevent MSR passthrough when MSR
access is denied").  Without the fix, KVM is completely busted on
non-APICV systems.

Patch 2 is a cleanup of sorts to revert back to the pre-filtering approach
of initializing the x2APIC MSR bitmaps for APICV.

Note, I haven't tested on an APICV system.  My APICV system appears to
have crashed over the weekend and I haven't yet journeyed back to the
lab to kick it.

Peter Xu (1):
  KVM: VMX: Fix x2APIC MSR intercept handling on !APICV platforms

Sean Christopherson (1):
  KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is
    enabled

 arch/x86/kvm/vmx/vmx.c | 45 ++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: VMX: Fix x2APIC MSR intercept handling on !APICV platforms
  2020-10-05 19:55 [PATCH 0/2] KVM: VMX: x2APIC + APICV MSR fix and cleanup Sean Christopherson
@ 2020-10-05 19:55 ` Sean Christopherson
  2020-10-05 19:55 ` [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled Sean Christopherson
  1 sibling, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2020-10-05 19:55 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Alexander Graf, Aaron Lewis,
	Peter Xu

From: Peter Xu <peterx@redhat.com>

Fix an inverted flag for intercepting x2APIC MSRs and intercept writes
by default, even when APICV is enabled.

Fixes: 3eb900173c71 ("KVM: x86: VMX: Prevent MSR passthrough when MSR access is denied")
Not-signed-off-by: Peter Xu <peterx@redhat.com>
[sean: added changelog]
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4551a7e80ebc..25ef0b22ac9e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3787,9 +3787,10 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu, u8 mode)
 	int msr;
 
 	for (msr = 0x800; msr <= 0x8ff; msr++) {
-		bool intercepted = !!(mode & MSR_BITMAP_MODE_X2APIC_APICV);
+		bool apicv = !!(mode & MSR_BITMAP_MODE_X2APIC_APICV);
 
-		vmx_set_intercept_for_msr(vcpu, msr, MSR_TYPE_RW, intercepted);
+		vmx_set_intercept_for_msr(vcpu, msr, MSR_TYPE_R, !apicv);
+		vmx_set_intercept_for_msr(vcpu, msr, MSR_TYPE_W, true);
 	}
 
 	if (mode & MSR_BITMAP_MODE_X2APIC) {
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled
  2020-10-05 19:55 [PATCH 0/2] KVM: VMX: x2APIC + APICV MSR fix and cleanup Sean Christopherson
  2020-10-05 19:55 ` [PATCH 1/2] KVM: VMX: Fix x2APIC MSR intercept handling on !APICV platforms Sean Christopherson
@ 2020-10-05 19:55 ` Sean Christopherson
  2020-10-07 14:01   ` Alexander Graf
  1 sibling, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2020-10-05 19:55 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Alexander Graf, Aaron Lewis,
	Peter Xu

Rework the resetting of the MSR bitmap for x2APIC MSRs to ignore
userspace filtering when APICV is enabled.  Allowing userspace to
intercept reads to x2APIC MSRs when APICV is fully enabled for the guest
simply can't work.   The LAPIC and thus virtual APIC is in-kernel and
cannot be directly accessed by userspace.  If userspace wants to
intercept x2APIC MSRs, then it should first disable APICV.

Opportunistically change the behavior to reset the full range of MSRs if
and only if APICV is enabled for KVM.  The MSR bitmaps are initialized
to intercept all reads and writes by default, and enable_apicv cannot be
toggled after KVM is loaded.  I.e. if APICV is disabled, simply toggle
the TPR MSR accordingly.

Note, this still allows userspace to intercept reads and writes to TPR,
and writes to EOI and SELF_IPI.  It is at least plausible userspace
interception could work for those registers, though it is still silly.

Cc: Alexander Graf <graf@amazon.com>
Cc: Aaron Lewis <aaronlewis@google.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 46 +++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 25ef0b22ac9e..e23c41ccfac9 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3782,28 +3782,42 @@ static u8 vmx_msr_bitmap_mode(struct kvm_vcpu *vcpu)
 	return mode;
 }
 
-static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu, u8 mode)
+static void vmx_reset_x2apic_msrs_for_apicv(struct kvm_vcpu *vcpu, u8 mode)
 {
+	unsigned long *msr_bitmap = to_vmx(vcpu)->vmcs01.msr_bitmap;
+	unsigned long read_intercept;
 	int msr;
 
-	for (msr = 0x800; msr <= 0x8ff; msr++) {
-		bool apicv = !!(mode & MSR_BITMAP_MODE_X2APIC_APICV);
+	read_intercept = (mode & MSR_BITMAP_MODE_X2APIC_APICV) ? 0 : ~0;
 
-		vmx_set_intercept_for_msr(vcpu, msr, MSR_TYPE_R, !apicv);
-		vmx_set_intercept_for_msr(vcpu, msr, MSR_TYPE_W, true);
+	for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) {
+		unsigned int read_idx = msr / BITS_PER_LONG;
+		unsigned int write_idx = read_idx + (0x800 / sizeof(long));
+
+		msr_bitmap[read_idx] = read_intercept;
+		msr_bitmap[write_idx] = ~0ul;
 	}
+}
 
-	if (mode & MSR_BITMAP_MODE_X2APIC) {
-		/*
-		 * TPR reads and writes can be virtualized even if virtual interrupt
-		 * delivery is not in use.
-		 */
-		vmx_disable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_TASKPRI), MSR_TYPE_RW);
-		if (mode & MSR_BITMAP_MODE_X2APIC_APICV) {
-			vmx_enable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_TMCCT), MSR_TYPE_RW);
-			vmx_disable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_EOI), MSR_TYPE_W);
-			vmx_disable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_SELF_IPI), MSR_TYPE_W);
-		}
+static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu, u8 mode)
+{
+	if (!cpu_has_vmx_msr_bitmap())
+		return;
+
+	if (enable_apicv)
+		vmx_reset_x2apic_msrs_for_apicv(vcpu, mode);
+
+	/*
+	 * TPR reads and writes can be virtualized even if virtual interrupt
+	 * delivery is not in use.
+	 */
+	vmx_set_intercept_for_msr(vcpu, X2APIC_MSR(APIC_TASKPRI), MSR_TYPE_RW,
+				  !(mode & MSR_BITMAP_MODE_X2APIC));
+
+	if (mode & MSR_BITMAP_MODE_X2APIC_APICV) {
+		vmx_enable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_TMCCT), MSR_TYPE_RW);
+		vmx_disable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_EOI), MSR_TYPE_W);
+		vmx_disable_intercept_for_msr(vcpu, X2APIC_MSR(APIC_SELF_IPI), MSR_TYPE_W);
 	}
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled
  2020-10-05 19:55 ` [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled Sean Christopherson
@ 2020-10-07 14:01   ` Alexander Graf
  2020-10-07 16:44     ` Peter Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2020-10-07 14:01 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Aaron Lewis, Peter Xu



On 05.10.20 21:55, Sean Christopherson wrote:
> 
> Rework the resetting of the MSR bitmap for x2APIC MSRs to ignore
> userspace filtering when APICV is enabled.  Allowing userspace to
> intercept reads to x2APIC MSRs when APICV is fully enabled for the guest
> simply can't work.   The LAPIC and thus virtual APIC is in-kernel and
> cannot be directly accessed by userspace.  If userspace wants to
> intercept x2APIC MSRs, then it should first disable APICV.
> 
> Opportunistically change the behavior to reset the full range of MSRs if
> and only if APICV is enabled for KVM.  The MSR bitmaps are initialized
> to intercept all reads and writes by default, and enable_apicv cannot be
> toggled after KVM is loaded.  I.e. if APICV is disabled, simply toggle
> the TPR MSR accordingly.
> 
> Note, this still allows userspace to intercept reads and writes to TPR,
> and writes to EOI and SELF_IPI.  It is at least plausible userspace
> interception could work for those registers, though it is still silly.
> 
> Cc: Alexander Graf <graf@amazon.com>
> Cc: Aaron Lewis <aaronlewis@google.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I'm not opposed in general to leaving APICV handled registers out of the 
filtering logic. However, this really needs a note in the documentation 
then, no?


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled
  2020-10-07 14:01   ` Alexander Graf
@ 2020-10-07 16:44     ` Peter Xu
  2020-10-19 16:32       ` Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Xu @ 2020-10-07 16:44 UTC (permalink / raw)
  To: Alexander Graf, Sean Christopherson
  Cc: Sean Christopherson, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, kvm, linux-kernel, Aaron Lewis

On Wed, Oct 07, 2020 at 04:01:59PM +0200, Alexander Graf wrote:
> 
> 
> On 05.10.20 21:55, Sean Christopherson wrote:
> > 
> > Rework the resetting of the MSR bitmap for x2APIC MSRs to ignore
> > userspace filtering when APICV is enabled.  Allowing userspace to
> > intercept reads to x2APIC MSRs when APICV is fully enabled for the guest
> > simply can't work.   The LAPIC and thus virtual APIC is in-kernel and
> > cannot be directly accessed by userspace.  If userspace wants to
> > intercept x2APIC MSRs, then it should first disable APICV.
> > 
> > Opportunistically change the behavior to reset the full range of MSRs if
> > and only if APICV is enabled for KVM.  The MSR bitmaps are initialized
> > to intercept all reads and writes by default, and enable_apicv cannot be
> > toggled after KVM is loaded.  I.e. if APICV is disabled, simply toggle
> > the TPR MSR accordingly.
> > 
> > Note, this still allows userspace to intercept reads and writes to TPR,
> > and writes to EOI and SELF_IPI.  It is at least plausible userspace
> > interception could work for those registers, though it is still silly.
> > 
> > Cc: Alexander Graf <graf@amazon.com>
> > Cc: Aaron Lewis <aaronlewis@google.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I'm not opposed in general to leaving APICV handled registers out of the
> filtering logic. However, this really needs a note in the documentation
> then, no?

If we want to forbid apicv msrs, should we even fail KVM_X86_SET_MSR_FILTER
directly then?

I've no strong opinion on whether these msrs should be restricted. I'm not sure
whether my understanding is correct here - to me, kvm should always depend on
the userspace to do the right thing to make the vm work.  To me, as long as the
error is self-contained and it does not affect kvm as a whole or the host, then
it seems still fine.

However I do agree that I also worried about vmx_update_msr_bitmap_x2apic()
being slower.  Majorly I see calls from vmx_refresh_apicv_exec_ctrl() or
nested, so I'm not sure whether that could make sense for some workload.  Btw,
that seems to be another change corresponds to the idea to restrict msr
filitering on apicv regs.

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled
  2020-10-07 16:44     ` Peter Xu
@ 2020-10-19 16:32       ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-10-19 16:32 UTC (permalink / raw)
  To: Peter Xu, Alexander Graf, Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Aaron Lewis

On 07/10/20 18:44, Peter Xu wrote:
> If we want to forbid apicv msrs, should we even fail KVM_X86_SET_MSR_FILTER
> directly then?

Yes, probably it should.  I'll send a patch shortly.

Paolo


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-19 16:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 19:55 [PATCH 0/2] KVM: VMX: x2APIC + APICV MSR fix and cleanup Sean Christopherson
2020-10-05 19:55 ` [PATCH 1/2] KVM: VMX: Fix x2APIC MSR intercept handling on !APICV platforms Sean Christopherson
2020-10-05 19:55 ` [PATCH 2/2] KVM: VMX: Ignore userspace MSR filters for x2APIC when APICV is enabled Sean Christopherson
2020-10-07 14:01   ` Alexander Graf
2020-10-07 16:44     ` Peter Xu
2020-10-19 16:32       ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).