Re: 2.6.38-rc6: general protection error inside KVM 64 bits guest

[Gleb Natapov <gleb@redhat.com>]

On Sat, Mar 05, 2011 at 10:40:08AM +0100, Francis Moreau wrote:
> [ was post on LKML but try it here since it's more appropriate ]
> 
> Hello,
> 
> I'm running kernel 2.6.38-rc6 with qemu-kvm 0.12.3.
> 
> Doing this inside the guest:
> 
> int main(void)
> {
> 	if (ioperm(0x3e0, 4, 1) < 0) {
> 		perror("ioperm");
> 		exit(1);
> 	}
> 
> 	outb(0, 0x3e0);
> 	inb(0x3e1);
> 	printf("exiting succesfully\n");
> 	return 0;
> }
> 
> make a general protection error.
> 
> Looking for the faulty instruction with gdb and found that's the 'inb'
> instruction the culprit.
> 
> Running the same program on the host works fine.
> 
> Could anybody tell me what's wrong ?
> 
IO permission checking for 64-bit guest in KVM is wrong. The patch bellow
should fix it.


diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 50ebc32..7ef5b86 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -142,10 +142,8 @@ struct x86_emulate_ops {
 	int (*pio_out_emulated)(int size, unsigned short port, const void *val,
 				unsigned int count, struct kvm_vcpu *vcpu);
 
-	bool (*get_cached_descriptor)(struct desc_struct *desc,
-				      int seg, struct kvm_vcpu *vcpu);
-	void (*set_cached_descriptor)(struct desc_struct *desc,
-				      int seg, struct kvm_vcpu *vcpu);
+	bool (*get_cached_descriptor)(void *p, int seg, struct kvm_vcpu *vcpu);
+	void (*set_cached_descriptor)(void *p, int seg, struct kvm_vcpu *vcpu);
 	u16 (*get_segment_selector)(int seg, struct kvm_vcpu *vcpu);
 	void (*set_segment_selector)(u16 sel, int seg, struct kvm_vcpu *vcpu);
 	unsigned long (*get_cached_segment_base)(int seg, struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index ad46239..d7d8b63 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1764,25 +1764,35 @@ static bool emulator_io_port_access_allowed(struct x86_emulate_ctxt *ctxt,
 					    struct x86_emulate_ops *ops,
 					    u16 port, u16 len)
 {
-	struct desc_struct tr_seg;
+	union {
+		struct desc_struct tss32;
+#ifdef CONFIG_X86_64
+		struct ldttss_desc64 tss64;
+#endif
+	} tr_seg; 
 	int r;
 	u16 io_bitmap_ptr;
 	u8 perm, bit_idx = port & 0x7;
 	unsigned mask = (1 << len) - 1;
+	unsigned long base;
 
 	ops->get_cached_descriptor(&tr_seg, VCPU_SREG_TR, ctxt->vcpu);
-	if (!tr_seg.p)
+	if (!tr_seg.tss32.p)
 		return false;
-	if (desc_limit_scaled(&tr_seg) < 103)
+	if (desc_limit_scaled(&tr_seg.tss32) < 103)
 		return false;
-	r = ops->read_std(get_desc_base(&tr_seg) + 102, &io_bitmap_ptr, 2,
-			  ctxt->vcpu, NULL);
+	base = get_desc_base(&tr_seg.tss32);
+#ifdef CONFIG_X86_64
+	if (ctxt->mode == X86EMUL_MODE_PROT64)
+		base |= ((u64)tr_seg.tss64.base3) << 32;
+#endif
+	r = ops->read_std(base + 102, &io_bitmap_ptr, 2, ctxt->vcpu, NULL);
 	if (r != X86EMUL_CONTINUE)
 		return false;
-	if (io_bitmap_ptr + port/8 > desc_limit_scaled(&tr_seg))
+	if (io_bitmap_ptr + port/8 > desc_limit_scaled(&tr_seg.tss32))
 		return false;
-	r = ops->read_std(get_desc_base(&tr_seg) + io_bitmap_ptr + port/8,
-			  &perm, 1, ctxt->vcpu, NULL);
+	r = ops->read_std(base + io_bitmap_ptr + port/8, &perm, 1, ctxt->vcpu,
+			  NULL);
 	if (r != X86EMUL_CONTINUE)
 		return false;
 	if ((perm >> bit_idx) & mask)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 785ae0c..e41e098 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4162,11 +4162,12 @@ static unsigned long emulator_get_cached_segment_base(int seg,
 	return get_segment_base(vcpu, seg);
 }
 
-static bool emulator_get_cached_descriptor(struct desc_struct *desc, int seg,
+static bool emulator_get_cached_descriptor(void *p, int seg,
 					   struct kvm_vcpu *vcpu)
 {
 	struct kvm_segment var;
-
+	struct desc_struct *desc = p;
+	
 	kvm_get_segment(vcpu, &var, seg);
 
 	if (var.unusable)
@@ -4185,13 +4186,22 @@ static bool emulator_get_cached_descriptor(struct desc_struct *desc, int seg,
 	desc->d = var.db;
 	desc->g = var.g;
 
+#ifdef CONFIG_X86_64
+	if (seg == VCPU_SREG_TR && is_long_mode(vcpu)) {
+		struct ldttss_desc64 *tss64 = p;
+		tss64->base3 = var.base >> 32;
+		tss64->zero1 = 0;
+	}
+#endif
+
 	return true;
 }
 
-static void emulator_set_cached_descriptor(struct desc_struct *desc, int seg,
+static void emulator_set_cached_descriptor(void *p, int seg,
 					   struct kvm_vcpu *vcpu)
 {
 	struct kvm_segment var;
+	struct desc_struct *desc = p;
 
 	/* needed to preserve selector */
 	kvm_get_segment(vcpu, &var, seg);
@@ -4211,6 +4221,12 @@ static void emulator_set_cached_descriptor(struct desc_struct *desc, int seg,
 	var.present = desc->p;
 	var.unusable = !var.present;
 	var.padding = 0;
+#ifdef CONFIG_X86_64
+	if (seg == VCPU_SREG_TR && is_long_mode(vcpu)) {
+		struct ldttss_desc64 *tss64 = p;
+		var.base |= ((u64)tss64->base3) << 32;
+	}
+#endif
 
 	kvm_set_segment(vcpu, &var, seg);
 	return;
--
			Gleb.