[PATCH 0/2] Speedup ins instruction emulation a little

[PATCH 0/2] Speedup ins instruction emulation a little

[Gleb Natapov]

Recent emulator changes fixed many bugs and shortcomings in string io
instructions emulation (ability to do io into mmio region, proper DF
flag handling) but emulation became much slower. Those patches bring
some performance back.

Gleb Natapov (2):
  KVM: x86 emulator: don't update vcpu state if instruction is
    restarted.
  KVM: x86 emulator: check io permissions only once for string pio

 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |    6 ++++++
 arch/x86/kvm/x86.c                 |   32 ++++++++++++++------------------
 3 files changed, 21 insertions(+), 18 deletions(-)

[PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

No need to update vcpu state since instruction is in the middle of the
emulation.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/x86.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 76fbc32..7e5f075 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4057,32 +4057,27 @@ restart:
 		return handle_emulation_failure(vcpu);
 	}
 
-	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
-	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
-	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
-	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
+	r = EMULATE_DONE;
 
-	if (vcpu->arch.emulate_ctxt.exception >= 0) {
+	if (vcpu->arch.emulate_ctxt.exception >= 0)
 		inject_emulated_exception(vcpu);
-		return EMULATE_DONE;
-	}
-
-	if (vcpu->arch.pio.count) {
+	else if (vcpu->arch.pio.count) {
 		if (!vcpu->arch.pio.in)
 			vcpu->arch.pio.count = 0;
-		return EMULATE_DO_MMIO;
-	}
-
-	if (vcpu->mmio_needed) {
+		r = EMULATE_DO_MMIO;
+	} else if (vcpu->mmio_needed) {
 		if (vcpu->mmio_is_write)
 			vcpu->mmio_needed = 0;
-		return EMULATE_DO_MMIO;
-	}
-
-	if (vcpu->arch.emulate_ctxt.restart)
+		r = EMULATE_DO_MMIO;
+	} else if (vcpu->arch.emulate_ctxt.restart)
 		goto restart;
 
-	return EMULATE_DONE;
+	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
+	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
+	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
+	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
+
+	return r;
 }
 EXPORT_SYMBOL_GPL(emulate_instruction);
 
-- 
1.7.1

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 07/29/2010 03:11 PM, Gleb Natapov wrote:
> No need to update vcpu state since instruction is in the middle of the
> emulation.
>
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
>   arch/x86/kvm/x86.c |   31 +++++++++++++------------------
>   1 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 76fbc32..7e5f075 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4057,32 +4057,27 @@ restart:
>   		return handle_emulation_failure(vcpu);
>   	}
>
> -	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> -	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
> -	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
> -	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
> +	r = EMULATE_DONE;
>
> -	if (vcpu->arch.emulate_ctxt.exception>= 0) {
> +	if (vcpu->arch.emulate_ctxt.exception>= 0)
>   		inject_emulated_exception(vcpu);
> -		return EMULATE_DONE;
> -	}
> -
> -	if (vcpu->arch.pio.count) {
> +	else if (vcpu->arch.pio.count) {
>   		if (!vcpu->arch.pio.in)
>   			vcpu->arch.pio.count = 0;
> -		return EMULATE_DO_MMIO;
> -	}
> -
> -	if (vcpu->mmio_needed) {
> +		r = EMULATE_DO_MMIO;
> +	} else if (vcpu->mmio_needed) {
>   		if (vcpu->mmio_is_write)
>   			vcpu->mmio_needed = 0;
> -		return EMULATE_DO_MMIO;
> -	}
> -
> -	if (vcpu->arch.emulate_ctxt.restart)
> +		r = EMULATE_DO_MMIO;
> +	} else if (vcpu->arch.emulate_ctxt.restart)
>   		goto restart;
>
> -	return EMULATE_DONE;
> +	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> +	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
> +	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
> +	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
> +
> +	return r;
>   }
>   EXPORT_SYMBOL_GPL(emulate_instruction);
>

What about kvm-tpr-opt.c?  It uses rip after pio.

