[PATCH 1/5] KVM: mmio_fault_cr2 is not used.

[PATCH 1/5] KVM: mmio_fault_cr2 is not used.

[Gleb Natapov]

Remove unused variable mmio_fault_cr2.

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

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bd57639..c1a7f54 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -358,7 +358,6 @@ struct kvm_vcpu_arch {
 	struct fpu guest_fpu;
 	u64 xcr0;
 
-	gva_t mmio_fault_cr2;
 	struct kvm_pio_request pio;
 	void *pio_data;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1d5a7f4..b568779 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4504,7 +4504,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 	struct decode_cache *c = &vcpu->arch.emulate_ctxt.decode;
 
 	kvm_clear_exception_queue(vcpu);
-	vcpu->arch.mmio_fault_cr2 = cr2;
+
 	/*
 	 * TODO: fix emulate.c to use guest_read/write_register
 	 * instead of direct ->regs accesses, can save hundred cycles
-- 
1.7.2.3

[PATCH 2/5] KVM: emulator: sysexit should #GP in non protected mode.

[Gleb Natapov]

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3e8b4ab..eb4c7eb 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1973,6 +1973,8 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 		cs.d = 0;
 		cs.l = 1;
 		break;
+	default:
+		return emulate_gp(ctxt, 0);
 	}
 	cs_sel |= SELECTOR_RPL_MASK;
 	ss_sel |= SELECTOR_RPL_MASK;
-- 
1.7.2.3

[PATCH 3/5] KVM: emulator: Propagate fault in far jump emulation.

[Gleb Natapov]

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index eb4c7eb..4f95249 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3876,7 +3876,8 @@ special_insn:
 	jump_far:
 		memcpy(&sel, c->src.valptr + c->op_bytes, 2);
 
-		if (load_segment_descriptor(ctxt, ops, sel, VCPU_SREG_CS))
+		rc = load_segment_descriptor(ctxt, ops, sel, VCPU_SREG_CS);
+		if (rc != X86EMUL_CONTINUE)
 			goto done;
 
 		c->eip = 0;
-- 
1.7.2.3

[PATCH 4/5] KVM: Fix compound mmio.

[Gleb Natapov]

mmio_index should be taken into account when copying data from
userspace.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b568779..609c7ab 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
 	if (vcpu->mmio_needed) {
 		vcpu->mmio_needed = 0;
 		if (!vcpu->mmio_is_write)
-			memcpy(vcpu->mmio_data, run->mmio.data, 8);
+			memcpy(vcpu->mmio_data + vcpu->mmio_index,
+			       run->mmio.data, 8);
 		vcpu->mmio_index += 8;
 		if (vcpu->mmio_index < vcpu->mmio_size) {
 			run->exit_reason = KVM_EXIT_MMIO;
-- 
1.7.2.3

[PATCH 5/5] KVM: call cache_all_regs() only once during instruction emulation.

[Gleb Natapov]

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 609c7ab..efe196c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4404,6 +4404,12 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
 	struct decode_cache *c = &vcpu->arch.emulate_ctxt.decode;
 	int cs_db, cs_l;
 
+	/*
+	 * TODO: fix emulate.c to use guest_read/write_register
+	 * instead of direct ->regs accesses, can save hundred cycles
+	 * on Intel for instructions that don't read/change RSP, for
+	 * for example.
+	 */
 	cache_all_regs(vcpu);
 
 	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
@@ -4505,14 +4511,6 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 
 	kvm_clear_exception_queue(vcpu);
 
-	/*
-	 * TODO: fix emulate.c to use guest_read/write_register
-	 * instead of direct ->regs accesses, can save hundred cycles
-	 * on Intel for instructions that don't read/change RSP, for
-	 * for example.
-	 */
-	cache_all_regs(vcpu);
-
 	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
 		init_emulate_ctxt(vcpu);
 		vcpu->arch.emulate_ctxt.interruptibility = 0;
-- 
1.7.2.3

Re: [PATCH 2/5] KVM: emulator: sysexit should #GP in non protected mode.

[Avi Kivity]

On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
>   arch/x86/kvm/emulate.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 3e8b4ab..eb4c7eb 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1973,6 +1973,8 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
>   		cs.d = 0;
>   		cs.l = 1;
>   		break;
> +	default:
> +		return emulate_gp(ctxt, 0);
>   	}

In addition, looks like the instruction is valid in 16-bit protected mode.

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

