* [PATCH 1/3] KVM: x86 emulator: Rename variable that shadows another local variable.
@ 2010-08-24 11:30 Gleb Natapov
2010-08-24 11:30 ` [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function Gleb Natapov
2010-08-24 11:30 ` [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context Gleb Natapov
0 siblings, 2 replies; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 11:30 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
arch/x86/kvm/emulate.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 017ae0c..f9f8353 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3393,7 +3393,7 @@ writeback:
&c->dst);
if (c->rep_prefix && (c->d & String)) {
- struct read_cache *rc = &ctxt->decode.io_read;
+ struct read_cache *r = &ctxt->decode.io_read;
register_address_increment(c, &c->regs[VCPU_REGS_RCX], -1);
/* The second termination condition only applies for REPE
* and REPNE. Test if the repeat string operation prefix is
@@ -3413,8 +3413,8 @@ writeback:
* Re-enter guest when pio read ahead buffer is empty or,
* if it is not used, after each 1024 iteration.
*/
- else if ((rc->end == 0 && !(c->regs[VCPU_REGS_RCX] & 0x3ff)) ||
- (rc->end != 0 && rc->end == rc->pos)) {
+ else if ((r->end == 0 && !(c->regs[VCPU_REGS_RCX] & 0x3ff)) ||
+ (r->end != 0 && r->end == r->pos)) {
ctxt->restart = false;
c->eip = ctxt->eip;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function
2010-08-24 11:30 [PATCH 1/3] KVM: x86 emulator: Rename variable that shadows another local variable Gleb Natapov
@ 2010-08-24 11:30 ` Gleb Natapov
2010-08-24 13:11 ` Avi Kivity
2010-08-24 11:30 ` [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context Gleb Natapov
1 sibling, 1 reply; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 11:30 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
arch/x86/kvm/emulate.c | 42 +++++++++++++++++++++++++++++-------------
1 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f9f8353..d34d706 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2921,6 +2921,32 @@ done:
return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
}
+static bool string_inst_completed(struct x86_emulate_ctxt *ctxt)
+{
+ struct decode_cache *c = &ctxt->decode;
+
+ /* All REP prefixes have the same first termination condition */
+ if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0)
+ return true;
+
+ /* The second termination condition only applies for REPE
+ * and REPNE. Test if the repeat string operation prefix is
+ * REPE/REPZ or REPNE/REPNZ and if it's the case it tests the
+ * corresponding termination condition according to:
+ * - if REPE/REPZ and ZF = 0 then done
+ * - if REPNE/REPNZ and ZF = 1 then done
+ */
+ if (((c->b == 0xa6) || (c->b == 0xa7) ||
+ (c->b == 0xae) || (c->b == 0xaf))
+ && (((c->rep_prefix == REPE_PREFIX) &&
+ ((ctxt->eflags & EFLG_ZF) == 0))
+ || ((c->rep_prefix == REPNE_PREFIX) &&
+ ((ctxt->eflags & EFLG_ZF) == EFLG_ZF))))
+ return true;
+
+ return false;
+}
+
int
x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
{
@@ -3395,19 +3421,9 @@ writeback:
if (c->rep_prefix && (c->d & String)) {
struct read_cache *r = &ctxt->decode.io_read;
register_address_increment(c, &c->regs[VCPU_REGS_RCX], -1);
- /* The second termination condition only applies for REPE
- * and REPNE. Test if the repeat string operation prefix is
- * REPE/REPZ or REPNE/REPNZ and if it's the case it tests the
- * corresponding termination condition according to:
- * - if REPE/REPZ and ZF = 0 then done
- * - if REPNE/REPNZ and ZF = 1 then done
- */
- if (((c->b == 0xa6) || (c->b == 0xa7) ||
- (c->b == 0xae) || (c->b == 0xaf))
- && (((c->rep_prefix == REPE_PREFIX) &&
- ((ctxt->eflags & EFLG_ZF) == 0))
- || ((c->rep_prefix == REPNE_PREFIX) &&
- ((ctxt->eflags & EFLG_ZF) == EFLG_ZF))))
+
+
+ if (string_inst_completed(ctxt))
ctxt->restart = false;
/*
* Re-enter guest when pio read ahead buffer is empty or,
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 11:30 [PATCH 1/3] KVM: x86 emulator: Rename variable that shadows another local variable Gleb Natapov
2010-08-24 11:30 ` [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function Gleb Natapov
@ 2010-08-24 11:30 ` Gleb Natapov
2010-08-24 13:13 ` Avi Kivity
1 sibling, 1 reply; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 11:30 UTC (permalink / raw)
To: avi, mtosatti; +Cc: kvm
x86_emulate_insn() will return 1 if instruction can be restarted
without re-entering a guest.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
arch/x86/include/asm/kvm_emulate.h | 1 -
arch/x86/kvm/emulate.c | 47 ++++++++++++++++-------------------
arch/x86/kvm/x86.c | 16 ++++++------
3 files changed, 30 insertions(+), 34 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index f22e5da..5e120a7 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -220,7 +220,6 @@ struct x86_emulate_ctxt {
/* interruptibility state, as a result of execution of STI or MOV SS */
int interruptibility;
- bool restart; /* restart string instruction after writeback */
bool perm_ok; /* do not check permissions if true */
int exception; /* exception that happens during emulation or -1 */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index d34d706..d98fc3b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -438,7 +438,6 @@ static void emulate_exception(struct x86_emulate_ctxt *ctxt, int vec,
ctxt->exception = vec;
ctxt->error_code = error;
ctxt->error_code_valid = valid;
- ctxt->restart = false;
}
static void emulate_gp(struct x86_emulate_ctxt *ctxt, int err)
@@ -2618,9 +2617,6 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt)
struct opcode opcode, *g_mod012, *g_mod3;
struct operand memop = { .type = OP_NONE };
- /* we cannot decode insn before we complete previous rep insn */
- WARN_ON(ctxt->restart);
-
c->eip = ctxt->eip;
c->fetch.start = c->fetch.end = c->eip;
ctxt->cs_base = seg_base(ctxt, ops, VCPU_SREG_CS);
@@ -2977,10 +2973,11 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
}
if (c->rep_prefix && (c->d & String)) {
- ctxt->restart = true;
- /* All REP prefixes have the same first termination condition */
+ /*
+ * counter == 0 termination condition is checked before
+ * instruction execution.
+ */
if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0) {
- ctxt->restart = false;
ctxt->eip = c->eip;
goto done;
}
@@ -3422,26 +3419,26 @@ writeback:
struct read_cache *r = &ctxt->decode.io_read;
register_address_increment(c, &c->regs[VCPU_REGS_RCX], -1);
-
- if (string_inst_completed(ctxt))
- ctxt->restart = false;
- /*
- * Re-enter guest when pio read ahead buffer is empty or,
- * if it is not used, after each 1024 iteration.
- */
- else if ((r->end == 0 && !(c->regs[VCPU_REGS_RCX] & 0x3ff)) ||
- (r->end != 0 && r->end == r->pos)) {
- ctxt->restart = false;
- c->eip = ctxt->eip;
+ if (!string_inst_completed(ctxt)) {
+ /*
+ * Re-enter guest when pio read ahead buffer is empty
+ * or, if it is not used, after each 1024 iteration.
+ */
+ if ((r->end != 0 || c->regs[VCPU_REGS_RCX] & 0x3ff) &&
+ (r->end == 0 || r->end != r->pos)) {
+ /*
+ * Reset read cache. Usually happens before
+ * decode, but since instruction is restarted
+ * we have to do it here.
+ */
+ ctxt->decode.mem_read.end = 0;
+ return 1; /* restart instruction */
+ }
+ goto done; /* skip rip writeback */
}
}
- /*
- * reset read cache here in case string instruction is restared
- * without decoding
- */
- ctxt->decode.mem_read.end = 0;
- if (!ctxt->restart)
- ctxt->eip = c->eip;
+
+ ctxt->eip = c->eip;
done:
return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4e179c5..131f2c8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4066,18 +4066,17 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
restart:
r = x86_emulate_insn(&vcpu->arch.emulate_ctxt);
- if (r) { /* emulation failed */
+ if (r < 0) { /* emulation failed */
if (reexecute_instruction(vcpu, cr2))
return EMULATE_DONE;
return handle_emulation_failure(vcpu);
}
- r = EMULATE_DONE;
-
- if (vcpu->arch.emulate_ctxt.exception >= 0)
+ if (vcpu->arch.emulate_ctxt.exception >= 0) {
inject_emulated_exception(vcpu);
- else if (vcpu->arch.pio.count) {
+ r = EMULATE_DONE;
+ } else if (vcpu->arch.pio.count) {
if (!vcpu->arch.pio.in)
vcpu->arch.pio.count = 0;
r = EMULATE_DO_MMIO;
@@ -4085,8 +4084,10 @@ restart:
if (vcpu->mmio_is_write)
vcpu->mmio_needed = 0;
r = EMULATE_DO_MMIO;
- } else if (vcpu->arch.emulate_ctxt.restart)
+ } else if (r > 0)
goto restart;
+ else
+ r = EMULATE_DONE;
toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
@@ -4909,8 +4910,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
if (!irqchip_in_kernel(vcpu->kvm))
kvm_set_cr8(vcpu, kvm_run->cr8);
- if (vcpu->arch.pio.count || vcpu->mmio_needed ||
- vcpu->arch.emulate_ctxt.restart) {
+ if (vcpu->arch.pio.count || vcpu->mmio_needed) {
if (vcpu->mmio_needed) {
memcpy(vcpu->mmio_data, kvm_run->mmio.data, 8);
vcpu->mmio_read_completed = 1;
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function
2010-08-24 11:30 ` [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function Gleb Natapov
@ 2010-08-24 13:11 ` Avi Kivity
2010-08-24 13:20 ` Gleb Natapov
0 siblings, 1 reply; 14+ messages in thread
From: Avi Kivity @ 2010-08-24 13:11 UTC (permalink / raw)
To: Gleb Natapov; +Cc: mtosatti, kvm
On 08/24/2010 02:30 PM, Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
> arch/x86/kvm/emulate.c | 42 +++++++++++++++++++++++++++++-------------
> 1 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f9f8353..d34d706 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2921,6 +2921,32 @@ done:
> return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
> }
>
> +static bool string_inst_completed(struct x86_emulate_ctxt *ctxt)
s/inst/insn/.
> +{
> + struct decode_cache *c =&ctxt->decode;
> +
> + /* All REP prefixes have the same first termination condition */
> + if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0)
> + return true;
This is checked during the beginning of the instruction, not after
completion. Why is it here? it will just be duplicated.
> +
> + /* The second termination condition only applies for REPE
> + * and REPNE. Test if the repeat string operation prefix is
> + * REPE/REPZ or REPNE/REPNZ and if it's the case it tests the
> + * corresponding termination condition according to:
> + * - if REPE/REPZ and ZF = 0 then done
> + * - if REPNE/REPNZ and ZF = 1 then done
> + */
> + if (((c->b == 0xa6) || (c->b == 0xa7) ||
> + (c->b == 0xae) || (c->b == 0xaf))
> + && (((c->rep_prefix == REPE_PREFIX)&&
> + ((ctxt->eflags& EFLG_ZF) == 0))
> + || ((c->rep_prefix == REPNE_PREFIX)&&
> + ((ctxt->eflags& EFLG_ZF) == EFLG_ZF))))
> + return true;
> +
> + return false;
> +}
> +
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 11:30 ` [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context Gleb Natapov
@ 2010-08-24 13:13 ` Avi Kivity
2010-08-24 13:37 ` Gleb Natapov
0 siblings, 1 reply; 14+ messages in thread
From: Avi Kivity @ 2010-08-24 13:13 UTC (permalink / raw)
To: Gleb Natapov; +Cc: mtosatti, kvm
On 08/24/2010 02:30 PM, Gleb Natapov wrote:
> x86_emulate_insn() will return 1 if instruction can be restarted
> without re-entering a guest.
>
So now we have an undocumented -1/0/1 return code?
Better to have an enum for this.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function
2010-08-24 13:11 ` Avi Kivity
@ 2010-08-24 13:20 ` Gleb Natapov
2010-08-24 13:24 ` Avi Kivity
0 siblings, 1 reply; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 13:20 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Tue, Aug 24, 2010 at 04:11:20PM +0300, Avi Kivity wrote:
> On 08/24/2010 02:30 PM, Gleb Natapov wrote:
> >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >---
> > arch/x86/kvm/emulate.c | 42 +++++++++++++++++++++++++++++-------------
> > 1 files changed, 29 insertions(+), 13 deletions(-)
> >
> >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >index f9f8353..d34d706 100644
> >--- a/arch/x86/kvm/emulate.c
> >+++ b/arch/x86/kvm/emulate.c
> >@@ -2921,6 +2921,32 @@ done:
> > return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
> > }
> >
> >+static bool string_inst_completed(struct x86_emulate_ctxt *ctxt)
>
> s/inst/insn/.
>
> >+{
> >+ struct decode_cache *c =&ctxt->decode;
> >+
> >+ /* All REP prefixes have the same first termination condition */
> >+ if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0)
> >+ return true;
>
> This is checked during the beginning of the instruction, not after
> completion. Why is it here? it will just be duplicated.
>
SDM describes REP instruction algorithm this way:
WHILE CountReg ≠ 0
DO
Service pending interrupts (if any);
Execute associated string instruction;
CountReg ← (CountReg – 1);
IF CountReg = 0
THEN exit WHILE loop; FI;
IF (Repeat prefix is REPZ or REPE) and (ZF = 0)
or (Repeat prefix is REPNZ or REPNE) and (ZF = 1)
THEN exit WHILE loop; FI;
OD;
So CountReg is checked at the beginning and after each iteration.
Practically it will save us one return to a guest and exit back
to emulator at the end of rep instruction (not a big deal).
> >+
> >+ /* The second termination condition only applies for REPE
> >+ * and REPNE. Test if the repeat string operation prefix is
> >+ * REPE/REPZ or REPNE/REPNZ and if it's the case it tests the
> >+ * corresponding termination condition according to:
> >+ * - if REPE/REPZ and ZF = 0 then done
> >+ * - if REPNE/REPNZ and ZF = 1 then done
> >+ */
> >+ if (((c->b == 0xa6) || (c->b == 0xa7) ||
> >+ (c->b == 0xae) || (c->b == 0xaf))
> >+ && (((c->rep_prefix == REPE_PREFIX)&&
> >+ ((ctxt->eflags& EFLG_ZF) == 0))
> >+ || ((c->rep_prefix == REPNE_PREFIX)&&
> >+ ((ctxt->eflags& EFLG_ZF) == EFLG_ZF))))
> >+ return true;
> >+
> >+ return false;
> >+}
> >+
>
> --
> error compiling committee.c: too many arguments to function
--
Gleb.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function
2010-08-24 13:20 ` Gleb Natapov
@ 2010-08-24 13:24 ` Avi Kivity
0 siblings, 0 replies; 14+ messages in thread
From: Avi Kivity @ 2010-08-24 13:24 UTC (permalink / raw)
To: Gleb Natapov; +Cc: mtosatti, kvm
On 08/24/2010 04:20 PM, Gleb Natapov wrote:
>>
>>> +{
>>> + struct decode_cache *c =&ctxt->decode;
>>> +
>>> + /* All REP prefixes have the same first termination condition */
>>> + if (address_mask(c, c->regs[VCPU_REGS_RCX]) == 0)
>>> + return true;
>> This is checked during the beginning of the instruction, not after
>> completion. Why is it here? it will just be duplicated.
>>
> SDM describes REP instruction algorithm this way:
>
> WHILE CountReg ≠ 0
> DO
> Service pending interrupts (if any);
> Execute associated string instruction;
> CountReg ← (CountReg – 1);
> IF CountReg = 0
> THEN exit WHILE loop; FI;
> IF (Repeat prefix is REPZ or REPE) and (ZF = 0)
> or (Repeat prefix is REPNZ or REPNE) and (ZF = 1)
> THEN exit WHILE loop; FI;
> OD;
>
> So CountReg is checked at the beginning and after each iteration.
The second check is meaningless (and ZF checks should be qualified with
the actual instruction).
> Practically it will save us one return to a guest and exit back
> to emulator at the end of rep instruction (not a big deal).
Not even that - if we reenter to the beginning of the rep instruction
the cpu will skip over it without exiting (unless in big real mode with
eigs=1).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 13:13 ` Avi Kivity
@ 2010-08-24 13:37 ` Gleb Natapov
2010-08-24 13:41 ` Avi Kivity
0 siblings, 1 reply; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 13:37 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Tue, Aug 24, 2010 at 04:13:38PM +0300, Avi Kivity wrote:
> On 08/24/2010 02:30 PM, Gleb Natapov wrote:
> >x86_emulate_insn() will return 1 if instruction can be restarted
> >without re-entering a guest.
> >
>
> So now we have an undocumented -1/0/1 return code?
>
> Better to have an enum for this.
>
We already have two. First is X86EMUL_ (not enum but close) for
more or less internal emulator use. Second is EMULATE_* for users of
emulate_instruction() now you want one more enum for communication
between emulate_instruction() and x86_emulate_insn(). Lost in enums.
emulate_instruction() and x86_emulate_insn() are tightly coupled right
now should we define formal interface between them? May be comment will
be enough?
--
Gleb.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 13:37 ` Gleb Natapov
@ 2010-08-24 13:41 ` Avi Kivity
2010-08-24 13:52 ` Gleb Natapov
0 siblings, 1 reply; 14+ messages in thread
From: Avi Kivity @ 2010-08-24 13:41 UTC (permalink / raw)
To: Gleb Natapov; +Cc: mtosatti, kvm
On 08/24/2010 04:37 PM, Gleb Natapov wrote:
> On Tue, Aug 24, 2010 at 04:13:38PM +0300, Avi Kivity wrote:
>> On 08/24/2010 02:30 PM, Gleb Natapov wrote:
>>> x86_emulate_insn() will return 1 if instruction can be restarted
>>> without re-entering a guest.
>>>
>> So now we have an undocumented -1/0/1 return code?
>>
>> Better to have an enum for this.
>>
> We already have two. First is X86EMUL_ (not enum but close) for
> more or less internal emulator use. Second is EMULATE_* for users of
> emulate_instruction() now you want one more enum for communication
> between emulate_instruction() and x86_emulate_insn(). Lost in enums.
> emulate_instruction() and x86_emulate_insn() are tightly coupled right
> now should we define formal interface between them? May be comment will
> be enough?
Can we reuse one or the other? Perhaps with extensions?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 13:41 ` Avi Kivity
@ 2010-08-24 13:52 ` Gleb Natapov
2010-08-24 14:01 ` Avi Kivity
0 siblings, 1 reply; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 13:52 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Tue, Aug 24, 2010 at 04:41:10PM +0300, Avi Kivity wrote:
> On 08/24/2010 04:37 PM, Gleb Natapov wrote:
> >On Tue, Aug 24, 2010 at 04:13:38PM +0300, Avi Kivity wrote:
> >> On 08/24/2010 02:30 PM, Gleb Natapov wrote:
> >>>x86_emulate_insn() will return 1 if instruction can be restarted
> >>>without re-entering a guest.
> >>>
> >>So now we have an undocumented -1/0/1 return code?
> >>
> >>Better to have an enum for this.
> >>
> >We already have two. First is X86EMUL_ (not enum but close) for
> >more or less internal emulator use. Second is EMULATE_* for users of
> >emulate_instruction() now you want one more enum for communication
> >between emulate_instruction() and x86_emulate_insn(). Lost in enums.
> >emulate_instruction() and x86_emulate_insn() are tightly coupled right
> >now should we define formal interface between them? May be comment will
> >be enough?
>
> Can we reuse one or the other? Perhaps with extensions?
>
We can, of course. But for me it looks as arbitrary as -1/0/1 since not
all enum values have meanings to the caller.
--
Gleb.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 13:52 ` Gleb Natapov
@ 2010-08-24 14:01 ` Avi Kivity
2010-08-24 14:06 ` Gleb Natapov
0 siblings, 1 reply; 14+ messages in thread
From: Avi Kivity @ 2010-08-24 14:01 UTC (permalink / raw)
To: Gleb Natapov; +Cc: mtosatti, kvm
On 08/24/2010 04:52 PM, Gleb Natapov wrote:
>
> We can, of course. But for me it looks as arbitrary as -1/0/1 since not
> all enum values have meanings to the caller.
Yeah. -1/0/1's problem is that between reading the callee code and
caller code, I manage to forget what the values mean.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 14:01 ` Avi Kivity
@ 2010-08-24 14:06 ` Gleb Natapov
2010-08-24 14:28 ` Avi Kivity
0 siblings, 1 reply; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 14:06 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Tue, Aug 24, 2010 at 05:01:10PM +0300, Avi Kivity wrote:
> On 08/24/2010 04:52 PM, Gleb Natapov wrote:
> >
> >We can, of course. But for me it looks as arbitrary as -1/0/1 since not
> >all enum values have meanings to the caller.
>
> Yeah. -1/0/1's problem is that between reading the callee code and
> caller code, I manage to forget what the values mean.
>
Luckily we have only one caller of x86_emulate_insn(), so documenting
return values right where function is called should help. :)
--
Gleb.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 14:06 ` Gleb Natapov
@ 2010-08-24 14:28 ` Avi Kivity
2010-08-24 14:46 ` Gleb Natapov
0 siblings, 1 reply; 14+ messages in thread
From: Avi Kivity @ 2010-08-24 14:28 UTC (permalink / raw)
To: Gleb Natapov; +Cc: mtosatti, kvm
On 08/24/2010 05:06 PM, Gleb Natapov wrote:
> On Tue, Aug 24, 2010 at 05:01:10PM +0300, Avi Kivity wrote:
>> On 08/24/2010 04:52 PM, Gleb Natapov wrote:
>>> We can, of course. But for me it looks as arbitrary as -1/0/1 since not
>>> all enum values have meanings to the caller.
>> Yeah. -1/0/1's problem is that between reading the callee code and
>> caller code, I manage to forget what the values mean.
>>
> Luckily we have only one caller of x86_emulate_insn(), so documenting
> return values right where function is called should help. :)
Please use #define instead of /* */.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context.
2010-08-24 14:28 ` Avi Kivity
@ 2010-08-24 14:46 ` Gleb Natapov
0 siblings, 0 replies; 14+ messages in thread
From: Gleb Natapov @ 2010-08-24 14:46 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
On Tue, Aug 24, 2010 at 05:28:12PM +0300, Avi Kivity wrote:
> On 08/24/2010 05:06 PM, Gleb Natapov wrote:
> >On Tue, Aug 24, 2010 at 05:01:10PM +0300, Avi Kivity wrote:
> >> On 08/24/2010 04:52 PM, Gleb Natapov wrote:
> >>>We can, of course. But for me it looks as arbitrary as -1/0/1 since not
> >>>all enum values have meanings to the caller.
> >>Yeah. -1/0/1's problem is that between reading the callee code and
> >>caller code, I manage to forget what the values mean.
> >>
> >Luckily we have only one caller of x86_emulate_insn(), so documenting
> >return values right where function is called should help. :)
>
> Please use #define instead of /* */.
>
Sigh.
--
Gleb.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-08-24 14:46 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-24 11:30 [PATCH 1/3] KVM: x86 emulator: Rename variable that shadows another local variable Gleb Natapov
2010-08-24 11:30 ` [PATCH 2/3] KVM: x86 emulator: move string instruction completion check into separate function Gleb Natapov
2010-08-24 13:11 ` Avi Kivity
2010-08-24 13:20 ` Gleb Natapov
2010-08-24 13:24 ` Avi Kivity
2010-08-24 11:30 ` [PATCH 3/3] KVM: x86 emulator: get rid of "restart" in emulation context Gleb Natapov
2010-08-24 13:13 ` Avi Kivity
2010-08-24 13:37 ` Gleb Natapov
2010-08-24 13:41 ` Avi Kivity
2010-08-24 13:52 ` Gleb Natapov
2010-08-24 14:01 ` Avi Kivity
2010-08-24 14:06 ` Gleb Natapov
2010-08-24 14:28 ` Avi Kivity
2010-08-24 14:46 ` Gleb Natapov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.