It's true that it usually doesn't go through the emulator.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Sat, Jul 31, 2010 at 08:25:13PM +0300, Avi Kivity wrote:
>  On 07/29/2010 03:11 PM, Gleb Natapov wrote:
> >No need to update vcpu state since instruction is in the middle of the
> >emulation.
> >
> >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >---
> >  arch/x86/kvm/x86.c |   31 +++++++++++++------------------
> >  1 files changed, 13 insertions(+), 18 deletions(-)
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 76fbc32..7e5f075 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -4057,32 +4057,27 @@ restart:
> >  		return handle_emulation_failure(vcpu);
> >  	}
> >
> >-	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> >-	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
> >-	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
> >-	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
> >+	r = EMULATE_DONE;
> >
> >-	if (vcpu->arch.emulate_ctxt.exception>= 0) {
> >+	if (vcpu->arch.emulate_ctxt.exception>= 0)
> >  		inject_emulated_exception(vcpu);
> >-		return EMULATE_DONE;
> >-	}
> >-
> >-	if (vcpu->arch.pio.count) {
> >+	else if (vcpu->arch.pio.count) {
> >  		if (!vcpu->arch.pio.in)
> >  			vcpu->arch.pio.count = 0;
> >-		return EMULATE_DO_MMIO;
> >-	}
> >-
> >-	if (vcpu->mmio_needed) {
> >+		r = EMULATE_DO_MMIO;
> >+	} else if (vcpu->mmio_needed) {
> >  		if (vcpu->mmio_is_write)
> >  			vcpu->mmio_needed = 0;
> >-		return EMULATE_DO_MMIO;
> >-	}
> >-
> >-	if (vcpu->arch.emulate_ctxt.restart)
> >+		r = EMULATE_DO_MMIO;
> >+	} else if (vcpu->arch.emulate_ctxt.restart)
> >  		goto restart;
> >
> >-	return EMULATE_DONE;
> >+	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> >+	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
> >+	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
> >+	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
> >+
> >+	return r;
> >  }
> >  EXPORT_SYMBOL_GPL(emulate_instruction);
> >
> 
> What about kvm-tpr-opt.c?  It uses rip after pio.
> 
It uses rip _during_ pio. And pio emulation changes rip
only at the end of emulation.

> It's true that it usually doesn't go through the emulator.
> 
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/01/2010 11:28 AM, Gleb Natapov wrote:
> On Sat, Jul 31, 2010 at 08:25:13PM +0300, Avi Kivity wrote:
>>   On 07/29/2010 03:11 PM, Gleb Natapov wrote:
>>> No need to update vcpu state since instruction is in the middle of the
>>> emulation.
>>>
>>> Signed-off-by: Gleb Natapov<gleb@redhat.com>
>>> ---
>>>   arch/x86/kvm/x86.c |   31 +++++++++++++------------------
>>>   1 files changed, 13 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 76fbc32..7e5f075 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -4057,32 +4057,27 @@ restart:
>>>   		return handle_emulation_failure(vcpu);
>>>   	}
>>>
>>> -	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
>>> -	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
>>> -	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
>>> -	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
>>> +	r = EMULATE_DONE;
>>>
>>> -	if (vcpu->arch.emulate_ctxt.exception>= 0) {
>>> +	if (vcpu->arch.emulate_ctxt.exception>= 0)
>>>   		inject_emulated_exception(vcpu);
>>> -		return EMULATE_DONE;
>>> -	}
>>> -
>>> -	if (vcpu->arch.pio.count) {
>>> +	else if (vcpu->arch.pio.count) {
>>>   		if (!vcpu->arch.pio.in)
>>>   			vcpu->arch.pio.count = 0;
>>> -		return EMULATE_DO_MMIO;
>>> -	}
>>> -
>>> -	if (vcpu->mmio_needed) {
>>> +		r = EMULATE_DO_MMIO;
>>> +	} else if (vcpu->mmio_needed) {
>>>   		if (vcpu->mmio_is_write)
>>>   			vcpu->mmio_needed = 0;
>>> -		return EMULATE_DO_MMIO;
>>> -	}
>>> -
>>> -	if (vcpu->arch.emulate_ctxt.restart)
>>> +		r = EMULATE_DO_MMIO;
>>> +	} else if (vcpu->arch.emulate_ctxt.restart)
>>>   		goto restart;
>>>
>>> -	return EMULATE_DONE;
>>> +	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
>>> +	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
>>> +	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
>>> +	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
>>> +
>>> +	return r;
>>>   }
>>>   EXPORT_SYMBOL_GPL(emulate_instruction);
>>>
>> What about kvm-tpr-opt.c?  It uses rip after pio.
>>
> It uses rip _during_ pio. And pio emulation changes rip
> only at the end of emulation.

