[PATCH v2 0/8] Fix x86 emulator's fault propagations

[PATCH v2 0/8] Fix x86 emulator's fault propagations

[Takuya Yoshikawa]

This patch set consists of macro replacements
and some fixes of fault handling in the x86
emulator.


Suggested by Marcelo, I separated these two
works and tried to make it clear what effects
each patch will produce: if you think that
reordering something in this series makes your
maintainance easier, I am willing to do so.


My motivation: What I want to achive by this
work is to make the basic style of x86 emulator
better for the following developments. Actually
unless we handle the fault properly, our works
implementing each instruction's emulation may
end up with no effect.

[PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Takuya Yoshikawa]

This patch injects page fault when reading descriptor in
load_guest_segment_descriptor() fails with FAULT.

Effects of this injection: This function is used by
kvm_load_segment_descriptor() which is necessary for the
following instructions.
 - mov seg,r/m16
 - jmp far
 - pop ?s
This patch makes it possible to emulate the page faults
generated by these instructions. But be sure that unless
we change the kvm_load_segment_descriptor()'s ret value
propagation this patch has no effect.


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/x86.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b2f91b9..38cb488 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 {
 	struct descriptor_table dtable;
 	u16 index = selector >> 3;
+	int ret;
 
 	get_segment_descriptor_dtable(vcpu, selector, &dtable);
 
@@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 		kvm_queue_exception_e(vcpu, GP_VECTOR, selector & 0xfffc);
 		return X86EMUL_PROPAGATE_FAULT;
 	}
-	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
+	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
+	if (ret == X86EMUL_PROPAGATE_FAULT)
+		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
+
+	return ret;
 }
 
 /* allowed just for 8 bytes segments */
-- 
1.6.3.3

[PATCH v2 2/8] Fix kvm_load_segment_descriptor()'s fault propagation

[Takuya Yoshikawa]

This patch makes kvm_load_segment_descriptor() to propagate
faults generated by load_guest_segment_descriptor().

