[PATCH 1/2] x86 emulator: Fix emulator return values

[PATCH 1/2] x86 emulator: Fix emulator return values

[Mohammed Gamal]

Let the emulator return X86EMUL_* return codes instead of hardcoded values.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/emulate.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c476a67..c718589 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2434,7 +2434,7 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt)
 		break;
 #endif
 	default:
-		return -1;
+		return X86EMUL_UNHANDLEABLE;
 	}
 
 	c->op_bytes = def_op_bytes;
@@ -2531,7 +2531,7 @@ done_prefixes:
 	/* Unrecognised? */
 	if (c->d == 0 || (c->d & Undefined)) {
 		DPRINTF("Cannot emulate %02x\n", c->b);
-		return -1;
+		return X86EMUL_UNHANDLEABLE;
 	}
 
 	if (mode == X86EMUL_MODE_PROT64 && (c->d & Stack))
@@ -2720,11 +2720,11 @@ done_prefixes:
 		/* Special instructions do their own operand decoding. */
 	default:
 		c->dst.type = OP_NONE; /* Disable writeback. */
-		return 0;
+		return X86EMUL_CONTINUE;
 	}
 
 done:
-	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
+	return rc;
 }
 
 int
@@ -3256,7 +3256,7 @@ writeback:
 	ctxt->eip = c->eip;
 
 done:
-	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
+	return rc;
 
 twobyte_insn:
 	switch (c->b) {
@@ -3558,5 +3558,5 @@ twobyte_insn:
 
 cannot_emulate:
 	DPRINTF("Cannot emulate %02x\n", c->b);
-	return -1;
+	return X86EMUL_UNHANDLEABLE;
 }
-- 
1.7.0.4

[PATCH 2/2] x86: Bail out on unemulated instructions

[Mohammed Gamal]

If emulation fails due to the instruction being unemulated. Return immediately
instead of restarting the instruction and infinitely trying to execute it.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/x86.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 416aa0e..a31db44 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4036,6 +4036,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		}
 
 		++vcpu->stat.insn_emulation;
+		if (r == X86EMUL_UNHANDLEABLE)
+			return handle_emulation_failure(vcpu);
+
 		if (r)  {
 			if (reexecute_instruction(vcpu, cr2))
 				return EMULATE_DONE;
@@ -4057,6 +4060,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 restart:
 	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt);
 
+	if (r == X86EMUL_UNHANDLEABLE)
+		return handle_emulation_failure(vcpu);
+
 	if (r) { /* emulation failed */
 		if (reexecute_instruction(vcpu, cr2))
 			return EMULATE_DONE;
-- 
1.7.0.4

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Gleb Natapov]

On Sat, Aug 14, 2010 at 06:51:34PM +0300, Mohammed Gamal wrote:
> If emulation fails due to the instruction being unemulated. Return immediately
> instead of restarting the instruction and infinitely trying to execute it.
> 
This is already handled correctly as far as I can see. Sometimes
instruction should be retried and reexecute_instruction() checks
for that case. If instruction emulation fails in big real mode
re-executing instruction will be useless though, so what should be done
is to make reexecute_instruction() return false if vcpu is in big real
mode and cpu relies on emulation to handle it.
 
> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
> ---
>  arch/x86/kvm/x86.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 416aa0e..a31db44 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4036,6 +4036,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  		}
>  
>  		++vcpu->stat.insn_emulation;
> +		if (r == X86EMUL_UNHANDLEABLE)
> +			return handle_emulation_failure(vcpu);
> +
>  		if (r)  {
>  			if (reexecute_instruction(vcpu, cr2))
>  				return EMULATE_DONE;
> @@ -4057,6 +4060,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  restart:
>  	r = x86_emulate_insn(&vcpu->arch.emulate_ctxt);
>  
> +	if (r == X86EMUL_UNHANDLEABLE)
> +		return handle_emulation_failure(vcpu);
> +
>  	if (r) { /* emulation failed */
>  		if (reexecute_instruction(vcpu, cr2))
>  			return EMULATE_DONE;
> -- 
> 1.7.0.4
> 
> --
> 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 2/2] x86: Bail out on unemulated instructions

[Mohammed Gamal]

On Sun, Aug 15, 2010 at 10:32 AM, Gleb Natapov <gleb@redhat.com> wrote:
> On Sat, Aug 14, 2010 at 06:51:34PM +0300, Mohammed Gamal wrote:
>> If emulation fails due to the instruction being unemulated. Return immediately
>> instead of restarting the instruction and infinitely trying to execute it.
>>
> This is already handled correctly as far as I can see. Sometimes
> instruction should be retried and reexecute_instruction() checks
> for that case. If instruction emulation fails in big real mode
> re-executing instruction will be useless though, so what should be done
> is to make reexecute_instruction() return false if vcpu is in big real
> mode and cpu relies on emulation to handle it.
We don't have a separate mode for big real mode. The emulation modes
we have are real and vm86

>
>> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
>> ---
>>  arch/x86/kvm/x86.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 416aa0e..a31db44 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -4036,6 +4036,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>>               }
>>
>>               ++vcpu->stat.insn_emulation;
>> +             if (r == X86EMUL_UNHANDLEABLE)
>> +                     return handle_emulation_failure(vcpu);
>> +
>>               if (r)  {
>>                       if (reexecute_instruction(vcpu, cr2))
>>                               return EMULATE_DONE;
>> @@ -4057,6 +4060,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>>  restart:
>>       r = x86_emulate_insn(&vcpu->arch.emulate_ctxt);
>>
>> +     if (r == X86EMUL_UNHANDLEABLE)
>> +             return handle_emulation_failure(vcpu);
>> +
>>       if (r) { /* emulation failed */
>>               if (reexecute_instruction(vcpu, cr2))
>>                       return EMULATE_DONE;
>> --
>> 1.7.0.4
>>
>> --
>> 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 2/2] x86: Bail out on unemulated instructions

[Gleb Natapov]

On Sun, Aug 15, 2010 at 03:40:00PM +0300, Mohammed Gamal wrote:
> On Sun, Aug 15, 2010 at 10:32 AM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Sat, Aug 14, 2010 at 06:51:34PM +0300, Mohammed Gamal wrote:
> >> If emulation fails due to the instruction being unemulated. Return immediately
> >> instead of restarting the instruction and infinitely trying to execute it.
> >>
> > This is already handled correctly as far as I can see. Sometimes
> > instruction should be retried and reexecute_instruction() checks
> > for that case. If instruction emulation fails in big real mode
> > re-executing instruction will be useless though, so what should be done
> > is to make reexecute_instruction() return false if vcpu is in big real
> > mode and cpu relies on emulation to handle it.
> We don't have a separate mode for big real mode. The emulation modes
> we have are real and vm86
> 
That doesn't makes the patch right. So we will have to figure something
out.

> >
> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
> >> ---
> >>  arch/x86/kvm/x86.c |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 416aa0e..a31db44 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -4036,6 +4036,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
> >>               }
> >>
> >>               ++vcpu->stat.insn_emulation;
> >> +             if (r == X86EMUL_UNHANDLEABLE)
> >> +                     return handle_emulation_failure(vcpu);
> >> +
> >>               if (r)  {
> >>                       if (reexecute_instruction(vcpu, cr2))
> >>                               return EMULATE_DONE;
> >> @@ -4057,6 +4060,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
> >>  restart:
> >>       r = x86_emulate_insn(&vcpu->arch.emulate_ctxt);
> >>
> >> +     if (r == X86EMUL_UNHANDLEABLE)
> >> +             return handle_emulation_failure(vcpu);
> >> +
> >>       if (r) { /* emulation failed */
> >>               if (reexecute_instruction(vcpu, cr2))
> >>                       return EMULATE_DONE;
> >> --
> >> 1.7.0.4
> >>
> >> --
> >> 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.
> >

--
			Gleb.

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Mohammed Gamal]

2010/8/15 Gleb Natapov <gleb@redhat.com>:
> On Sun, Aug 15, 2010 at 03:40:00PM +0300, Mohammed Gamal wrote:
>> On Sun, Aug 15, 2010 at 10:32 AM, Gleb Natapov <gleb@redhat.com> wrote:
>> > On Sat, Aug 14, 2010 at 06:51:34PM +0300, Mohammed Gamal wrote:
>> >> If emulation fails due to the instruction being unemulated. Return immediately
>> >> instead of restarting the instruction and infinitely trying to execute it.
>> >>
>> > This is already handled correctly as far as I can see. Sometimes
>> > instruction should be retried and reexecute_instruction() checks
>> > for that case. If instruction emulation fails in big real mode
>> > re-executing instruction will be useless though, so what should be done
>> > is to make reexecute_instruction() return false if vcpu is in big real
>> > mode and cpu relies on emulation to handle it.
>> We don't have a separate mode for big real mode. The emulation modes
>> we have are real and vm86
>>
> That doesn't makes the patch right. So we will have to figure something
> out.
True. Can we do it for real mode in general (i.e. X86EMUL_MODE_REAL)?
>
>> >
>> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
>> >> ---
>> >>  arch/x86/kvm/x86.c |    6 ++++++
>> >>  1 files changed, 6 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> >> index 416aa0e..a31db44 100644
>> >> --- a/arch/x86/kvm/x86.c
>> >> +++ b/arch/x86/kvm/x86.c
>> >> @@ -4036,6 +4036,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>> >>               }
>> >>
>> >>               ++vcpu->stat.insn_emulation;
>> >> +             if (r == X86EMUL_UNHANDLEABLE)
>> >> +                     return handle_emulation_failure(vcpu);
>> >> +
>> >>               if (r)  {
>> >>                       if (reexecute_instruction(vcpu, cr2))
>> >>                               return EMULATE_DONE;
>> >> @@ -4057,6 +4060,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>> >>  restart:
>> >>       r = x86_emulate_insn(&vcpu->arch.emulate_ctxt);
>> >>
>> >> +     if (r == X86EMUL_UNHANDLEABLE)
>> >> +             return handle_emulation_failure(vcpu);
>> >> +
>> >>       if (r) { /* emulation failed */
>> >>               if (reexecute_instruction(vcpu, cr2))
>> >>                       return EMULATE_DONE;
>> >> --
>> >> 1.7.0.4
>> >>
>> >> --
>> >> 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.
>> >
>
> --
>                        Gleb.
>

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Gleb Natapov]

On Sun, Aug 15, 2010 at 03:43:15PM +0300, Mohammed Gamal wrote:
> 2010/8/15 Gleb Natapov <gleb@redhat.com>:
> > On Sun, Aug 15, 2010 at 03:40:00PM +0300, Mohammed Gamal wrote:
> >> On Sun, Aug 15, 2010 at 10:32 AM, Gleb Natapov <gleb@redhat.com> wrote:
> >> > On Sat, Aug 14, 2010 at 06:51:34PM +0300, Mohammed Gamal wrote:
> >> >> If emulation fails due to the instruction being unemulated. Return immediately
> >> >> instead of restarting the instruction and infinitely trying to execute it.
> >> >>
> >> > This is already handled correctly as far as I can see. Sometimes
> >> > instruction should be retried and reexecute_instruction() checks
> >> > for that case. If instruction emulation fails in big real mode
> >> > re-executing instruction will be useless though, so what should be done
> >> > is to make reexecute_instruction() return false if vcpu is in big real
> >> > mode and cpu relies on emulation to handle it.
> >> We don't have a separate mode for big real mode. The emulation modes
> >> we have are real and vm86
> >>
> > That doesn't makes the patch right. So we will have to figure something
> > out.
> True. Can we do it for real mode in general (i.e. X86EMUL_MODE_REAL)?
If we flush all shadow pages when moving from paged mode to non paged
checking for X86EMUL_MODE_REAL sounds enough to me, but Avi knows better.
Or we can add is_big_real_mode() callback to x86_ops and implement it in
vmx accordingly.

--
			Gleb.

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Avi Kivity]

  On 08/15/2010 03:49 PM, Gleb Natapov wrote:
