Skip to contentSkip to conversation

KVM: nSVM: Fix IOIO bitmap evaluation

PATCH In review Jan Kiszka 3 participants 5 messages Jun 30, 2014 – Jul 1, 2014

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 KVM: nSVM: Fix IOIO bitmap evaluation

From: Jan Kiszka <[email protected]>

First, kvm_read_guest returns 0 on success. And then we need to take the access size into account when testing the bitmap: intercept if any of bits corresponding to the access is set.

Signed-off-by Jan Kiszka <[email protected]>

---

This does not yet solve all IOIO issues, there are emulation-related bugs remaining, but this fix is self-contained.

 arch/x86/kvm/svm.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
arch/x86/kvm/svm.c +12 -7
index c79766e1..3483ac9 100644
@@ -2116,22 +2116,27 @@ static void nested_svm_unmap(struct page *page)
 
 static int nested_svm_intercept_ioio(struct vcpu_svm *svm)
 {
-	unsigned port;
-	u8 val, bit;
+	unsigned port, size, iopm_len;
+	u16 val, mask;
+	u8 start_bit;
 	u64 gpa;
 
 	if (!(svm->nested.intercept & (1ULL << INTERCEPT_IOIO_PROT)))
 		return NESTED_EXIT_HOST;
 
 	port = svm->vmcb->control.exit_info_1 >> 16;
+	size = (svm->vmcb->control.exit_info_1 & SVM_IOIO_SIZE_MASK) >>
+		SVM_IOIO_SIZE_SHIFT;
 	gpa  = svm->nested.vmcb_iopm + (port / 8);
-	bit  = port % 8;
-	val  = 0;
+	start_bit = port % 8;
+	iopm_len = (start_bit + size > 8) ? 2 : 1;
+	mask = (0xf >> (4 - size)) << start_bit;
+	val = 0;
 
-	if (kvm_read_guest(svm->vcpu.kvm, gpa, &val, 1))
-		val &= (1 << bit);
+	if (kvm_read_guest(svm->vcpu.kvm, gpa, &val, iopm_len))
+		return NESTED_EXIT_DONE;
 
-	return val ? NESTED_EXIT_DONE : NESTED_EXIT_HOST;
+	return (val & mask) ? NESTED_EXIT_DONE : NESTED_EXIT_HOST;
 }
 
 static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
Signature · 1 line
1.8.4.5
maintainer
#1 lore raw

Il 30/06/2014 10:54, Jan Kiszka ha scritto:

arch/x86/kvm/svm.c static void nested_svm_unmap(struct page *page) · line 2133

Quoted 7 lines
arch/x86/kvm/svm.c +4 -2
+		SVM_IOIO_SIZE_SHIFT;
 	gpa  = svm->nested.vmcb_iopm + (port / 8);
-	bit  = port % 8;
-	val  = 0;
+	start_bit = port % 8;
+	iopm_len = (start_bit + size > 8) ? 2 : 1;
+	mask = (0xf >> (4 - size)) << start_bit;

These two lines are tricky. :)

The patch looks good, I'll write a test case if you don't beat me to it.

Paolo

Quoted 2 lines
arch/x86/kvm/svm.c +1 -0
+	val = 0;
author
#2 lore raw
Quoted 10 lines · Paolo Bonzini

Il 30/06/2014 10:54, Jan Kiszka ha scritto:

arch/x86/kvm/svm.c static void nested_svm_unmap(struct page *page) · line 2133

Quoted 7 lines
arch/x86/kvm/svm.c +4 -2
+        SVM_IOIO_SIZE_SHIFT;
     gpa  = svm->nested.vmcb_iopm + (port / 8);
-    bit  = port % 8;
-    val  = 0;
+    start_bit = port % 8;
+    iopm_len = (start_bit + size > 8) ? 2 : 1;
+    mask = (0xf >> (4 - size)) << start_bit;

These two lines are tricky. :)

That last line was, indeed... :)

Quoted 2 lines

The patch looks good, I'll write a test case if you don't beat me to it.

I'm not going to beat you if you write any test case. :)

Didn't check the svm part of the unit tests recently, if they are as easily extensible for such scenarios like vmx is now.

Jan

Reviewed-by Acked-by on v1 1/1
#3 lore raw
In reply to Jan Kiszka
Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

First, kvm_read_guest returns 0 on success. And then we need to take the access size into account when testing the bitmap: intercept if any of bits corresponding to the access is set.

Signed-off-by Jan Kiszka <[email protected]>
Reviewed-by Joerg Roedel <[email protected]>
Acked-by Joerg Roedel <[email protected]>

I have the slight hope that this fixes the issues with L2 Linux guests on L1 Windows hypervisors. Have to check that at some point :)

arch/x86/kvm/svm.c static void nested_svm_unmap(struct page *page) · line 2137

Quoted 4 lines
arch/x86/kvm/svm.c +2 -2
-	if (kvm_read_guest(svm->vcpu.kvm, gpa, &val, 1))
-		val &= (1 << bit);
+	if (kvm_read_guest(svm->vcpu.kvm, gpa, &val, iopm_len))
+		return NESTED_EXIT_DONE;

Not related to that fix, but as a further improvement we should probably do a #vmexit(invalid-vmcb) or something if we can't read the iopm.

Joerg

author
#4 lore raw
Quoted 15 lines · Joerg Roedel
Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

First, kvm_read_guest returns 0 on success. And then we need to take the access size into account when testing the bitmap: intercept if any of bits corresponding to the access is set.

Signed-off-by Jan Kiszka <[email protected]>
Reviewed-by Joerg Roedel <[email protected]>
Acked-by Joerg Roedel <[email protected]>

I have the slight hope that this fixes the issues with L2 Linux guests on L1 Windows hypervisors. Have to check that at some point :)

Quoted 7 lines

arch/x86/kvm/svm.c static void nested_svm_unmap(struct page *page) · line 2137

Quoted 4 lines
arch/x86/kvm/svm.c +2 -2
-	if (kvm_read_guest(svm->vcpu.kvm, gpa, &val, 1))
-		val &= (1 << bit);
+	if (kvm_read_guest(svm->vcpu.kvm, gpa, &val, iopm_len))
+		return NESTED_EXIT_DONE;

Not related to that fix, but as a further improvement we should probably do a #vmexit(invalid-vmcb) or something if we can't read the iopm.

Yes, thought about this as well when thinking about kvm_read_guest failing. Some for MSR bitmap.

Jan