We have confirmed that unless we change x86_emulate_insn() to
handle this propagated faults, this patch has no effect.


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/x86.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 38cb488..feed085 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4717,7 +4717,7 @@ static int kvm_load_realmode_segment(struct kvm_vcpu *vcpu, u16 selector, int se
 		.unusable = 0,
 	};
 	kvm_x86_ops->set_segment(vcpu, &segvar, seg);
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static int is_vm86_segment(struct kvm_vcpu *vcpu, int seg)
@@ -4741,12 +4741,15 @@ int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 {
 	struct kvm_segment kvm_seg;
 	struct desc_struct seg_desc;
+	int ret;
 
 	if (is_vm86_segment(vcpu, seg) || !is_protmode(vcpu))
 		return kvm_load_realmode_segment(vcpu, selector, seg);
 
-	if (load_guest_segment_descriptor(vcpu, selector, &seg_desc))
-		return 1;
+	ret = load_guest_segment_descriptor(vcpu, selector, &seg_desc);
+	if (ret != X86EMUL_CONTINUE)
+		return ret;
+
 	seg_desct_to_kvm_desct(&seg_desc, selector, &kvm_seg);
 
 	kvm_check_segment_descriptor(vcpu, seg, selector);
@@ -4763,7 +4766,7 @@ int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 		seg_desc.type |= 1;
 		save_guest_segment_descriptor(vcpu, selector, &seg_desc);
 	}
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static void save_state_to_tss32(struct kvm_vcpu *vcpu,
-- 
1.6.3.3

[PATCH v2 3/8] Fix x86_emulate_insn() to handle faults propagated from kvm_load_segment_descriptor()

[Takuya Yoshikawa]

We change the x86_emulate_insn() to handle faults propagated
from kvm_load_segment_descriptor().

Original code checks if the return value is negative or not.
But this means nothing because kvm_load_segment_descriptor()
never returns negative value. Instead of this, we use the
rc variable to hold the return value and if it is not
X86EMUL_CONTINUE goto done, not cannot_emulate.

Be sure that those codes following done label checks rc is
X86EMUL_UNHANDLEABLE or not, and if it is so do the same
thing as cannot_emulate: only the FAULT case will change.


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 645b245..4527940 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1980,7 +1980,6 @@ special_insn:
 	case 0x8e: { /* mov seg, r/m16 */
 		uint16_t sel;
 		int type_bits;
-		int err;
 
 		sel = c->src.val;
 		if (c->modrm_reg == VCPU_SREG_SS)
@@ -1988,16 +1987,16 @@ special_insn:
 
 		if (c->modrm_reg <= 5) {
 			type_bits = (c->modrm_reg == 1) ? 9 : 1;
-			err = kvm_load_segment_descriptor(ctxt->vcpu, sel,
-							  type_bits, c->modrm_reg);
+			rc = kvm_load_segment_descriptor(ctxt->vcpu, sel,
+							 type_bits, c->modrm_reg);
 		} else {
 			printk(KERN_INFO "Invalid segreg in modrm byte 0x%02x\n",
 					c->modrm);
 			goto cannot_emulate;
 		}
 
-		if (err < 0)
-			goto cannot_emulate;
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 
 		c->dst.type = OP_NONE;  /* Disable writeback. */
 		break;
@@ -2168,11 +2167,10 @@ special_insn:
 	case 0xe9: /* jmp rel */
 		goto jmp;
 	case 0xea: /* jmp far */
-		if (kvm_load_segment_descriptor(ctxt->vcpu, c->src2.val, 9,
-					VCPU_SREG_CS) < 0) {
-			DPRINTF("jmp far: Failed to load CS descriptor\n");
-			goto cannot_emulate;
-		}
+		rc = kvm_load_segment_descriptor(ctxt->vcpu, c->src2.val, 9,
+						 VCPU_SREG_CS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 
 		c->eip = c->src.val;
 		break;
-- 
1.6.3.3

[PATCH v2 4/8] X86EMUL macro replacements: from do_fetch_insn_byte() to x86_decode_insn()

[Takuya Yoshikawa]

This patch just replaces the integer values used inside x86's
decode functions to X86EMUL_*.

By this patch, it becomes clearer that we are using X86EMUL_*
value propagated from ops->read_std() in do_fetch_insn_byte().


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4527940..3bcd75e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -607,20 +607,20 @@ static int do_fetch_insn_byte(struct x86_emulate_ctxt *ctxt,
 	if (linear < fc->start || linear >= fc->end) {
 		size = min(15UL, PAGE_SIZE - offset_in_page(linear));
 		rc = ops->read_std(linear, fc->data, size, ctxt->vcpu);
-		if (rc)
+		if (rc != X86EMUL_CONTINUE)
 			return rc;
 		fc->start = linear;
 		fc->end = linear + size;
 	}
 	*dest = fc->data[linear - fc->start];
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static int do_insn_fetch(struct x86_emulate_ctxt *ctxt,
 			 struct x86_emulate_ops *ops,
 			 unsigned long eip, void *dest, unsigned size)
 {
-	int rc = 0;
+	int rc;
 
 	/* x86 instructions are limited to 15 bytes. */
 	if (eip + size - ctxt->decode.eip_orig > 15)
@@ -628,10 +628,10 @@ static int do_insn_fetch(struct x86_emulate_ctxt *ctxt,
 	eip += ctxt->cs_base;
 	while (size--) {
 		rc = do_fetch_insn_byte(ctxt, ops, eip++, dest++);
-		if (rc)
+		if (rc != X86EMUL_CONTINUE)
 			return rc;
 	}
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 /*
@@ -742,7 +742,7 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 	struct decode_cache *c = &ctxt->decode;
 	u8 sib;
 	int index_reg = 0, base_reg = 0, scale;
-	int rc = 0;
+	int rc = X86EMUL_CONTINUE;
 
 	if (c->rex_prefix) {
 		c->modrm_reg = (c->rex_prefix & 4) << 1;	/* REX.R */
@@ -855,7 +855,7 @@ static int decode_abs(struct x86_emulate_ctxt *ctxt,
 		      struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
-	int rc = 0;
+	int rc = X86EMUL_CONTINUE;
 
 	switch (c->ad_bytes) {
 	case 2:
@@ -876,7 +876,7 @@ int
 x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
-	int rc = 0;
+	int rc = X86EMUL_CONTINUE;
 	int mode = ctxt->mode;
 	int def_op_bytes, def_ad_bytes, group;
 
@@ -1005,7 +1005,7 @@ done_prefixes:
 		rc = decode_modrm(ctxt, ops);
 	else if (c->d & MemAbs)
 		rc = decode_abs(ctxt, ops);
-	if (rc)
+	if (rc != X86EMUL_CONTINUE)
 		goto done;
 
 	if (!c->has_seg_override)
-- 
1.6.3.3

[PATCH v2 5/8] X86EMUL macro replacements: x86_emulate_insn() and its helpers

[Takuya Yoshikawa]

This patch just replaces integer values used inside
x86_emulate_insn() and its helper functions to X86EMUL_*.

The purpose of this is to make it clear what will happen
when rc variable is compared to X86EMUL_* at the end of
x86_emulate_insn().


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |   62 ++++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3bcd75e..91c0eb7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -662,7 +662,7 @@ static int read_descriptor(struct x86_emulate_ctxt *ctxt,
 	*address = 0;
 	rc = ops->read_std((unsigned long)ptr, (unsigned long *)size, 2,
 			   ctxt->vcpu);
-	if (rc)
+	if (rc != X86EMUL_CONTINUE)
 		return rc;
 	rc = ops->read_std((unsigned long)ptr + 2, address, op_bytes,
 			   ctxt->vcpu);
@@ -1222,7 +1222,7 @@ static int emulate_pop_sreg(struct x86_emulate_ctxt *ctxt,
 	int rc;
 
 	rc = emulate_pop(ctxt, ops, &selector, c->op_bytes);
-	if (rc != 0)
+	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
 	rc = kvm_load_segment_descriptor(ctxt->vcpu, (u16)selector, 1, seg);
@@ -1248,7 +1248,7 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
 			struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
-	int rc = 0;
+	int rc = X86EMUL_CONTINUE;
 	int reg = VCPU_REGS_RDI;
 
 	while (reg >= VCPU_REGS_RAX) {
@@ -1259,7 +1259,7 @@ static int emulate_popa(struct x86_emulate_ctxt *ctxt,
 		}
 
 		rc = emulate_pop(ctxt, ops, &c->regs[reg], c->op_bytes);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			break;
 		--reg;
 	}
@@ -1270,12 +1270,8 @@ static inline int emulate_grp1a(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
-	int rc;
 
-	rc = emulate_pop(ctxt, ops, &c->dst.val, c->dst.bytes);
-	if (rc != 0)
-		return rc;
-	return 0;
+	return emulate_pop(ctxt, ops, &c->dst.val, c->dst.bytes);
 }
 
 static inline void emulate_grp2(struct x86_emulate_ctxt *ctxt)
@@ -1311,7 +1307,7 @@ static inline int emulate_grp3(struct x86_emulate_ctxt *ctxt,
 			       struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
-	int rc = 0;
+	int rc = X86EMUL_CONTINUE;
 
 	switch (c->modrm_reg) {
 	case 0 ... 1:	/* test */
@@ -1358,7 +1354,7 @@ static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
 		emulate_push(ctxt);
 		break;
 	}
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt,
@@ -1389,7 +1385,7 @@ static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt,
 			return rc;
 		ctxt->eflags |= EFLG_ZF;
 	}
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static int emulate_ret_far(struct x86_emulate_ctxt *ctxt,
@@ -1400,12 +1396,12 @@ static int emulate_ret_far(struct x86_emulate_ctxt *ctxt,
 	unsigned long cs;
 
 	rc = emulate_pop(ctxt, ops, &c->eip, c->op_bytes);
-	if (rc)
+	if (rc != X86EMUL_CONTINUE)
 		return rc;
 	if (c->op_bytes == 4)
 		c->eip = (u32)c->eip;
 	rc = emulate_pop(ctxt, ops, &cs, c->op_bytes);
-	if (rc)
+	if (rc != X86EMUL_CONTINUE)
 		return rc;
 	rc = kvm_load_segment_descriptor(ctxt->vcpu, (u16)cs, 1, VCPU_SREG_CS);
 	return rc;
@@ -1460,7 +1456,7 @@ static inline int writeback(struct x86_emulate_ctxt *ctxt,
 	default:
 		break;
 	}
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static void toggle_interruptibility(struct x86_emulate_ctxt *ctxt, u32 mask)
@@ -1695,7 +1691,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	struct decode_cache *c = &ctxt->decode;
 	unsigned int port;
 	int io_dir_in;
-	int rc = 0;
+	int rc = X86EMUL_CONTINUE;
 
 	ctxt->interruptibility = 0;
 
@@ -1793,7 +1789,7 @@ special_insn:
 		break;
 	case 0x07:		/* pop es */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_ES);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0x08 ... 0x0d:
@@ -1812,7 +1808,7 @@ special_insn:
 		break;
 	case 0x17:		/* pop ss */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_SS);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0x18 ... 0x1d:
@@ -1824,7 +1820,7 @@ special_insn:
 		break;
 	case 0x1f:		/* pop ds */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_DS);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0x20 ... 0x25:
@@ -1855,7 +1851,7 @@ special_insn:
 	case 0x58 ... 0x5f: /* pop reg */
 	pop_instruction:
 		rc = emulate_pop(ctxt, ops, &c->dst.val, c->op_bytes);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0x60:	/* pusha */
@@ -1863,7 +1859,7 @@ special_insn:
 		break;
 	case 0x61:	/* popa */
 		rc = emulate_popa(ctxt, ops);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0x63:		/* movsxd */
@@ -2003,7 +1999,7 @@ special_insn:
 	}
 	case 0x8f:		/* pop (sole member of Grp1a) */
 		rc = emulate_grp1a(ctxt, ops);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0x90: /* nop / xchg r8,rax */
@@ -2136,7 +2132,7 @@ special_insn:
 		break;
 	case 0xcb:		/* ret far */
 		rc = emulate_ret_far(ctxt, ops);
-		if (rc)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0xd0 ... 0xd1:	/* Grp2 */
@@ -2205,7 +2201,7 @@ special_insn:
 		break;
 	case 0xf6 ... 0xf7:	/* Grp3 */
 		rc = emulate_grp3(ctxt, ops);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0xf8: /* clc */
@@ -2231,14 +2227,14 @@ special_insn:
 		break;
 	case 0xfe ... 0xff:	/* Grp4/Grp5 */
 		rc = emulate_grp45(ctxt, ops);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	}
 
 writeback:
 	rc = writeback(ctxt, ops);
-	if (rc != 0)
+	if (rc != X86EMUL_CONTINUE)
 		goto done;
 
 	/* Commit shadow register state. */
@@ -2264,7 +2260,7 @@ twobyte_insn:
 				goto cannot_emulate;
 
 			rc = kvm_fix_hypercall(ctxt->vcpu);
-			if (rc)
+			if (rc != X86EMUL_CONTINUE)
 				goto done;
 
 			/* Let the processor re-execute the fixed hypercall */
@@ -2275,7 +2271,7 @@ twobyte_insn:
 		case 2: /* lgdt */
 			rc = read_descriptor(ctxt, ops, c->src.ptr,
 					     &size, &address, c->op_bytes);
-			if (rc)
+			if (rc != X86EMUL_CONTINUE)
 				goto done;
 			realmode_lgdt(ctxt->vcpu, size, address);
 			/* Disable writeback. */
@@ -2286,7 +2282,7 @@ twobyte_insn:
 				switch (c->modrm_rm) {
 				case 1:
 					rc = kvm_fix_hypercall(ctxt->vcpu);
-					if (rc)
+					if (rc != X86EMUL_CONTINUE)
 						goto done;
 					break;
 				default:
@@ -2296,7 +2292,7 @@ twobyte_insn:
 				rc = read_descriptor(ctxt, ops, c->src.ptr,
 						     &size, &address,
 						     c->op_bytes);
-				if (rc)
+				if (rc != X86EMUL_CONTINUE)
 					goto done;
 				realmode_lidt(ctxt->vcpu, size, address);
 			}
@@ -2420,7 +2416,7 @@ twobyte_insn:
 		break;
 	case 0xa1:	 /* pop fs */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_FS);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0xa3:
@@ -2439,7 +2435,7 @@ twobyte_insn:
 		break;
 	case 0xa9:	/* pop gs */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_GS);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
 	case 0xab:
@@ -2512,7 +2508,7 @@ twobyte_insn:
 		break;
 	case 0xc7:		/* Grp9 (cmpxchg8b) */
 		rc = emulate_grp9(ctxt, ops, memop);
-		if (rc != 0)
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		c->dst.type = OP_NONE;
 		break;
-- 
1.6.3.3

[PATCH v2 6/8] Fix x86_emulate_insn() not to use rc variable for non-X86EMUL values

[Takuya Yoshikawa]

This patch makes non-X86EMUL_* family functions not to use
rc variable.

Be sure that this changes nothing but makes the purpose of
rc variable clearer.


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 91c0eb7..a4e44ca 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2343,9 +2343,9 @@ twobyte_insn:
 	case 0x21: /* mov from dr to reg */
 		if (c->modrm_mod != 3)
 			goto cannot_emulate;
-		rc = emulator_get_dr(ctxt, c->modrm_reg, &c->regs[c->modrm_rm]);
-		if (rc)
+		if (emulator_get_dr(ctxt, c->modrm_reg, &c->regs[c->modrm_rm]))
 			goto cannot_emulate;
+		rc = X86EMUL_CONTINUE;
 		c->dst.type = OP_NONE;	/* no writeback */
 		break;
 	case 0x22: /* mov reg, cr */
@@ -2358,18 +2358,16 @@ twobyte_insn:
 	case 0x23: /* mov from reg to dr */
 		if (c->modrm_mod != 3)
 			goto cannot_emulate;
-		rc = emulator_set_dr(ctxt, c->modrm_reg,
-				     c->regs[c->modrm_rm]);
-		if (rc)
+		if (emulator_set_dr(ctxt, c->modrm_reg, c->regs[c->modrm_rm]))
 			goto cannot_emulate;
+		rc = X86EMUL_CONTINUE;
 		c->dst.type = OP_NONE;	/* no writeback */
 		break;
 	case 0x30:
 		/* wrmsr */
 		msr_data = (u32)c->regs[VCPU_REGS_RAX]
 			| ((u64)c->regs[VCPU_REGS_RDX] << 32);
-		rc = kvm_set_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], msr_data);
-		if (rc) {
+		if (kvm_set_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], msr_data)) {
 			kvm_inject_gp(ctxt->vcpu, 0);
 			c->eip = kvm_rip_read(ctxt->vcpu);
 		}
@@ -2378,8 +2376,7 @@ twobyte_insn:
 		break;
 	case 0x32:
 		/* rdmsr */
-		rc = kvm_get_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], &msr_data);
-		if (rc) {
+		if (kvm_get_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], &msr_data)) {
 			kvm_inject_gp(ctxt->vcpu, 0);
 			c->eip = kvm_rip_read(ctxt->vcpu);
 		} else {
-- 
1.6.3.3

[PATCH v2 7/8] Fix emulate_sys[call, enter, exit]()'s fault handling

[Takuya Yoshikawa]

This patch fixes emulate_syscall(), emulate_sysenter() and
emulate_sysexit() to handle injected faults properly.

Even though original code injects faults in these functions,
we cannot handle these unless we use the different return
value from the UNHANDLEABLE case. So this patch use X86EMUL_*
codes instead of -1 and 0 and makes x86_emulate_insn() to
handle these propagated faults.

Be sure that, in x86_emulate_insn(), goto cannot_emulate and
goto done with rc equals X86EMUL_UNHANDLEABLE have same effect.


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/emulate.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a4e44ca..ddec1c5 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1512,7 +1512,7 @@ emulate_syscall(struct x86_emulate_ctxt *ctxt)
 	/* syscall is not available in real mode */
 	if (c->lock_prefix || ctxt->mode == X86EMUL_MODE_REAL
 	    || !is_protmode(ctxt->vcpu))
-		return -1;
+		return X86EMUL_UNHANDLEABLE;
 
 	setup_syscalls_segments(ctxt, &cs, &ss);
 
@@ -1549,7 +1549,7 @@ emulate_syscall(struct x86_emulate_ctxt *ctxt)
 		ctxt->eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
 	}
 
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static int
@@ -1561,19 +1561,19 @@ emulate_sysenter(struct x86_emulate_ctxt *ctxt)
 
 	/* inject #UD if LOCK prefix is used */
 	if (c->lock_prefix)