>>
>> True. Can we do it for real mode in general (i.e. X86EMUL_MODE_REAL)?
> If we flush all shadow pages when moving from paged mode to non paged
> checking for X86EMUL_MODE_REAL sounds enough to me, but Avi knows better.
> Or we can add is_big_real_mode() callback to x86_ops and implement it in
> vmx accordingly.

Neither are possible.  We can have one cpu in big real mode and others 
in paged mode, so even in real mode we cannot rule out a spurious page 
fault due to shadow write protection.

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

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Gleb Natapov]

On Sun, Aug 15, 2010 at 06:58:06PM +0300, Avi Kivity wrote:
>  On 08/15/2010 03:49 PM, Gleb Natapov wrote:
> >>
> >>True. Can we do it for real mode in general (i.e. X86EMUL_MODE_REAL)?
> >If we flush all shadow pages when moving from paged mode to non paged
> >checking for X86EMUL_MODE_REAL sounds enough to me, but Avi knows better.
> >Or we can add is_big_real_mode() callback to x86_ops and implement it in
> >vmx accordingly.
> 
> Neither are possible.  We can have one cpu in big real mode and
> others in paged mode, so even in real mode we cannot rule out a
> spurious page fault due to shadow write protection.
> 
Correct, just checking X86EMUL_MODE_REAL is not enough due to smp, but
why checking for big real mode will not work? If instruction can't be
emulated while vcpu is in big real mode returning to vcpu is not an option,
so kvm will fail anyway.

