* [PATCH 1/5] KVM: Use X86EMUL_* to check the return value from read_std
2010-01-28 13:51 [PATCH 0/5] KVM: Cleanups: X86EMUL_* related Takuya Yoshikawa
@ 2010-01-28 13:54 ` Takuya Yoshikawa
2010-01-29 20:46 ` Marcelo Tosatti
2010-01-28 13:56 ` [PATCH 2/5] KVM: These functions should return X86EMUL_* not 0 or 1 or Takuya Yoshikawa
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Takuya Yoshikawa @ 2010-01-28 13:54 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
read_std is one of the x86_emulate_ops. This patch fix the
return value check to use the proper macro.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
arch/x86/kvm/emulate.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 645b245..b124578 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -607,7 +607,7 @@ 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;
@@ -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);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/5] KVM: These functions should return X86EMUL_* not 0 or 1 or ...
2010-01-28 13:51 [PATCH 0/5] KVM: Cleanups: X86EMUL_* related Takuya Yoshikawa
2010-01-28 13:54 ` [PATCH 1/5] KVM: Use X86EMUL_* to check the return value from read_std Takuya Yoshikawa
@ 2010-01-28 13:56 ` Takuya Yoshikawa
2010-01-29 21:18 ` Marcelo Tosatti
2010-01-28 13:59 ` [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to X86EMUL_* values Takuya Yoshikawa
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Takuya Yoshikawa @ 2010-01-28 13:56 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
These functions returns X86EMUL_* or 0 or 1 or ...
This patch fix the conflicts between these values and make
them return one of X86EMUL_* values.
NOTE: In these functions, directly returning the ret value
from the kvm_load_segment_descriptor should have been fixed.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
arch/x86/kvm/emulate.c | 44 +++++++++++++++++++++-----------------------
1 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b124578..9953f5b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -613,14 +613,14 @@ static int do_fetch_insn_byte(struct x86_emulate_ctxt *ctxt,
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;
@@ -1222,10 +1222,11 @@ 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);
+ if (kvm_load_segment_descriptor(ctxt->vcpu, (u16)selector, 1, seg))
+ return X86EMUL_UNHANDLEABLE;
return rc;
}
@@ -1248,7 +1249,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 +1260,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 +1271,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 +1308,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 +1355,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 +1386,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,14 +1397,15 @@ 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);
+ if (kvm_load_segment_descriptor(ctxt->vcpu, (u16)cs, 1, VCPU_SREG_CS))
+ return X86EMUL_UNHANDLEABLE;
return rc;
}
@@ -1460,7 +1458,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)
--
1.6.3.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/5] KVM: These functions should return X86EMUL_* not 0 or 1 or ...
2010-01-28 13:56 ` [PATCH 2/5] KVM: These functions should return X86EMUL_* not 0 or 1 or Takuya Yoshikawa
@ 2010-01-29 21:18 ` Marcelo Tosatti
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Tosatti @ 2010-01-29 21:18 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: avi, kvm
On Thu, Jan 28, 2010 at 10:56:52PM +0900, Takuya Yoshikawa wrote:
> These functions returns X86EMUL_* or 0 or 1 or ...
> This patch fix the conflicts between these values and make
> them return one of X86EMUL_* values.
>
> NOTE: In these functions, directly returning the ret value
> from the kvm_load_segment_descriptor should have been fixed.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
> arch/x86/kvm/emulate.c | 44 +++++++++++++++++++++-----------------------
> 1 files changed, 21 insertions(+), 23 deletions(-)
>
> - rc = kvm_load_segment_descriptor(ctxt->vcpu, (u16)selector, 1, seg);
> + if (kvm_load_segment_descriptor(ctxt->vcpu, (u16)selector, 1, seg))
> + return X86EMUL_UNHANDLEABLE;
Its better to propagate the return value from
kvm_load_segment_descriptor (which can be updated to return accurate
codes, eg PROPAGATE_FAULT if exception has been raised).
Also please send logic changes separately from macro replacement.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to X86EMUL_* values
2010-01-28 13:51 [PATCH 0/5] KVM: Cleanups: X86EMUL_* related Takuya Yoshikawa
2010-01-28 13:54 ` [PATCH 1/5] KVM: Use X86EMUL_* to check the return value from read_std Takuya Yoshikawa
2010-01-28 13:56 ` [PATCH 2/5] KVM: These functions should return X86EMUL_* not 0 or 1 or Takuya Yoshikawa
@ 2010-01-28 13:59 ` Takuya Yoshikawa
2010-01-29 21:21 ` Marcelo Tosatti
2010-01-28 14:01 ` [PATCH 4/5] KVM: load|save_guest_segment_descriptor() should return " Takuya Yoshikawa
2010-01-28 14:03 ` [PATCH 5/5] KVM: Fix the usage of X86EMUL_* values in x86.c Takuya Yoshikawa
4 siblings, 1 reply; 11+ messages in thread
From: Takuya Yoshikawa @ 2010-01-28 13:59 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
This patch differentiate the X86EMUL_* values returned from
X86EMUL_* type functions.
Note: During this work, we noticed some buggy return value
checks in x86_emulate_insn(). See FIXME in this patch.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
arch/x86/kvm/emulate.c | 73 +++++++++++++++++++++++++++++-------------------
1 files changed, 44 insertions(+), 29 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 9953f5b..d49e9de 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1693,7 +1693,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;
@@ -1791,7 +1791,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:
@@ -1810,7 +1810,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:
@@ -1822,7 +1822,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:
@@ -1853,7 +1853,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 */
@@ -1861,7 +1861,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 */
@@ -2002,7 +2002,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 */
@@ -2135,7 +2135,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 +2205,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 +2231,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. */
@@ -2263,8 +2263,18 @@ twobyte_insn:
if (c->modrm_mod != 3 || c->modrm_rm != 1)
goto cannot_emulate;
- rc = kvm_fix_hypercall(ctxt->vcpu);
- if (rc)
+ /* FIXME:
+ * kvm_fix_hypercall() calls emulator_write_emulated()
+ * and if the return value is not X86EMUL_CONTINUE then
+ * returns -EFAULT, otherwise returns X86EMUL_CONTINUE.
+ *
+ * To handle the former case, original code just did
+ * goto done with rc = -EFAULT and passed the
+ * if (X86EMUL_UNHANDLEABLE) check.
+ * Instead of this, we just set rc to X86EMUL_CONTINUE.
+ */
+ rc = X86EMUL_CONTINUE;
+ if (kvm_fix_hypercall(ctxt->vcpu))
goto done;
/* Let the processor re-execute the fixed hypercall */
@@ -2275,7 +2285,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. */
@@ -2285,8 +2295,9 @@ twobyte_insn:
if (c->modrm_mod == 3) {
switch (c->modrm_rm) {
case 1:
- rc = kvm_fix_hypercall(ctxt->vcpu);
- if (rc)
+ /* FIXME: See above */
+ rc = X86EMUL_CONTINUE;
+ if (kvm_fix_hypercall(ctxt->vcpu))
goto done;
break;
default:
@@ -2296,7 +2307,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);
}
@@ -2347,9 +2358,12 @@ 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;
+ /* FIXME: Next line is just for ensuring to pass
+ * the if (rc != X86EMUL_UNHANDLEABLE) test.
+ */
+ rc = X86EMUL_CONTINUE;
c->dst.type = OP_NONE; /* no writeback */
break;
case 0x22: /* mov reg, cr */
@@ -2362,18 +2376,19 @@ 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;
+ /* FIXME: Next line is just for ensuring to pass
+ * the if (rc != X86EMUL_UNHANDLEABLE) test.
+ */
+ 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);
}
@@ -2382,8 +2397,8 @@ 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 {
@@ -2420,7 +2435,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 +2454,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 +2527,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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to X86EMUL_* values
2010-01-28 13:59 ` [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to X86EMUL_* values Takuya Yoshikawa
@ 2010-01-29 21:21 ` Marcelo Tosatti
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Tosatti @ 2010-01-29 21:21 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: avi, kvm
On Thu, Jan 28, 2010 at 10:59:29PM +0900, Takuya Yoshikawa wrote:
> This patch differentiate the X86EMUL_* values returned from
> X86EMUL_* type functions.
>
> Note: During this work, we noticed some buggy return value
> checks in x86_emulate_insn(). See FIXME in this patch.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
> arch/x86/kvm/emulate.c | 73 +++++++++++++++++++++++++++++-------------------
> 1 files changed, 44 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 9953f5b..d49e9de 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
>
> /* Commit shadow register state. */
> @@ -2263,8 +2263,18 @@ twobyte_insn:
> if (c->modrm_mod != 3 || c->modrm_rm != 1)
> goto cannot_emulate;
>
> - rc = kvm_fix_hypercall(ctxt->vcpu);
> - if (rc)
> + /* FIXME:
> + * kvm_fix_hypercall() calls emulator_write_emulated()
> + * and if the return value is not X86EMUL_CONTINUE then
> + * returns -EFAULT, otherwise returns X86EMUL_CONTINUE.
> + *
> + * To handle the former case, original code just did
> + * goto done with rc = -EFAULT and passed the
> + * if (X86EMUL_UNHANDLEABLE) check.
> + * Instead of this, we just set rc to X86EMUL_CONTINUE.
> + */
> + rc = X86EMUL_CONTINUE;
> + if (kvm_fix_hypercall(ctxt->vcpu))
> goto done;
Should fix kvm_fix_hypercall to return X86EMUL_ codes, and send macro
updates separately from logic changes.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/5] KVM: load|save_guest_segment_descriptor() should return X86EMUL_* values
2010-01-28 13:51 [PATCH 0/5] KVM: Cleanups: X86EMUL_* related Takuya Yoshikawa
` (2 preceding siblings ...)
2010-01-28 13:59 ` [PATCH 3/5] KVM: Restrict rc values in x86_emulate_insn to X86EMUL_* values Takuya Yoshikawa
@ 2010-01-28 14:01 ` Takuya Yoshikawa
2010-01-29 21:30 ` Marcelo Tosatti
2010-01-28 14:03 ` [PATCH 5/5] KVM: Fix the usage of X86EMUL_* values in x86.c Takuya Yoshikawa
4 siblings, 1 reply; 11+ messages in thread
From: Takuya Yoshikawa @ 2010-01-28 14:01 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
These two functions should return X86EMUL_* values.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
arch/x86/kvm/x86.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ac8672f..78b8ddb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4663,7 +4663,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
if (dtable.limit < index * 8 + 7) {
kvm_queue_exception_e(vcpu, GP_VECTOR, selector & 0xfffc);
- return 1;
+ return X86EMUL_UNHANDLEABLE;
}
return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
}
@@ -4678,7 +4678,7 @@ static int save_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
get_segment_descriptor_dtable(vcpu, selector, &dtable);
if (dtable.limit < index * 8 + 7)
- return 1;
+ return X86EMUL_UNHANDLEABLE;
return kvm_write_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 4/5] KVM: load|save_guest_segment_descriptor() should return X86EMUL_* values
2010-01-28 14:01 ` [PATCH 4/5] KVM: load|save_guest_segment_descriptor() should return " Takuya Yoshikawa
@ 2010-01-29 21:30 ` Marcelo Tosatti
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Tosatti @ 2010-01-29 21:30 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: avi, kvm
On Thu, Jan 28, 2010 at 11:01:30PM +0900, Takuya Yoshikawa wrote:
> These two functions should return X86EMUL_* values.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
> arch/x86/kvm/x86.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
I think it would be best to improve the return values here (as
documented in kvm_emulate.h).
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ac8672f..78b8ddb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4663,7 +4663,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>
> if (dtable.limit < index * 8 + 7) {
> kvm_queue_exception_e(vcpu, GP_VECTOR, selector & 0xfffc);
> - return 1;
> + return X86EMUL_UNHANDLEABLE;
> }
This should be X86EMUL_PROPAGATE_FAULT so the #GP is injected (and error
not propagated to caller).
> return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> }
> @@ -4678,7 +4678,7 @@ static int save_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
> get_segment_descriptor_dtable(vcpu, selector, &dtable);
>
> if (dtable.limit < index * 8 + 7)
> - return 1;
> + return X86EMUL_UNHANDLEABLE;
> return kvm_write_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
> }
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 5/5] KVM: Fix the usage of X86EMUL_* values in x86.c
2010-01-28 13:51 [PATCH 0/5] KVM: Cleanups: X86EMUL_* related Takuya Yoshikawa
` (3 preceding siblings ...)
2010-01-28 14:01 ` [PATCH 4/5] KVM: load|save_guest_segment_descriptor() should return " Takuya Yoshikawa
@ 2010-01-28 14:03 ` Takuya Yoshikawa
2010-01-29 21:39 ` Marcelo Tosatti
4 siblings, 1 reply; 11+ messages in thread
From: Takuya Yoshikawa @ 2010-01-28 14:03 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
pio_copy_data() and load|save_guest_segment_descriptor()
return X86EMUL_* values. Mixing up these values with 0, 1, ...
may produce unpridictable bugs.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
arch/x86/kvm/x86.c | 27 +++++++++++++++------------
1 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 78b8ddb..67f8231 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3454,7 +3454,6 @@ int complete_pio(struct kvm_vcpu *vcpu)
{
struct kvm_pio_request *io = &vcpu->arch.pio;
long delta;
- int r;
unsigned long val;
if (!io->string) {
@@ -3465,9 +3464,9 @@ int complete_pio(struct kvm_vcpu *vcpu)
}
} else {
if (io->in) {
- r = pio_copy_data(vcpu);
- if (r)
- return r;
+ int ret = pio_copy_data(vcpu);
+ if (ret != X86EMUL_CONTINUE)
+ return 1;
}
delta = 1;
@@ -3567,7 +3566,6 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, int in,
gva_t address, int rep, unsigned port)
{
unsigned now, in_page;
- int ret = 0;
vcpu->run->exit_reason = KVM_EXIT_IO;
vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
@@ -3613,20 +3611,22 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, int in,
if (!vcpu->arch.pio.in) {
/* string PIO write */
- ret = pio_copy_data(vcpu);
+ int ret = pio_copy_data(vcpu);
if (ret == X86EMUL_PROPAGATE_FAULT) {
kvm_inject_gp(vcpu, 0);
return 1;
}
- if (ret == 0 && !pio_string_write(vcpu)) {
+ if (ret == X86EMUL_UNHANDLEABLE)
+ return 1;
+ if (ret == X86EMUL_CONTINUE && !pio_string_write(vcpu)) {
complete_pio(vcpu);
if (vcpu->arch.pio.count == 0)
- ret = 1;
+ return 1;
}
}
/* no string PIO read support yet */
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(kvm_emulate_pio_string);
@@ -4743,7 +4743,8 @@ int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
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))
+ if (load_guest_segment_descriptor(vcpu, selector, &seg_desc)
+ != X86EMUL_CONTINUE)
return 1;
seg_desct_to_kvm_desct(&seg_desc, selector, &kvm_seg);
@@ -4971,10 +4972,12 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason)
/* FIXME: Handle errors. Failure to read either TSS or their
* descriptors should generate a pagefault.
*/
- if (load_guest_segment_descriptor(vcpu, tss_selector, &nseg_desc))
+ if (load_guest_segment_descriptor(vcpu, tss_selector, &nseg_desc)
+ != X86EMUL_CONTINUE)
goto out;
- if (load_guest_segment_descriptor(vcpu, old_tss_sel, &cseg_desc))
+ if (load_guest_segment_descriptor(vcpu, old_tss_sel, &cseg_desc)
+ != X86EMUL_CONTINUE)
goto out;
if (reason != TASK_SWITCH_IRET) {
--
1.6.3.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 5/5] KVM: Fix the usage of X86EMUL_* values in x86.c
2010-01-28 14:03 ` [PATCH 5/5] KVM: Fix the usage of X86EMUL_* values in x86.c Takuya Yoshikawa
@ 2010-01-29 21:39 ` Marcelo Tosatti
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Tosatti @ 2010-01-29 21:39 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: avi, kvm
On Thu, Jan 28, 2010 at 11:03:34PM +0900, Takuya Yoshikawa wrote:
> pio_copy_data() and load|save_guest_segment_descriptor()
> return X86EMUL_* values. Mixing up these values with 0, 1, ...
> may produce unpridictable bugs.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
> arch/x86/kvm/x86.c | 27 +++++++++++++++------------
> 1 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 78b8ddb..67f8231 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3454,7 +3454,6 @@ int complete_pio(struct kvm_vcpu *vcpu)
> {
> struct kvm_pio_request *io = &vcpu->arch.pio;
> long delta;
> - int r;
> unsigned long val;
>
> if (!io->string) {
> @@ -3465,9 +3464,9 @@ int complete_pio(struct kvm_vcpu *vcpu)
> }
> } else {
> if (io->in) {
> - r = pio_copy_data(vcpu);
> - if (r)
> - return r;
> + int ret = pio_copy_data(vcpu);
> + if (ret != X86EMUL_CONTINUE)
> + return 1;
> }
>
> delta = 1;
> @@ -3567,7 +3566,6 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, int in,
> gva_t address, int rep, unsigned port)
> {
> unsigned now, in_page;
> - int ret = 0;
>
> vcpu->run->exit_reason = KVM_EXIT_IO;
> vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
> @@ -3613,20 +3611,22 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, int in,
>
> if (!vcpu->arch.pio.in) {
> /* string PIO write */
> - ret = pio_copy_data(vcpu);
> + int ret = pio_copy_data(vcpu);
> if (ret == X86EMUL_PROPAGATE_FAULT) {
> kvm_inject_gp(vcpu, 0);
> return 1;
> }
> - if (ret == 0 && !pio_string_write(vcpu)) {
> + if (ret == X86EMUL_UNHANDLEABLE)
> + return 1;
> + if (ret == X86EMUL_CONTINUE && !pio_string_write(vcpu)) {
> complete_pio(vcpu);
> if (vcpu->arch.pio.count == 0)
> - ret = 1;
> + return 1;
> }
> }
> /* no string PIO read support yet */
>
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_pio_string);
This function is used by the emulator, and as such should return
X86_EMUL values?
^ permalink raw reply [flat|nested] 11+ messages in thread