-		return -1;
+		return X86EMUL_UNHANDLEABLE;
 
 	/* inject #GP if in real mode or paging is disabled */
 	if (ctxt->mode == X86EMUL_MODE_REAL || !is_protmode(ctxt->vcpu)) {
 		kvm_inject_gp(ctxt->vcpu, 0);
-		return -1;
+		return X86EMUL_PROPAGATE_FAULT;
 	}
 
 	/* XXX sysenter/sysexit have not been tested in 64bit mode.
 	* Therefore, we inject an #UD.
 	*/
 	if (ctxt->mode == X86EMUL_MODE_PROT64)
-		return -1;
+		return X86EMUL_UNHANDLEABLE;
 
 	setup_syscalls_segments(ctxt, &cs, &ss);
 
@@ -1582,13 +1582,13 @@ emulate_sysenter(struct x86_emulate_ctxt *ctxt)
 	case X86EMUL_MODE_PROT32:
 		if ((msr_data & 0xfffc) == 0x0) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			return -1;
+			return X86EMUL_PROPAGATE_FAULT;
 		}
 		break;
 	case X86EMUL_MODE_PROT64:
 		if (msr_data == 0x0) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			return -1;
+			return X86EMUL_PROPAGATE_FAULT;
 		}
 		break;
 	}