--
			Gleb.

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Avi Kivity]

  On 08/15/2010 07:11 PM, Gleb Natapov wrote:
>
>> Neither are possible.  We can have one cpu in big real mode and
>> others in paged mode, so even in real mode we cannot rule out a
>> spurious page fault due to shadow write protection.
>>
> Correct, just checking X86EMUL_MODE_REAL is not enough due to smp, but
> why checking for big real mode will not work? If instruction can't be
> emulated while vcpu is in big real mode returning to vcpu is not an option,
> so kvm will fail anyway.

Right.  I guess we can have an emulation_reason variable which explains 
why we are emulating (unvirtualizable state, mmu page fault, mmio page 
fault, unvirtualizable instruction) and decide accordingly.  But it's a 
lot of work.

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

Re: [PATCH 2/2] x86: Bail out on unemulated instructions

[Avi Kivity]

  On 08/15/2010 03:43 PM, Mohammed Gamal wrote:
> 2010/8/15 Gleb Natapov<gleb@redhat.com>:
>> On Sun, Aug 15, 2010 at 03:40:00PM +0300, Mohammed Gamal wrote:
>>> On Sun, Aug 15, 2010 at 10:32 AM, Gleb Natapov<gleb@redhat.com>  wrote:
>>>> On Sat, Aug 14, 2010 at 06:51:34PM +0300, Mohammed Gamal wrote:
>>>>> If emulation fails due to the instruction being unemulated. Return immediately
>>>>> instead of restarting the instruction and infinitely trying to execute it.
>>>>>
>>>> This is already handled correctly as far as I can see. Sometimes
>>>> instruction should be retried and reexecute_instruction() checks
>>>> for that case. If instruction emulation fails in big real mode
>>>> re-executing instruction will be useless though, so what should be done
>>>> is to make reexecute_instruction() return false if vcpu is in big real
>>>> mode and cpu relies on emulation to handle it.
>>> We don't have a separate mode for big real mode. The emulation modes
>>> we have are real and vm86
>>>
>> That doesn't makes the patch right. So we will have to figure something
>> out.
> True. Can we do it for real mode in general (i.e. X86EMUL_MODE_REAL)?

We can do it conditionally for CPL=0.  That includes real mode (and 
excludes vm86).

However, there's a race involved (see a895e576cfd96).  I don't see how 
we can call handle_emulation_failure() without opening the race again.

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