But non-emulated pio does a skip_emulated_instruction() immediately (or 
so the code in kvm-tpr-opt.c assumes:

> static void vtpr_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> {
>     CPUState *env = cpu_single_env;
>     uint32_t rip;
>
>     cpu_synchronize_state(env);
>
>     rip = env->eip - 2;
>     write_byte_virt(env, rip, 0x66);
>     write_byte_virt(env, rip + 1, 0x90);

)

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Sun, Aug 01, 2010 at 11:54:38AM +0300, Avi Kivity wrote:
>  On 08/01/2010 11:28 AM, Gleb Natapov wrote:
> >On Sat, Jul 31, 2010 at 08:25:13PM +0300, Avi Kivity wrote:
> >>  On 07/29/2010 03:11 PM, Gleb Natapov wrote:
> >>>No need to update vcpu state since instruction is in the middle of the
> >>>emulation.
> >>>
> >>>Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >>>---
> >>>  arch/x86/kvm/x86.c |   31 +++++++++++++------------------
> >>>  1 files changed, 13 insertions(+), 18 deletions(-)
> >>>
> >>>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >>>index 76fbc32..7e5f075 100644
> >>>--- a/arch/x86/kvm/x86.c
> >>>+++ b/arch/x86/kvm/x86.c
> >>>@@ -4057,32 +4057,27 @@ restart:
> >>>  		return handle_emulation_failure(vcpu);
> >>>  	}
> >>>
> >>>-	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> >>>-	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
> >>>-	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
> >>>-	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
> >>>+	r = EMULATE_DONE;
> >>>
> >>>-	if (vcpu->arch.emulate_ctxt.exception>= 0) {
> >>>+	if (vcpu->arch.emulate_ctxt.exception>= 0)
> >>>  		inject_emulated_exception(vcpu);
> >>>-		return EMULATE_DONE;
> >>>-	}
> >>>-
> >>>-	if (vcpu->arch.pio.count) {
> >>>+	else if (vcpu->arch.pio.count) {
> >>>  		if (!vcpu->arch.pio.in)
> >>>  			vcpu->arch.pio.count = 0;
> >>>-		return EMULATE_DO_MMIO;
> >>>-	}
> >>>-
> >>>-	if (vcpu->mmio_needed) {
> >>>+		r = EMULATE_DO_MMIO;
> >>>+	} else if (vcpu->mmio_needed) {
> >>>  		if (vcpu->mmio_is_write)
> >>>  			vcpu->mmio_needed = 0;
> >>>-		return EMULATE_DO_MMIO;
> >>>-	}
> >>>-
> >>>-	if (vcpu->arch.emulate_ctxt.restart)
> >>>+		r = EMULATE_DO_MMIO;
> >>>+	} else if (vcpu->arch.emulate_ctxt.restart)
> >>>  		goto restart;
> >>>
> >>>-	return EMULATE_DONE;
> >>>+	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
> >>>+	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
> >>>+	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
> >>>+	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
> >>>+
> >>>+	return r;
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(emulate_instruction);
> >>>
> >>What about kvm-tpr-opt.c?  It uses rip after pio.
> >>
> >It uses rip _during_ pio. And pio emulation changes rip
> >only at the end of emulation.
> 
> But non-emulated pio does a skip_emulated_instruction() immediately
> (or so the code in kvm-tpr-opt.c assumes:
> 
Indeed, this is bug in non-emulated pio. But the patch does not change
rip behaviour for emulated pio. vcpu->arch.emulate_ctxt.eip is updated
only at the end of emulation.

> >static void vtpr_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >{
> >    CPUState *env = cpu_single_env;
> >    uint32_t rip;
> >
> >    cpu_synchronize_state(env);
> >
> >    rip = env->eip - 2;
> >    write_byte_virt(env, rip, 0x66);
> >    write_byte_virt(env, rip + 1, 0x90);
> 
> )
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/01/2010 12:01 PM, Gleb Natapov wrote:
>>>
>>> It uses rip _during_ pio. And pio emulation changes rip
>>> only at the end of emulation.
>> But non-emulated pio does a skip_emulated_instruction() immediately
>> (or so the code in kvm-tpr-opt.c assumes:
>>
> Indeed, this is bug in non-emulated pio.

But userspace depends on this bug.

> But the patch does not change
> rip behaviour for emulated pio. vcpu->arch.emulate_ctxt.eip is updated
> only at the end of emulation.

That will lead to failures if the emulator is used for the kvm-tpr-opt 
pio (which may happen with big real mode).


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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Sun, Aug 01, 2010 at 12:14:40PM +0300, Avi Kivity wrote:
>  On 08/01/2010 12:01 PM, Gleb Natapov wrote:
> >>>
> >>>It uses rip _during_ pio. And pio emulation changes rip
> >>>only at the end of emulation.
> >>But non-emulated pio does a skip_emulated_instruction() immediately
> >>(or so the code in kvm-tpr-opt.c assumes:
> >>
> >Indeed, this is bug in non-emulated pio.
> 
> But userspace depends on this bug.
We can fix that, or make it smarter. Look for io instruction at
rip/rip-2 and use rip accordingly for instance.

> 
> >But the patch does not change
> >rip behaviour for emulated pio. vcpu->arch.emulate_ctxt.eip is updated
> >only at the end of emulation.
> 
> That will lead to failures if the emulator is used for the
> kvm-tpr-opt pio (which may happen with big real mode).
> 
IIRC it was always this way in emulator. I'd rather fix userspace than
break emulator.

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/01/2010 12:24 PM, Gleb Natapov wrote:
> On Sun, Aug 01, 2010 at 12:14:40PM +0300, Avi Kivity wrote:
>>   On 08/01/2010 12:01 PM, Gleb Natapov wrote:
>>>>> It uses rip _during_ pio. And pio emulation changes rip
>>>>> only at the end of emulation.
>>>> But non-emulated pio does a skip_emulated_instruction() immediately
>>>> (or so the code in kvm-tpr-opt.c assumes:
>>>>
>>> Indeed, this is bug in non-emulated pio.
>> But userspace depends on this bug.
> We can fix that, or make it smarter. Look for io instruction at
> rip/rip-2 and use rip accordingly for instance.

That requires everyone to update, or suffer major breakage.

>>> But the patch does not change
>>> rip behaviour for emulated pio. vcpu->arch.emulate_ctxt.eip is updated
>>> only at the end of emulation.
>> That will lead to failures if the emulator is used for the
>> kvm-tpr-opt pio (which may happen with big real mode).
>>
> IIRC it was always this way in emulator. I'd rather fix userspace than
> break emulator.

It wasn't a problem because the emulator wasn't (and still isn't) used 
for this.  But it has the potential to break badly once we make 
emulate_invalid_guest_state=1 the default.

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Sun, Aug 01, 2010 at 01:00:11PM +0300, Avi Kivity wrote:
>  On 08/01/2010 12:24 PM, Gleb Natapov wrote:
> >On Sun, Aug 01, 2010 at 12:14:40PM +0300, Avi Kivity wrote:
> >>  On 08/01/2010 12:01 PM, Gleb Natapov wrote:
> >>>>>It uses rip _during_ pio. And pio emulation changes rip
> >>>>>only at the end of emulation.
> >>>>But non-emulated pio does a skip_emulated_instruction() immediately
> >>>>(or so the code in kvm-tpr-opt.c assumes:
> >>>>
> >>>Indeed, this is bug in non-emulated pio.
> >>But userspace depends on this bug.
> >We can fix that, or make it smarter. Look for io instruction at
> >rip/rip-2 and use rip accordingly for instance.
> 
> That requires everyone to update, or suffer major breakage.
> 
They will suffer major breakage when they update to a kvm that calls to
kvm-tpr-opt.c from emulator anyway.

> >>>But the patch does not change
> >>>rip behaviour for emulated pio. vcpu->arch.emulate_ctxt.eip is updated
> >>>only at the end of emulation.
> >>That will lead to failures if the emulator is used for the
> >>kvm-tpr-opt pio (which may happen with big real mode).
> >>
> >IIRC it was always this way in emulator. I'd rather fix userspace than
> >break emulator.
> 
> It wasn't a problem because the emulator wasn't (and still isn't)
> used for this.  But it has the potential to break badly once we make
> emulate_invalid_guest_state=1 the default.
> 
So what can we do about it?

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/01/2010 01:53 PM, Gleb Natapov wrote:
>
>> That requires everyone to update, or suffer major breakage.
>>
> They will suffer major breakage when they update to a kvm that calls to
> kvm-tpr-opt.c from emulator anyway.

Why?

>>> IIRC it was always this way in emulator. I'd rather fix userspace than
>>> break emulator.
>> It wasn't a problem because the emulator wasn't (and still isn't)
>> used for this.  But it has the potential to break badly once we make
>> emulate_invalid_guest_state=1 the default.
>>
> So what can we do about it?
>

Keep the existing behaviour.

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Sun, Aug 01, 2010 at 03:17:10PM +0300, Avi Kivity wrote:
>  On 08/01/2010 01:53 PM, Gleb Natapov wrote:
> >
> >>That requires everyone to update, or suffer major breakage.
> >>
> >They will suffer major breakage when they update to a kvm that calls to
> >kvm-tpr-opt.c from emulator anyway.
> 
> Why?
> 
Because tpr code will be called with wrong rip. Emulator always updated rip at the end
of an instruction emulation in writeback stage.

> >>>IIRC it was always this way in emulator. I'd rather fix userspace than
> >>>break emulator.
> >>It wasn't a problem because the emulator wasn't (and still isn't)
> >>used for this.  But it has the potential to break badly once we make
> >>emulate_invalid_guest_state=1 the default.
> >>
> >So what can we do about it?
> >
> 
> Keep the existing behaviour.
> 
Existing behaviour will cause breakage.

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/01/2010 03:23 PM, Gleb Natapov wrote:
> On Sun, Aug 01, 2010 at 03:17:10PM +0300, Avi Kivity wrote:
>>   On 08/01/2010 01:53 PM, Gleb Natapov wrote:
>>>> That requires everyone to update, or suffer major breakage.
>>>>
>>> They will suffer major breakage when they update to a kvm that calls to
>>> kvm-tpr-opt.c from emulator anyway.
>> Why?
>>
> Because tpr code will be called with wrong rip. Emulator always updated rip at the end
> of an instruction emulation in writeback stage.
>

We can change it before switching enabling e_i_g_s by default.


>>> So what can we do about it?
>>>
>> Keep the existing behaviour.
>>
> Existing behaviour will cause breakage.
>

The existing user-visible behaviour.  The user doesn't know whether the 
emulator is involved or not.

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Sun, Aug 01, 2010 at 03:35:41PM +0300, Avi Kivity wrote:
>  On 08/01/2010 03:23 PM, Gleb Natapov wrote:
> >On Sun, Aug 01, 2010 at 03:17:10PM +0300, Avi Kivity wrote:
> >>  On 08/01/2010 01:53 PM, Gleb Natapov wrote:
> >>>>That requires everyone to update, or suffer major breakage.
> >>>>
> >>>They will suffer major breakage when they update to a kvm that calls to
> >>>kvm-tpr-opt.c from emulator anyway.
> >>Why?
> >>
> >Because tpr code will be called with wrong rip. Emulator always updated rip at the end
> >of an instruction emulation in writeback stage.
> >
> 
> We can change it before switching enabling e_i_g_s by default.
> 
> 
Break emulator? We can't increment rip for all instructions before
emulation since then exception will be injected at incorrect rip.
Adding code that rollbacks rip in case of exception will complicate
things and exception is not the only reason to keep rip pointed to the
instruction. We may want to reenter guest to reexecute it for instance.

> >>>So what can we do about it?
> >>>
> >>Keep the existing behaviour.
> >>
> >Existing behaviour will cause breakage.
> >
> 
> The existing user-visible behaviour.  The user doesn't know whether
> the emulator is involved or not.
> 
When we are going to enable e_i_g_s by default? May be we have enough
time to fix userspace? Too ancient userspace already does not run on recent
kvm. Or may be we can make userspace enable e_i_g_s per guest. This way
userspace that knows it is OK can tell kernel so.

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/01/2010 04:27 PM, Gleb Natapov wrote:
>
> When we are going to enable e_i_g_s by default?

Optimistically, 2.6.37, so six months.

> May be we have enough
> time to fix userspace?

Sure we do, but will users update?

0.12 is mature enough that some users will forget about it and not 
update it.

> Too ancient userspace already does not run on recent
> kvm. Or may be we can make userspace enable e_i_g_s per guest. This way
> userspace that knows it is OK can tell kernel so.

Let's make it the other way round, enable the optimization for userspace 
that declares that it does not make use of rip during emulation 
(kvm-tpr-opt can be changed by queueing a signal and re-entering the 
guest to complete the operation).

Later we can make the optimization unconditional.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Mon, Aug 02, 2010 at 08:04:20AM +0300, Avi Kivity wrote:
>  On 08/01/2010 04:27 PM, Gleb Natapov wrote:
> >
> >When we are going to enable e_i_g_s by default?
> 
> Optimistically, 2.6.37, so six months.
> 
> >May be we have enough
> >time to fix userspace?
> 
> Sure we do, but will users update?
> 
> 0.12 is mature enough that some users will forget about it and not
> update it.
> 
So they will not update kernel too. 0.12/2.6.32 should be mature combo.
And we can add patch to 0.12/0.13 stable to work on newer kernels. But
realistically the problem will occur only if TPR access is done from big
real mode by Windows XP running on old Intel cpus. What are the chances
that this will be a problem in practice?

> >Too ancient userspace already does not run on recent
> >kvm. Or may be we can make userspace enable e_i_g_s per guest. This way
> >userspace that knows it is OK can tell kernel so.
> 
> Let's make it the other way round, enable the optimization for
> userspace that declares that it does not make use of rip during
> emulation (kvm-tpr-opt can be changed by queueing a signal and
> re-entering the guest to complete the operation).
> 
> Later we can make the optimization unconditional.
> 
What do you call "optimization"? e_i_g_s=1? Isn't it the same as I proposed
then?

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/02/2010 10:58 AM, Gleb Natapov wrote:
> On Mon, Aug 02, 2010 at 08:04:20AM +0300, Avi Kivity wrote:
>>   On 08/01/2010 04:27 PM, Gleb Natapov wrote:
>>> When we are going to enable e_i_g_s by default?
>> Optimistically, 2.6.37, so six months.
>>
>>> May be we have enough
>>> time to fix userspace?
>> Sure we do, but will users update?
>>
>> 0.12 is mature enough that some users will forget about it and not
>> update it.
>>
> So they will not update kernel too. 0.12/2.6.32 should be mature combo.
> And we can add patch to 0.12/0.13 stable to work on newer kernels.

We don't know what they'll do.  API stability means we only change 
things to fix bugs.

> But
> realistically the problem will occur only if TPR access is done from big
> real mode by Windows XP running on old Intel cpus. What are the chances
> that this will be a problem in practice?

Windows XP does use big real mode (I think unintentionally, some segment 
registers aren't cleared).

>>> Too ancient userspace already does not run on recent
>>> kvm. Or may be we can make userspace enable e_i_g_s per guest. This way
>>> userspace that knows it is OK can tell kernel so.
>> Let's make it the other way round, enable the optimization for
>> userspace that declares that it does not make use of rip during
>> emulation (kvm-tpr-opt can be changed by queueing a signal and
>> re-entering the guest to complete the operation).
>>
>> Later we can make the optimization unconditional.
>>
> What do you call "optimization"? e_i_g_s=1? Isn't it the same as I proposed
> then?

The optimization is your patch.

Step 1: only enable the optimization if userspace indicates it can 
handle it.
<time passes>
Step 2: drop the backwards compatibility code, always enable the 
optimization.

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Mon, Aug 02, 2010 at 11:03:26AM +0300, Avi Kivity wrote:
>  On 08/02/2010 10:58 AM, Gleb Natapov wrote:
> >On Mon, Aug 02, 2010 at 08:04:20AM +0300, Avi Kivity wrote:
> >>  On 08/01/2010 04:27 PM, Gleb Natapov wrote:
> >>>When we are going to enable e_i_g_s by default?
> >>Optimistically, 2.6.37, so six months.
> >>
> >>>May be we have enough
> >>>time to fix userspace?
> >>Sure we do, but will users update?
> >>
> >>0.12 is mature enough that some users will forget about it and not
> >>update it.
> >>
> >So they will not update kernel too. 0.12/2.6.32 should be mature combo.
> >And we can add patch to 0.12/0.13 stable to work on newer kernels.
> 
> We don't know what they'll do.  API stability means we only change
> things to fix bugs.
Is this API documented? Do we guaranty somewhere anywhere that rip during io
point past the instruction? I think it should be documented that cpu
state cannot be accessed during io emulation. And we can preserve old
behaviour for old guests by disabling e_i_g_s for them.

> 
> >But
> >realistically the problem will occur only if TPR access is done from big
> >real mode by Windows XP running on old Intel cpus. What are the chances
> >that this will be a problem in practice?
> 
> Windows XP does use big real mode (I think unintentionally, some
> segment registers aren't cleared).
How it works now then? If it works because Windows XP doesn't
realize it runs in big real mode so it doesn't actually access past
segment limit why starting emulating it? Boot will take much more time
without any gain. And finally does it access TPR while running in big real
mode?

> 
> >>>Too ancient userspace already does not run on recent
> >>>kvm. Or may be we can make userspace enable e_i_g_s per guest. This way
> >>>userspace that knows it is OK can tell kernel so.
> >>Let's make it the other way round, enable the optimization for
> >>userspace that declares that it does not make use of rip during
> >>emulation (kvm-tpr-opt can be changed by queueing a signal and
> >>re-entering the guest to complete the operation).
> >>
> >>Later we can make the optimization unconditional.
> >>
> >What do you call "optimization"? e_i_g_s=1? Isn't it the same as I proposed
> >then?
> 
> The optimization is your patch.
> 
I think there is misunderstanding here. My patch does not change
anything in this regards. If io exit to userspace is done from emulator
rip will point to io instruction with or without my patch and it was always
this way.

> Step 1: only enable the optimization if userspace indicates it can
> handle it.
> <time passes>
> Step 2: drop the backwards compatibility code, always enable the
> optimization.
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/02/2010 11:17 AM, Gleb Natapov wrote:
>
>> We don't know what they'll do.  API stability means we only change
>> things to fix bugs.
> Is this API documented? Do we guaranty somewhere anywhere that rip during io
> point past the instruction? I think it should be documented that cpu
> state cannot be accessed during io emulation.

The user code was written before the documentation.

> And we can preserve old
> behaviour for old guests by disabling e_i_g_s for them.

But the user will see a crash first and report a bug.  That's not the 
experience we want users to have.

>> Windows XP does use big real mode (I think unintentionally, some
>> segment registers aren't cleared).
> How it works now then? If it works because Windows XP doesn't
> realize it runs in big real mode so it doesn't actually access past
> segment limit why starting emulating it?

IIRC it leaves fs and gs pointing to large segments, but it never 
accesses them.  Since we can't tell whether the guest will use those 
segments, we can't avoid emulating big real mode.  Right now most things 
work, but that's because we hacked around everything.

> Boot will take much more time
> without any gain.

The gain is correctness.

> And finally does it access TPR while running in big real
> mode?

I don't think so.


>>> What do you call "optimization"? e_i_g_s=1? Isn't it the same as I proposed
>>> then?
>> The optimization is your patch.
>>
> I think there is misunderstanding here. My patch does not change
> anything in this regards. If io exit to userspace is done from emulator
> rip will point to io instruction with or without my patch and it was always
> this way.

In that case the whole thing is moot.  When we set eigs=1 we'll have to 
test Windows XP and hack around it if needed.

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Mon, Aug 02, 2010 at 11:24:14AM +0300, Avi Kivity wrote:
>  On 08/02/2010 11:17 AM, Gleb Natapov wrote:
> >
> >>We don't know what they'll do.  API stability means we only change
> >>things to fix bugs.
> >Is this API documented? Do we guaranty somewhere anywhere that rip during io
> >point past the instruction? I think it should be documented that cpu
> >state cannot be accessed during io emulation.
> 
> The user code was written before the documentation.
> 
We did it with unmapped pages in the middles of the slot recently.

> >And we can preserve old
> >behaviour for old guests by disabling e_i_g_s for them.
> 
> But the user will see a crash first and report a bug.  That's not
> the experience we want users to have.
Definitely. That is why I propose enabling e_i_g_s only if qemu
acknowledge that it can use it properly.

> 
> >>Windows XP does use big real mode (I think unintentionally, some
> >>segment registers aren't cleared).
> >How it works now then? If it works because Windows XP doesn't
> >realize it runs in big real mode so it doesn't actually access past
> >segment limit why starting emulating it?
> 
> IIRC it leaves fs and gs pointing to large segments, but it never
> accesses them.  Since we can't tell whether the guest will use those
> segments, we can't avoid emulating big real mode.  Right now most
> things work, but that's because we hacked around everything.
> 
We have logic in TPR patching code that tries to detect WindowsXP guest
and if XP is detected it enables vapic. We can disable e_i_g_s if vapic
is enabled.

> >Boot will take much more time
> >without any gain.
> 
> The gain is correctness.
> 
Agree. Worthy goal.

> >And finally does it access TPR while running in big real
> >mode?
> 
> I don't think so.
> 
> 
> >>>What do you call "optimization"? e_i_g_s=1? Isn't it the same as I proposed
> >>>then?
> >>The optimization is your patch.
> >>
> >I think there is misunderstanding here. My patch does not change
> >anything in this regards. If io exit to userspace is done from emulator
> >rip will point to io instruction with or without my patch and it was always
> >this way.
> 
> In that case the whole thing is moot.  When we set eigs=1 we'll have
> to test Windows XP and hack around it if needed.
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/02/2010 11:34 AM, Gleb Natapov wrote:
> On Mon, Aug 02, 2010 at 11:24:14AM +0300, Avi Kivity wrote:
>>   On 08/02/2010 11:17 AM, Gleb Natapov wrote:
>>>> We don't know what they'll do.  API stability means we only change
>>>> things to fix bugs.
>>> Is this API documented? Do we guaranty somewhere anywhere that rip during io
>>> point past the instruction? I think it should be documented that cpu
>>> state cannot be accessed during io emulation.
>> The user code was written before the documentation.
>>
> We did it with unmapped pages in the middles of the slot recently.

What guests did we break?

>> IIRC it leaves fs and gs pointing to large segments, but it never
>> accesses them.  Since we can't tell whether the guest will use those
>> segments, we can't avoid emulating big real mode.  Right now most
>> things work, but that's because we hacked around everything.
>>
> We have logic in TPR patching code that tries to detect WindowsXP guest
> and if XP is detected it enables vapic. We can disable e_i_g_s if vapic
> is enabled.

That code is in userspace.  If we can change userspace, the whole 
problem is gone.

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

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Gleb Natapov]

On Mon, Aug 02, 2010 at 11:54:58AM +0300, Avi Kivity wrote:
>  On 08/02/2010 11:34 AM, Gleb Natapov wrote:
> >On Mon, Aug 02, 2010 at 11:24:14AM +0300, Avi Kivity wrote:
> >>  On 08/02/2010 11:17 AM, Gleb Natapov wrote:
> >>>>We don't know what they'll do.  API stability means we only change
> >>>>things to fix bugs.
> >>>Is this API documented? Do we guaranty somewhere anywhere that rip during io
> >>>point past the instruction? I think it should be documented that cpu
> >>>state cannot be accessed during io emulation.
> >>The user code was written before the documentation.
> >>
> >We did it with unmapped pages in the middles of the slot recently.
> 
> What guests did we break?
> 
The one that uses device assignment with old qemu-kvm userspace. Old
qemu-kvm copied assigned card's ROM into memory and made it read
only (mprotect(RO)) to get MMIO exists on it. Even older device
assignment code unmapped one page in the middle of a slot to get mmio
exist if the page is accessed (and this breaks if mmap decides to mmap
something else there).

> >>IIRC it leaves fs and gs pointing to large segments, but it never
> >>accesses them.  Since we can't tell whether the guest will use those
> >>segments, we can't avoid emulating big real mode.  Right now most
> >>things work, but that's because we hacked around everything.
> >>
> >We have logic in TPR patching code that tries to detect WindowsXP guest
> >and if XP is detected it enables vapic. We can disable e_i_g_s if vapic
> >is enabled.
> 
> That code is in userspace.  If we can change userspace, the whole
> problem is gone.
Userspace calls kvm_enable_vapic() to let kernel know that vapic is
enabled.

--
			Gleb.

Re: [PATCH 1/2] KVM: x86 emulator: don't update vcpu state if instruction is restarted.

[Avi Kivity]

  On 08/02/2010 12:05 PM, Gleb Natapov wrote:
>>>
>>> We did it with unmapped pages in the middles of the slot recently.
>> What guests did we break?
>>
> The one that uses device assignment with old qemu-kvm userspace. Old
> qemu-kvm copied assigned card's ROM into memory and made it read
> only (mprotect(RO)) to get MMIO exists on it. Even older device
> assignment code unmapped one page in the middle of a slot to get mmio
> exist if the page is accessed (and this breaks if mmap decides to mmap
> something else there).

That's fine, device assignment is specialized enough.  I'm more worried 
about everyday workloads.

>>>> IIRC it leaves fs and gs pointing to large segments, but it never
>>>> accesses them.  Since we can't tell whether the guest will use those
>>>> segments, we can't avoid emulating big real mode.  Right now most
>>>> things work, but that's because we hacked around everything.
>>>>
>>> We have logic in TPR patching code that tries to detect WindowsXP guest
>>> and if XP is detected it enables vapic. We can disable e_i_g_s if vapic
>>> is enabled.
>> That code is in userspace.  If we can change userspace, the whole
>> problem is gone.
> Userspace calls kvm_enable_vapic() to let kernel know that vapic is
> enabled.

This happens before the vapic is detected.

In fact, this particular port is during option rom initialization; I 
don't think it's in big real mode.

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

[PATCH 2/2] KVM: x86 emulator: check io permissions only once for string pio

[Gleb Natapov]

Do not recheck io permission on every iteration.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |    6 ++++++
 arch/x86/kvm/x86.c                 |    1 +
 3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 51cfd73..ded05cf 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -216,6 +216,7 @@ struct x86_emulate_ctxt {
 	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 */
 	u32 error_code; /* error code for exception */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b38bd8b..3996fb0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2161,9 +2161,15 @@ static bool emulator_io_permited(struct x86_emulate_ctxt *ctxt,
 				 struct x86_emulate_ops *ops,
 				 u16 port, u16 len)
 {
+	if (ctxt->perm_ok)
+		return true;
+
 	if (emulator_bad_iopl(ctxt, ops))
 		if (!emulator_io_port_access_allowed(ctxt, ops, port, len))
 			return false;
+
+	ctxt->perm_ok = true;
+
 	return true;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7e5f075..de54582 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3997,6 +3997,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		memcpy(c->regs, vcpu->arch.regs, sizeof c->regs);
 		vcpu->arch.emulate_ctxt.interruptibility = 0;
 		vcpu->arch.emulate_ctxt.exception = -1;
+		vcpu->arch.emulate_ctxt.perm_ok = false;
 
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
 		trace_kvm_emulate_insn_start(vcpu);
-- 
1.7.1

Re: [PATCH 2/2] KVM: x86 emulator: check io permissions only once for string pio

[Marcelo Tosatti]

On Thu, Jul 29, 2010 at 03:11:53PM +0300, Gleb Natapov wrote:
> Do not recheck io permission on every iteration.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |    1 +
>  arch/x86/kvm/emulate.c             |    6 ++++++
>  arch/x86/kvm/x86.c                 |    1 +
>  3 files changed, 8 insertions(+), 0 deletions(-)

Applied 1, please regenerate patch 2.

Re: [PATCH 0/2] Speedup ins instruction emulation a little

[Avi Kivity]

  On 07/29/2010 03:11 PM, Gleb Natapov wrote:
> Recent emulator changes fixed many bugs and shortcomings in string io
> instructions emulation (ability to do io into mmio region, proper DF
> flag handling) but emulation became much slower. Those patches bring
> some performance back.
>

Looks good.

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