@@ -1613,7 +1613,7 @@ emulate_sysenter(struct x86_emulate_ctxt *ctxt)
 	kvm_x86_ops->get_msr(ctxt->vcpu, MSR_IA32_SYSENTER_ESP, &msr_data);
 	c->regs[VCPU_REGS_RSP] = msr_data;
 
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 static int
@@ -1626,18 +1626,18 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt)
 
 	/* inject #UD if LOCK prefix is used */
 	if (c->lock_prefix)
-		return -1;
+		return X86EMUL_UNHANDLEABLE;
 
 	/* inject #GP if in real mode or paging is disabled */
 	if (ctxt->mode == X86EMUL_MODE_REAL || !is_protmode(ctxt->vcpu)) {
 		kvm_inject_gp(ctxt->vcpu, 0);
-		return -1;
+		return X86EMUL_PROPAGATE_FAULT;
 	}
 
 	/* sysexit must be called from CPL 0 */
 	if (kvm_x86_ops->get_cpl(ctxt->vcpu) != 0) {
 		kvm_inject_gp(ctxt->vcpu, 0);
-		return -1;
+		return X86EMUL_PROPAGATE_FAULT;
 	}
 
 	setup_syscalls_segments(ctxt, &cs, &ss);
@@ -1655,7 +1655,7 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt)
 		cs.selector = (u16)(msr_data + 16);
 		if ((msr_data & 0xfffc) == 0x0) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			return -1;