Re: [PATCH 4/5] KVM: Fix compound mmio.

[Avi Kivity]

On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> mmio_index should be taken into account when copying data from
> userspace.
>
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
>   arch/x86/kvm/x86.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b568779..609c7ab 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
>   	if (vcpu->mmio_needed) {
>   		vcpu->mmio_needed = 0;
>   		if (!vcpu->mmio_is_write)
> -			memcpy(vcpu->mmio_data, run->mmio.data, 8);
> +			memcpy(vcpu->mmio_data + vcpu->mmio_index,
> +			       run->mmio.data, 8);
>   		vcpu->mmio_index += 8;
>   		if (vcpu->mmio_index<  vcpu->mmio_size) {
>   			run->exit_reason = KVM_EXIT_MMIO;

Interesting, the code passed the emulator.flat sse test.  Does it now?

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

Re: [PATCH 4/5] KVM: Fix compound mmio.

[Gleb Natapov]

On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote:
> On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> >mmio_index should be taken into account when copying data from
> >userspace.
> >
> >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >---
> >  arch/x86/kvm/x86.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index b568779..609c7ab 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
> >  	if (vcpu->mmio_needed) {
> >  		vcpu->mmio_needed = 0;
> >  		if (!vcpu->mmio_is_write)
> >-			memcpy(vcpu->mmio_data, run->mmio.data, 8);
> >+			memcpy(vcpu->mmio_data + vcpu->mmio_index,
> >+			       run->mmio.data, 8);
> >  		vcpu->mmio_index += 8;
> >  		if (vcpu->mmio_index<  vcpu->mmio_size) {
> >  			run->exit_reason = KVM_EXIT_MMIO;
> 
> Interesting, the code passed the emulator.flat sse test.  Does it now?
> 
It pass now and before. Probably by chance. But if I change read_emulated() to do

 int n = min(size, (unsigned)KVM_MMIO_SIZE);

instead of

 int n = min(size, 8u);

emulator.flat fails to emulate far jump instruction.

--
			Gleb.

Re: [PATCH 2/5] KVM: emulator: sysexit should #GP in non protected mode.

[Gleb Natapov]

On Tue, Apr 12, 2011 at 03:17:51PM +0300, Avi Kivity wrote:
> On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >---
> >  arch/x86/kvm/emulate.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >index 3e8b4ab..eb4c7eb 100644
> >--- a/arch/x86/kvm/emulate.c
> >+++ b/arch/x86/kvm/emulate.c
> >@@ -1973,6 +1973,8 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
> >  		cs.d = 0;
> >  		cs.l = 1;
> >  		break;
> >+	default:
> >+		return emulate_gp(ctxt, 0);
> >  	}
> 
> In addition, looks like the instruction is valid in 16-bit protected mode.
> 
Actually we already check for real mode and vm86 at the start of the
function, so the patch is incorrect. Will have to fix compile warning in
some other way.

--
			Gleb.

Re: [PATCH 4/5] KVM: Fix compound mmio.

[Avi Kivity]

On 04/12/2011 03:22 PM, Gleb Natapov wrote:
> On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote:
> >  On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> >  >mmio_index should be taken into account when copying data from
> >  >userspace.
> >  >
> >  >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >  >---
> >  >   arch/x86/kvm/x86.c |    3 ++-
> >  >   1 files changed, 2 insertions(+), 1 deletions(-)
> >  >
> >  >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >  >index b568779..609c7ab 100644
> >  >--- a/arch/x86/kvm/x86.c
> >  >+++ b/arch/x86/kvm/x86.c
> >  >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
> >  >   	if (vcpu->mmio_needed) {
> >  >   		vcpu->mmio_needed = 0;
> >  >   		if (!vcpu->mmio_is_write)
> >  >-			memcpy(vcpu->mmio_data, run->mmio.data, 8);
> >  >+			memcpy(vcpu->mmio_data + vcpu->mmio_index,
> >  >+			       run->mmio.data, 8);
> >  >   		vcpu->mmio_index += 8;
> >  >   		if (vcpu->mmio_index<   vcpu->mmio_size) {
> >  >   			run->exit_reason = KVM_EXIT_MMIO;
> >
> >  Interesting, the code passed the emulator.flat sse test.  Does it now?
> >
> It pass now and before. Probably by chance.

I don't understand how.  I explicitly set the values so that it would 
fail in that case.

Can you patch the test to fail with the current code?

> But if I change read_emulated() to do
>
>   int n = min(size, (unsigned)KVM_MMIO_SIZE);
>
> instead of
>
>   int n = min(size, 8u);
>
> emulator.flat fails to emulate far jump instruction.

Ouch, looks like we have the multi-transaction support in two places.  I 
guess this is what made sse mmio work.

Not sure what we should do (patch is fine, question is how to resolve 
the duplication).

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

Re: [PATCH 2/5] KVM: emulator: sysexit should #GP in non protected mode.

[Avi Kivity]

On 04/12/2011 03:23 PM, Gleb Natapov wrote:
> On Tue, Apr 12, 2011 at 03:17:51PM +0300, Avi Kivity wrote:
> >  On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> >  >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >  >---
> >  >   arch/x86/kvm/emulate.c |    2 ++
> >  >   1 files changed, 2 insertions(+), 0 deletions(-)
> >  >
> >  >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >  >index 3e8b4ab..eb4c7eb 100644
> >  >--- a/arch/x86/kvm/emulate.c
> >  >+++ b/arch/x86/kvm/emulate.c
> >  >@@ -1973,6 +1973,8 @@ emulate_sysexit(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
> >  >   		cs.d = 0;
> >  >   		cs.l = 1;
> >  >   		break;
> >  >+	default:
> >  >+		return emulate_gp(ctxt, 0);
> >  >   	}
> >
> >  In addition, looks like the instruction is valid in 16-bit protected mode.
> >
> Actually we already check for real mode and vm86 at the start of the
> function, so the patch is incorrect. Will have to fix compile warning in
> some other way.
>

Right.  Note the check for vm86 is unneeded, since we have the Priv tag 
set and vm86 implies cpl 3.

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

Re: [PATCH 4/5] KVM: Fix compound mmio.

[Gleb Natapov]

On Tue, Apr 12, 2011 at 03:27:40PM +0300, Avi Kivity wrote:
> On 04/12/2011 03:22 PM, Gleb Natapov wrote:
> >On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote:
> >>  On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> >>  >mmio_index should be taken into account when copying data from
> >>  >userspace.
> >>  >
> >>  >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >>  >---
> >>  >   arch/x86/kvm/x86.c |    3 ++-
> >>  >   1 files changed, 2 insertions(+), 1 deletions(-)
> >>  >
> >>  >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >>  >index b568779..609c7ab 100644
> >>  >--- a/arch/x86/kvm/x86.c
> >>  >+++ b/arch/x86/kvm/x86.c
> >>  >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
> >>  >   	if (vcpu->mmio_needed) {
> >>  >   		vcpu->mmio_needed = 0;
> >>  >   		if (!vcpu->mmio_is_write)
> >>  >-			memcpy(vcpu->mmio_data, run->mmio.data, 8);
> >>  >+			memcpy(vcpu->mmio_data + vcpu->mmio_index,
> >>  >+			       run->mmio.data, 8);
> >>  >   		vcpu->mmio_index += 8;
> >>  >   		if (vcpu->mmio_index<   vcpu->mmio_size) {
> >>  >   			run->exit_reason = KVM_EXIT_MMIO;
> >>
> >>  Interesting, the code passed the emulator.flat sse test.  Does it now?
> >>
> >It pass now and before. Probably by chance.
> 
> I don't understand how.  I explicitly set the values so that it
> would fail in that case.
> 
> Can you patch the test to fail with the current code?
> 
If I understand correctly you've already found the explanation why
test case worked?

> >But if I change read_emulated() to do
> >
> >  int n = min(size, (unsigned)KVM_MMIO_SIZE);
> >
> >instead of
> >
> >  int n = min(size, 8u);
> >
> >emulator.flat fails to emulate far jump instruction.
> 
> Ouch, looks like we have the multi-transaction support in two
> places.  I guess this is what made sse mmio work.
> 
> Not sure what we should do (patch is fine, question is how to
> resolve the duplication).
> 
Multi-transaction works faster in complete_mmio, so read_emulated()
should use it by doing int n = min(size, (unsigned)KVM_MMIO_SIZE).
Other code in read_emulated() is still needed since it provides read
re-play for multiple mmio reads during instruction emulation.

--
			Gleb.

Re: [PATCH 4/5] KVM: Fix compound mmio.

[Avi Kivity]

On 04/12/2011 03:34 PM, Gleb Natapov wrote:
> On Tue, Apr 12, 2011 at 03:27:40PM +0300, Avi Kivity wrote:
> >  On 04/12/2011 03:22 PM, Gleb Natapov wrote:
> >  >On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote:
> >  >>   On 04/12/2011 12:36 PM, Gleb Natapov wrote:
> >  >>   >mmio_index should be taken into account when copying data from
> >  >>   >userspace.
> >  >>   >
> >  >>   >Signed-off-by: Gleb Natapov<gleb@redhat.com>
> >  >>   >---
> >  >>   >    arch/x86/kvm/x86.c |    3 ++-
> >  >>   >    1 files changed, 2 insertions(+), 1 deletions(-)
> >  >>   >
> >  >>   >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >  >>   >index b568779..609c7ab 100644
> >  >>   >--- a/arch/x86/kvm/x86.c
> >  >>   >+++ b/arch/x86/kvm/x86.c
> >  >>   >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
> >  >>   >    	if (vcpu->mmio_needed) {
> >  >>   >    		vcpu->mmio_needed = 0;
> >  >>   >    		if (!vcpu->mmio_is_write)
> >  >>   >-			memcpy(vcpu->mmio_data, run->mmio.data, 8);
> >  >>   >+			memcpy(vcpu->mmio_data + vcpu->mmio_index,
> >  >>   >+			       run->mmio.data, 8);
> >  >>   >    		vcpu->mmio_index += 8;
> >  >>   >    		if (vcpu->mmio_index<    vcpu->mmio_size) {
> >  >>   >    			run->exit_reason = KVM_EXIT_MMIO;
> >  >>
> >  >>   Interesting, the code passed the emulator.flat sse test.  Does it now?
> >  >>
> >  >It pass now and before. Probably by chance.
> >
> >  I don't understand how.  I explicitly set the values so that it
> >  would fail in that case.
> >
> >  Can you patch the test to fail with the current code?
> >
> If I understand correctly you've already found the explanation why
> test case worked?

Yes, but I used O_APPEND when writing my message.

> >  >But if I change read_emulated() to do
> >  >
> >  >   int n = min(size, (unsigned)KVM_MMIO_SIZE);
> >  >
> >  >instead of
> >  >
> >  >   int n = min(size, 8u);
> >  >
> >  >emulator.flat fails to emulate far jump instruction.
> >
> >  Ouch, looks like we have the multi-transaction support in two
> >  places.  I guess this is what made sse mmio work.
> >
> >  Not sure what we should do (patch is fine, question is how to
> >  resolve the duplication).
> >
> Multi-transaction works faster in complete_mmio, so read_emulated()
> should use it by doing int n = min(size, (unsigned)KVM_MMIO_SIZE).
> Other code in read_emulated() is still needed since it provides read
> re-play for multiple mmio reads during instruction emulation.

Not concerned about speed here.

Core support for large mmio feels correct, but the duplication doesn't.

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

Re: [PATCH 1/5] KVM: mmio_fault_cr2 is not used.

[Gleb Natapov]

Ping? What about those patches? 2/5 should be dropped, but others are
not yet applied too as far as I see.

On Tue, Apr 12, 2011 at 12:36:21PM +0300, Gleb Natapov wrote:
> Remove unused variable mmio_fault_cr2.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    1 -
>  arch/x86/kvm/x86.c              |    2 +-
>  2 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index bd57639..c1a7f54 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -358,7 +358,6 @@ struct kvm_vcpu_arch {
>  	struct fpu guest_fpu;
>  	u64 xcr0;
>  
> -	gva_t mmio_fault_cr2;
>  	struct kvm_pio_request pio;
>  	void *pio_data;
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1d5a7f4..b568779 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4504,7 +4504,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  	struct decode_cache *c = &vcpu->arch.emulate_ctxt.decode;
>  
>  	kvm_clear_exception_queue(vcpu);
> -	vcpu->arch.mmio_fault_cr2 = cr2;
> +
>  	/*
>  	 * TODO: fix emulate.c to use guest_read/write_register
>  	 * instead of direct ->regs accesses, can save hundred cycles
> -- 
> 1.7.2.3
> 
> --
> 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 1/5] KVM: mmio_fault_cr2 is not used.

[Avi Kivity]

On 04/26/2011 11:25 AM, Gleb Natapov wrote:
> Ping? What about those patches? 2/5 should be dropped, but others are
> not yet applied too as far as I see.

Applied now, thanks for the reminder.

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