+			return X86EMUL_PROPAGATE_FAULT;
 		}
 		ss.selector = (u16)(msr_data + 24);
 		break;
@@ -1663,7 +1663,7 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt)
 		cs.selector = (u16)(msr_data + 32);
 		if (msr_data == 0x0) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			return -1;
+			return X86EMUL_PROPAGATE_FAULT;
 		}
 		ss.selector = cs.selector + 8;
 		cs.db = 0;
@@ -1679,7 +1679,7 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt)
 	c->eip = ctxt->vcpu->arch.regs[VCPU_REGS_RDX];
 	c->regs[VCPU_REGS_RSP] = ctxt->vcpu->arch.regs[VCPU_REGS_RCX];
 
-	return 0;
+	return X86EMUL_CONTINUE;
 }
 
 int
@@ -2318,8 +2318,9 @@ twobyte_insn:
 		}
 		break;
 	case 0x05: 		/* syscall */
-		if (emulate_syscall(ctxt) == -1)
-			goto cannot_emulate;
+		rc = emulate_syscall(ctxt);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		else
 			goto writeback;
 		break;
@@ -2387,14 +2388,16 @@ twobyte_insn:
 		c->dst.type = OP_NONE;
 		break;
 	case 0x34:		/* sysenter */
-		if (emulate_sysenter(ctxt) == -1)
-			goto cannot_emulate;
+		rc = emulate_sysenter(ctxt);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		else
 			goto writeback;
 		break;
 	case 0x35:		/* sysexit */
-		if (emulate_sysexit(ctxt) == -1)
-			goto cannot_emulate;
+		rc = emulate_sysexit(ctxt);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		else
 			goto writeback;
 		break;
-- 
1.6.3.3

[PATCH v2 8/8] Tiny fix: remove redundant prototype of of load_pdptrs()

[Takuya Yoshikawa]

This patch removes redundant prototype of load_pdptrs().

I found load_pdptrs() twice in kvm_host.h . Let's remove one. 


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/include/asm/kvm_host.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1522337..9a5c005 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -671,7 +671,6 @@ void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva);
 void kvm_enable_tdp(void);
 void kvm_disable_tdp(void);
 
-int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
 int complete_pio(struct kvm_vcpu *vcpu);
 
 struct kvm_memory_slot *gfn_to_memslot_unaliased(struct kvm *kvm, gfn_t gfn);
-- 
1.6.3.3

Re: [PATCH v2 0/8] Fix x86 emulator's fault propagations

[Gleb Natapov]

On Wed, Feb 10, 2010 at 10:45:41AM +0900, Takuya Yoshikawa wrote:
> My motivation: What I want to achive by this
> work is to make the basic style of x86 emulator
> better for the following developments. Actually
> unless we handle the fault properly, our works
> implementing each instruction's emulation may
> end up with no effect.
Why do you want to emulate every instruction?

--
			Gleb.

Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Avi Kivity]

On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
> This patch injects page fault when reading descriptor in
> load_guest_segment_descriptor() fails with FAULT.
>
> Effects of this injection: This function is used by
> kvm_load_segment_descriptor() which is necessary for the
> following instructions.
>   - mov seg,r/m16
>   - jmp far
>   - pop ?s
> This patch makes it possible to emulate the page faults
> generated by these instructions. But be sure that unless
> we change the kvm_load_segment_descriptor()'s ret value
> propagation this patch has no effect.
>
>
> @@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>   {
>   	struct descriptor_table dtable;
>   	u16 index = selector>>  3;
> +	int ret;
>
>   	get_segment_descriptor_dtable(vcpu, selector,&dtable);
>
> @@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>   		kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
>   		return X86EMUL_PROPAGATE_FAULT;
>   	}
> -	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> +	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> +	if (ret == X86EMUL_PROPAGATE_FAULT)
> +		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
> +
> +	return ret;
>   }
>    

If the descriptor table is not aligned, and a descriptor spans two 
pages, then we might need to inject a page fault at some other address.

Also, the injection should be done in kvm_read_guest_virt() to avoid 
duplicating code.

These instructions however are only emulated in big real mode.  Where 
did you encounter the need to inject page faults during their emulation?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Marcelo Tosatti]

On Wed, Feb 10, 2010 at 06:25:42PM +0200, Avi Kivity wrote:
> On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
> >This patch injects page fault when reading descriptor in
> >load_guest_segment_descriptor() fails with FAULT.
> >
> >Effects of this injection: This function is used by
> >kvm_load_segment_descriptor() which is necessary for the
> >following instructions.
> >  - mov seg,r/m16
> >  - jmp far
> >  - pop ?s
> >This patch makes it possible to emulate the page faults
> >generated by these instructions. But be sure that unless
> >we change the kvm_load_segment_descriptor()'s ret value
> >propagation this patch has no effect.
> >
> >
> >@@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
> >  {
> >  	struct descriptor_table dtable;
> >  	u16 index = selector>>  3;
> >+	int ret;
> >
> >  	get_segment_descriptor_dtable(vcpu, selector,&dtable);
> >
> >@@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
> >  		kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
> >  		return X86EMUL_PROPAGATE_FAULT;
> >  	}
> >-	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> >+	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> >+	if (ret == X86EMUL_PROPAGATE_FAULT)
> >+		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
> >+
> >+	return ret;
> >  }
> 
> If the descriptor table is not aligned, and a descriptor spans two
> pages, then we might need to inject a page fault at some other
> address.
> 
> Also, the injection should be done in kvm_read_guest_virt() to avoid
> duplicating code.
> 
> These instructions however are only emulated in big real mode.
> Where did you encounter the need to inject page faults during their
> emulation?

Task switch exits also use them, so it should inject faults as
documented.

Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Gleb Natapov]

On Wed, Feb 10, 2010 at 06:25:42PM +0200, Avi Kivity wrote:
> On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
> >This patch injects page fault when reading descriptor in
> >load_guest_segment_descriptor() fails with FAULT.
> >
> >Effects of this injection: This function is used by
> >kvm_load_segment_descriptor() which is necessary for the
> >following instructions.
> >  - mov seg,r/m16
> >  - jmp far
> >  - pop ?s
> >This patch makes it possible to emulate the page faults
> >generated by these instructions. But be sure that unless
> >we change the kvm_load_segment_descriptor()'s ret value
> >propagation this patch has no effect.
> >
> >
> >@@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
> >  {
> >  	struct descriptor_table dtable;
> >  	u16 index = selector>>  3;
> >+	int ret;
> >
> >  	get_segment_descriptor_dtable(vcpu, selector,&dtable);
> >
> >@@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
> >  		kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
> >  		return X86EMUL_PROPAGATE_FAULT;
> >  	}
> >-	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> >+	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> >+	if (ret == X86EMUL_PROPAGATE_FAULT)
> >+		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
> >+
> >+	return ret;
> >  }
> 
> If the descriptor table is not aligned, and a descriptor spans two
> pages, then we might need to inject a page fault at some other
> address.
> 
> Also, the injection should be done in kvm_read_guest_virt() to avoid
> duplicating code.
> 
This function is used from inside emulator and I hope one day we will
make emulator independent of KVM, so it shouldn't inject event directly,
but rather return them as a result of emulation. Also this function is
used in kvm_report_emulation_failure() may be not the best place to
inject #PF.

> These instructions however are only emulated in big real mode.
> Where did you encounter the need to inject page faults during their
> emulation?
> 
> -- 
> error compiling committee.c: too many arguments to function
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH v2 0/8] Fix x86 emulator's fault propagations

[Avi Kivity]

On 02/10/2010 03:45 AM, Takuya Yoshikawa wrote:
> This patch set consists of macro replacements
> and some fixes of fault handling in the x86
> emulator.
>
>
> Suggested by Marcelo, I separated these two
> works and tried to make it clear what effects
> each patch will produce: if you think that
> reordering something in this series makes your
> maintainance easier, I am willing to do so.
>
>
> My motivation: What I want to achive by this
> work is to make the basic style of x86 emulator
> better for the following developments. Actually
> unless we handle the fault properly, our works
> implementing each instruction's emulation may
> end up with no effect.
>    

Patches 2-8 look fine, but I'd like Gleb to review them as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2 0/8] Fix x86 emulator's fault propagations

[Gleb Natapov]

On Wed, Feb 10, 2010 at 06:57:05PM +0200, Avi Kivity wrote:
> On 02/10/2010 03:45 AM, Takuya Yoshikawa wrote:
> >This patch set consists of macro replacements
> >and some fixes of fault handling in the x86
> >emulator.
> >
> >
> >Suggested by Marcelo, I separated these two
> >works and tried to make it clear what effects
> >each patch will produce: if you think that
> >reordering something in this series makes your
> >maintainance easier, I am willing to do so.
> >
> >
> >My motivation: What I want to achive by this
> >work is to make the basic style of x86 emulator
> >better for the following developments. Actually
> >unless we handle the fault properly, our works
> >implementing each instruction's emulation may
> >end up with no effect.
> 
> Patches 2-8 look fine, but I'd like Gleb to review them as well.
> 
I will do, but I already see that they need to be rebased onto the
latest master.

--
			Gleb.

Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Takuya Yoshikawa]

Sorry for being late to reply.
   Japan was a holiday yesterday.

Avi Kivity wrote:
> On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
>> This patch injects page fault when reading descriptor in
>> load_guest_segment_descriptor() fails with FAULT.
>>
>> Effects of this injection: This function is used by
>> kvm_load_segment_descriptor() which is necessary for the
>> following instructions.
>>   - mov seg,r/m16
>>   - jmp far
>>   - pop ?s
>> This patch makes it possible to emulate the page faults
>> generated by these instructions. But be sure that unless
>> we change the kvm_load_segment_descriptor()'s ret value
>> propagation this patch has no effect.
>>
>>
>> @@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct 
>> kvm_vcpu *vcpu, u16 selector,
>>   {
>>       struct descriptor_table dtable;
>>       u16 index = selector>>  3;
>> +    int ret;
>>
>>       get_segment_descriptor_dtable(vcpu, selector,&dtable);
>>
>> @@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct 
>> kvm_vcpu *vcpu, u16 selector,
>>           kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
>>           return X86EMUL_PROPAGATE_FAULT;
>>       }
>> -    return kvm_read_guest_virt(dtable.base + index*8, seg_desc, 
>> sizeof(*seg_desc), vcpu);
>> +    ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, 
>> sizeof(*seg_desc), vcpu);
>> +    if (ret == X86EMUL_PROPAGATE_FAULT)
>> +        kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
>> +
>> +    return ret;
>>   }
>>    
> 
> If the descriptor table is not aligned, and a descriptor spans two 
> pages, then we might need to inject a page fault at some other address.
> 
> Also, the injection should be done in kvm_read_guest_virt() to avoid 
> duplicating code.
> 
> These instructions however are only emulated in big real mode.  Where 
> did you encounter the need to inject page faults during their emulation?
> 


I did not notice about that, we need not inject page faults for them.
But as Marcelo says in the next mail ...

Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Takuya Yoshikawa]

Gleb Natapov wrote:
> On Wed, Feb 10, 2010 at 06:25:42PM +0200, Avi Kivity wrote:
>> On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
>>> This patch injects page fault when reading descriptor in
>>> load_guest_segment_descriptor() fails with FAULT.
>>>
>>> Effects of this injection: This function is used by
>>> kvm_load_segment_descriptor() which is necessary for the
>>> following instructions.
>>>  - mov seg,r/m16
>>>  - jmp far
>>>  - pop ?s
>>> This patch makes it possible to emulate the page faults
>>> generated by these instructions. But be sure that unless
>>> we change the kvm_load_segment_descriptor()'s ret value
>>> propagation this patch has no effect.
>>>
>>>
>>> @@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>>>  {
>>>  	struct descriptor_table dtable;
>>>  	u16 index = selector>>  3;
>>> +	int ret;
>>>
>>>  	get_segment_descriptor_dtable(vcpu, selector,&dtable);
>>>
>>> @@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>>>  		kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
>>>  		return X86EMUL_PROPAGATE_FAULT;
>>>  	}
>>> -	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>>> +	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>>> +	if (ret == X86EMUL_PROPAGATE_FAULT)
>>> +		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
>>> +
>>> +	return ret;
>>>  }
>> If the descriptor table is not aligned, and a descriptor spans two
>> pages, then we might need to inject a page fault at some other
>> address.
>>
>> Also, the injection should be done in kvm_read_guest_virt() to avoid
>> duplicating code.
>>
> This function is used from inside emulator and I hope one day we will
> make emulator independent of KVM, so it shouldn't inject event directly,
> but rather return them as a result of emulation. Also this function is
> used in kvm_report_emulation_failure() may be not the best place to
> inject #PF.

Agree. Actually I tried to find how to, at which layer, inject page faults,
but could not find any good way in the current situation.

> 
>> These instructions however are only emulated in big real mode.
>> Where did you encounter the need to inject page faults during their
>> emulation?
>>
>> -- 
>> error compiling committee.c: too many arguments to function
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> 			Gleb.

Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Takuya Yoshikawa]

Marcelo Tosatti wrote:
> On Wed, Feb 10, 2010 at 06:25:42PM +0200, Avi Kivity wrote:
>> On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
>>> This patch injects page fault when reading descriptor in
>>> load_guest_segment_descriptor() fails with FAULT.
>>>
>>> Effects of this injection: This function is used by
>>> kvm_load_segment_descriptor() which is necessary for the
>>> following instructions.
>>>  - mov seg,r/m16
>>>  - jmp far
>>>  - pop ?s
>>> This patch makes it possible to emulate the page faults
>>> generated by these instructions. But be sure that unless
>>> we change the kvm_load_segment_descriptor()'s ret value
>>> propagation this patch has no effect.
>>>
>>>
>>> @@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>>>  {
>>>  	struct descriptor_table dtable;
>>>  	u16 index = selector>>  3;
>>> +	int ret;
>>>
>>>  	get_segment_descriptor_dtable(vcpu, selector,&dtable);
>>>
>>> @@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>>>  		kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
>>>  		return X86EMUL_PROPAGATE_FAULT;
>>>  	}
>>> -	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>>> +	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>>> +	if (ret == X86EMUL_PROPAGATE_FAULT)
>>> +		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
>>> +
>>> +	return ret;
>>>  }
>> If the descriptor table is not aligned, and a descriptor spans two
>> pages, then we might need to inject a page fault at some other
>> address.
>>
>> Also, the injection should be done in kvm_read_guest_virt() to avoid
>> duplicating code.
>>
>> These instructions however are only emulated in big real mode.
>> Where did you encounter the need to inject page faults during their
>> emulation?
> 
> Task switch exits also use them, so it should inject faults as
> documented.

Yes, though I did not touch the task switch parts, we will have to do.

Re: [PATCH v2 0/8] Fix x86 emulator's fault propagations

[Takuya Yoshikawa]

Gleb Natapov wrote:
> On Wed, Feb 10, 2010 at 10:45:41AM +0900, Takuya Yoshikawa wrote:
>> My motivation: What I want to achive by this
>> work is to make the basic style of x86 emulator
>> better for the following developments. Actually
>> unless we handle the fault properly, our works
>> implementing each instruction's emulation may
>> end up with no effect.
> Why do you want to emulate every instruction?
> 
> --
> 			Gleb.

No, I do not think we have to emulate every instruction.
I might write something misleading.

But I want to know which instructions or modes are not implemented
yet but have to be implemented in the future, if they may effect
to end users. We may get some questions like "Are there any
instructions which are not supported by KVM?" in the future:
because (advanced)customers will find "FIXME" in the source code.

Re: [PATCH v2 0/8] Fix x86 emulator's fault propagations

[Takuya Yoshikawa]

Gleb Natapov wrote:
> On Wed, Feb 10, 2010 at 06:57:05PM +0200, Avi Kivity wrote:

>> Patches 2-8 look fine, but I'd like Gleb to review them as well.
>>
> I will do, but I already see that they need to be rebased onto the
> latest master.
> 
> --
> 			Gleb.

Thank you! I will rebase my patch and may split this set not to
conflict your latest work.