[PATCH] KVM: Fix page-crossing MMIO

[PATCH] KVM: Fix page-crossing MMIO

[Avi Kivity]

MMIO that are split across a page boundary are currently broken - the
code does not expect to be aborted by the exit to userspace for the
first MMIO fragment.

This patch fixes the problem by generalizing the current code for handling
16-byte MMIOs to handle a number of "fragments", and changes the MMIO
code to create those fragments.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/ia64/include/asm/kvm_host.h |    2 +
 arch/ia64/kvm/kvm-ia64.c         |   10 ++--
 arch/x86/kvm/x86.c               |  114 +++++++++++++++++++++++++++-----------
 include/linux/kvm_host.h         |   31 +++++++++--
 4 files changed, 115 insertions(+), 42 deletions(-)

diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index c4b4bac..6d6a5ac 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -449,6 +449,8 @@ struct kvm_vcpu_arch {
 	char log_buf[VMM_LOG_LEN];
 	union context host;
 	union context guest;
+
+	char mmio_data[8];
 };
 
 struct kvm_vm_stat {
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 9d80ff8..882ab21 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -232,12 +232,12 @@ static int handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	if ((p->addr & PAGE_MASK) == IOAPIC_DEFAULT_BASE_ADDRESS)
 		goto mmio;
 	vcpu->mmio_needed = 1;
-	vcpu->mmio_phys_addr = kvm_run->mmio.phys_addr = p->addr;
-	vcpu->mmio_size = kvm_run->mmio.len = p->size;
+	vcpu->mmio_fragments[0].gpa = kvm_run->mmio.phys_addr = p->addr;
+	vcpu->mmio_fragments[0].len = kvm_run->mmio.len = p->size;
 	vcpu->mmio_is_write = kvm_run->mmio.is_write = !p->dir;
 
 	if (vcpu->mmio_is_write)
-		memcpy(vcpu->mmio_data, &p->data, p->size);
+		memcpy(vcpu->arch.mmio_data, &p->data, p->size);
 	memcpy(kvm_run->mmio.data, &p->data, p->size);
 	kvm_run->exit_reason = KVM_EXIT_MMIO;
 	return 0;
@@ -719,7 +719,7 @@ static void kvm_set_mmio_data(struct kvm_vcpu *vcpu)
 	struct kvm_mmio_req *p = kvm_get_vcpu_ioreq(vcpu);
 
 	if (!vcpu->mmio_is_write)
-		memcpy(&p->data, vcpu->mmio_data, 8);
+		memcpy(&p->data, vcpu->arch.mmio_data, 8);
 	p->state = STATE_IORESP_READY;
 }
 
@@ -739,7 +739,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	}
 
 	if (vcpu->mmio_needed) {
-		memcpy(vcpu->mmio_data, kvm_run->mmio.data, 8);
+		memcpy(vcpu->arch.mmio_data, kvm_run->mmio.data, 8);
 		kvm_set_mmio_data(vcpu);
 		vcpu->mmio_read_completed = 1;
 		vcpu->mmio_needed = 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0d9a578..4de705c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3718,9 +3718,8 @@ struct read_write_emulator_ops {
 static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes)
 {
 	if (vcpu->mmio_read_completed) {
-		memcpy(val, vcpu->mmio_data, bytes);
 		trace_kvm_mmio(KVM_TRACE_MMIO_READ, bytes,
-			       vcpu->mmio_phys_addr, *(u64 *)val);
+			       vcpu->mmio_fragments[0].gpa, *(u64 *)val);
 		vcpu->mmio_read_completed = 0;
 		return 1;
 	}
@@ -3756,8 +3755,9 @@ static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
 static int write_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
 			   void *val, int bytes)
 {
-	memcpy(vcpu->mmio_data, val, bytes);
-	memcpy(vcpu->run->mmio.data, vcpu->mmio_data, 8);
+	struct kvm_mmio_fragment *frag = &vcpu->mmio_fragments[0];
+
+	memcpy(vcpu->run->mmio.data, frag->data, frag->len);
 	return X86EMUL_CONTINUE;
 }
 
@@ -3784,10 +3784,7 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
 	gpa_t gpa;
 	int handled, ret;
 	bool write = ops->write;
-
-	if (ops->read_write_prepare &&
-		  ops->read_write_prepare(vcpu, val, bytes))
-		return X86EMUL_CONTINUE;
+	struct kvm_mmio_fragment *frag;
 
 	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
 
@@ -3813,15 +3810,19 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
 	bytes -= handled;
 	val += handled;
 
-	vcpu->mmio_needed = 1;
-	vcpu->run->exit_reason = KVM_EXIT_MMIO;
-	vcpu->run->mmio.phys_addr = vcpu->mmio_phys_addr = gpa;
-	vcpu->mmio_size = bytes;
-	vcpu->run->mmio.len = min(vcpu->mmio_size, 8);
-	vcpu->run->mmio.is_write = vcpu->mmio_is_write = write;
-	vcpu->mmio_index = 0;
+	while (bytes) {
+		unsigned now = min(bytes, 8U);
 
-	return ops->read_write_exit_mmio(vcpu, gpa, val, bytes);
+		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
+		frag->gpa = gpa;
+		frag->data = val;
+		frag->len = now;
+
+		gpa += now;
+		val += now;
+		bytes -= now;
+	}
+	return X86EMUL_CONTINUE;
 }
 
 int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
@@ -3830,10 +3831,18 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 			struct read_write_emulator_ops *ops)
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	gpa_t gpa;
+	int rc;
+
+	if (ops->read_write_prepare &&
+		  ops->read_write_prepare(vcpu, val, bytes))
+		return X86EMUL_CONTINUE;
+
+	vcpu->mmio_nr_fragments = 0;
 
 	/* Crossing a page boundary? */
 	if (((addr + bytes - 1) ^ addr) & PAGE_MASK) {
-		int rc, now;
+		int now;
 
 		now = -addr & ~PAGE_MASK;
 		rc = emulator_read_write_onepage(addr, val, now, exception,
@@ -3846,8 +3855,25 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 		bytes -= now;
 	}
 
-	return emulator_read_write_onepage(addr, val, bytes, exception,
-					   vcpu, ops);
+	rc = emulator_read_write_onepage(addr, val, bytes, exception,
+					 vcpu, ops);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	if (!vcpu->mmio_nr_fragments)
+		return rc;
+
+	gpa = vcpu->mmio_fragments[0].gpa;
+
+	vcpu->mmio_needed = 1;
+	vcpu->mmio_cur_fragment = 0;
+
+	vcpu->run->mmio.len = vcpu->mmio_fragments[0].len;
+	vcpu->run->mmio.is_write = vcpu->mmio_is_write = ops->write;
+	vcpu->run->exit_reason = KVM_EXIT_MMIO;
+	vcpu->run->mmio.phys_addr = gpa;
+
+	return ops->read_write_exit_mmio(vcpu, gpa, val, bytes);
 }
 
 static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
@@ -5446,33 +5472,55 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	return r;
 }
 
+/*
+ * Implements the following, as a state machine:
+ *
+ * read:
+ *   for each fragment
+ *     write gpa, len
+ *     exit
+ *     copy data
+ *   execute insn
+ *
+ * write:
+ *   for each fragment
+ *      write gpa, len
+ *      copy data
+ *      exit
+ */
 static int complete_mmio(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
+	struct kvm_mmio_fragment *frag;
 	int r;
 
 	if (!(vcpu->arch.pio.count || vcpu->mmio_needed))
 		return 1;
 
 	if (vcpu->mmio_needed) {
-		vcpu->mmio_needed = 0;
+		/* Complete previous fragment */
+		frag = &vcpu->mmio_fragments[vcpu->mmio_cur_fragment++];
 		if (!vcpu->mmio_is_write)
-			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;
-			run->mmio.phys_addr = vcpu->mmio_phys_addr + vcpu->mmio_index;
-			memcpy(run->mmio.data, vcpu->mmio_data + vcpu->mmio_index, 8);
-			run->mmio.len = min(vcpu->mmio_size - vcpu->mmio_index, 8);
-			run->mmio.is_write = vcpu->mmio_is_write;
-			vcpu->mmio_needed = 1;
-			return 0;
+			memcpy(frag->data, run->mmio.data, frag->len);
+		if (vcpu->mmio_cur_fragment == vcpu->mmio_nr_fragments) {
+			vcpu->mmio_needed = 0;
+			if (vcpu->mmio_is_write)
+				return 1;
+			vcpu->mmio_read_completed = 1;
+			goto done;
 		}
+		/* Initiate next fragment */
+		++frag;
+		run->exit_reason = KVM_EXIT_MMIO;
+		run->mmio.phys_addr = frag->gpa;
 		if (vcpu->mmio_is_write)
-			return 1;
-		vcpu->mmio_read_completed = 1;
+			memcpy(run->mmio.data, frag->data, frag->len);
+		run->mmio.len = frag->len;
+		run->mmio.is_write = vcpu->mmio_is_write;
+		return 0;
+
 	}
+done:
 	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 	r = emulate_instruction(vcpu, EMULTYPE_NO_DECODE);
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 49c2f2f..738b5ed 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -35,6 +35,20 @@
 #endif
 
 /*
+ * If we support unaligned MMIO, at most one fragment will be split into two:
+ */
+#ifdef KVM_UNALIGNED_MMIO
+#  define KVM_EXTRA_MMIO_FRAGMENTS 1
+#else
+#  define KVM_EXTRA_MMIO_FRAGMENTS 0
+#endif
+
+#define KVM_USER_MMIO_SIZE 8
+
+#define KVM_MAX_MMIO_FRAGMENTS \
+	(KVM_MMIO_SIZE / KVM_USER_MMIO_SIZE + KVM_EXTRA_MMIO_FRAGMENTS)
+
+/*
  * vcpu->requests bit members
  */
 #define KVM_REQ_TLB_FLUSH          0
@@ -117,6 +131,16 @@ enum {
 	EXITING_GUEST_MODE
 };
 
+/*
+ * Sometimes a large or cross-page mmio needs to be broken up into separate
+ * exits for userspace servicing.
+ */
+struct kvm_mmio_fragment {
+	gpa_t gpa;
+	void *data;
+	unsigned len;
+};
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 #ifdef CONFIG_PREEMPT_NOTIFIERS
@@ -144,10 +168,9 @@ struct kvm_vcpu {
 	int mmio_needed;
 	int mmio_read_completed;
 	int mmio_is_write;
-	int mmio_size;
-	int mmio_index;
-	unsigned char mmio_data[KVM_MMIO_SIZE];
-	gpa_t mmio_phys_addr;
+	int mmio_cur_fragment;
+	int mmio_nr_fragments;
+	struct kvm_mmio_fragment mmio_fragments[KVM_MAX_MMIO_FRAGMENTS];
 #endif
 
 #ifdef CONFIG_KVM_ASYNC_PF
-- 
1.7.10

Re: [PATCH] KVM: Fix page-crossing MMIO

[Gleb Natapov]

On Wed, Apr 18, 2012 at 07:22:47PM +0300, Avi Kivity wrote:
> MMIO that are split across a page boundary are currently broken - the
> code does not expect to be aborted by the exit to userspace for the
> first MMIO fragment.
> 
> This patch fixes the problem by generalizing the current code for handling
> 16-byte MMIOs to handle a number of "fragments", and changes the MMIO
> code to create those fragments.
> 
For 16 bit IO userspace will see two 8bit writes. Is this OK? Is there
real code that does this kind of IO, or the patch is for correctness
only?

> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/ia64/include/asm/kvm_host.h |    2 +
>  arch/ia64/kvm/kvm-ia64.c         |   10 ++--
>  arch/x86/kvm/x86.c               |  114 +++++++++++++++++++++++++++-----------
>  include/linux/kvm_host.h         |   31 +++++++++--
>  4 files changed, 115 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
> index c4b4bac..6d6a5ac 100644
> --- a/arch/ia64/include/asm/kvm_host.h
> +++ b/arch/ia64/include/asm/kvm_host.h
> @@ -449,6 +449,8 @@ struct kvm_vcpu_arch {
>  	char log_buf[VMM_LOG_LEN];
>  	union context host;
>  	union context guest;
> +
> +	char mmio_data[8];
>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 9d80ff8..882ab21 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -232,12 +232,12 @@ static int handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	if ((p->addr & PAGE_MASK) == IOAPIC_DEFAULT_BASE_ADDRESS)
>  		goto mmio;
>  	vcpu->mmio_needed = 1;
> -	vcpu->mmio_phys_addr = kvm_run->mmio.phys_addr = p->addr;
> -	vcpu->mmio_size = kvm_run->mmio.len = p->size;
> +	vcpu->mmio_fragments[0].gpa = kvm_run->mmio.phys_addr = p->addr;
> +	vcpu->mmio_fragments[0].len = kvm_run->mmio.len = p->size;
>  	vcpu->mmio_is_write = kvm_run->mmio.is_write = !p->dir;
>  
>  	if (vcpu->mmio_is_write)
> -		memcpy(vcpu->mmio_data, &p->data, p->size);
> +		memcpy(vcpu->arch.mmio_data, &p->data, p->size);
>  	memcpy(kvm_run->mmio.data, &p->data, p->size);
>  	kvm_run->exit_reason = KVM_EXIT_MMIO;
>  	return 0;
> @@ -719,7 +719,7 @@ static void kvm_set_mmio_data(struct kvm_vcpu *vcpu)
>  	struct kvm_mmio_req *p = kvm_get_vcpu_ioreq(vcpu);
>  
>  	if (!vcpu->mmio_is_write)
> -		memcpy(&p->data, vcpu->mmio_data, 8);
> +		memcpy(&p->data, vcpu->arch.mmio_data, 8);
>  	p->state = STATE_IORESP_READY;
>  }
>  
> @@ -739,7 +739,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	}
>  
>  	if (vcpu->mmio_needed) {
> -		memcpy(vcpu->mmio_data, kvm_run->mmio.data, 8);
> +		memcpy(vcpu->arch.mmio_data, kvm_run->mmio.data, 8);
>  		kvm_set_mmio_data(vcpu);
>  		vcpu->mmio_read_completed = 1;
>  		vcpu->mmio_needed = 0;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0d9a578..4de705c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3718,9 +3718,8 @@ struct read_write_emulator_ops {
>  static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes)
>  {
>  	if (vcpu->mmio_read_completed) {
> -		memcpy(val, vcpu->mmio_data, bytes);
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, bytes,
> -			       vcpu->mmio_phys_addr, *(u64 *)val);
> +			       vcpu->mmio_fragments[0].gpa, *(u64 *)val);
>  		vcpu->mmio_read_completed = 0;
>  		return 1;
>  	}
> @@ -3756,8 +3755,9 @@ static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
>  static int write_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
>  			   void *val, int bytes)
>  {
> -	memcpy(vcpu->mmio_data, val, bytes);
> -	memcpy(vcpu->run->mmio.data, vcpu->mmio_data, 8);
> +	struct kvm_mmio_fragment *frag = &vcpu->mmio_fragments[0];
> +
> +	memcpy(vcpu->run->mmio.data, frag->data, frag->len);
>  	return X86EMUL_CONTINUE;
>  }
>  
> @@ -3784,10 +3784,7 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	gpa_t gpa;
>  	int handled, ret;
>  	bool write = ops->write;
> -
> -	if (ops->read_write_prepare &&
> -		  ops->read_write_prepare(vcpu, val, bytes))
> -		return X86EMUL_CONTINUE;
> +	struct kvm_mmio_fragment *frag;
>  
>  	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
>  
> @@ -3813,15 +3810,19 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	bytes -= handled;
>  	val += handled;
>  
> -	vcpu->mmio_needed = 1;
> -	vcpu->run->exit_reason = KVM_EXIT_MMIO;
> -	vcpu->run->mmio.phys_addr = vcpu->mmio_phys_addr = gpa;
> -	vcpu->mmio_size = bytes;
> -	vcpu->run->mmio.len = min(vcpu->mmio_size, 8);
> -	vcpu->run->mmio.is_write = vcpu->mmio_is_write = write;
> -	vcpu->mmio_index = 0;
> +	while (bytes) {
> +		unsigned now = min(bytes, 8U);
>  
> -	return ops->read_write_exit_mmio(vcpu, gpa, val, bytes);
> +		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
> +		frag->gpa = gpa;
> +		frag->data = val;
> +		frag->len = now;
> +
> +		gpa += now;
> +		val += now;
> +		bytes -= now;
> +	}
> +	return X86EMUL_CONTINUE;
>  }
>  
>  int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
> @@ -3830,10 +3831,18 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
>  			struct read_write_emulator_ops *ops)
>  {
>  	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> +	gpa_t gpa;
> +	int rc;
> +
> +	if (ops->read_write_prepare &&
> +		  ops->read_write_prepare(vcpu, val, bytes))
> +		return X86EMUL_CONTINUE;
> +
> +	vcpu->mmio_nr_fragments = 0;
>  
>  	/* Crossing a page boundary? */
>  	if (((addr + bytes - 1) ^ addr) & PAGE_MASK) {
> -		int rc, now;
> +		int now;
>  
>  		now = -addr & ~PAGE_MASK;
>  		rc = emulator_read_write_onepage(addr, val, now, exception,
> @@ -3846,8 +3855,25 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
>  		bytes -= now;
>  	}
>  
> -	return emulator_read_write_onepage(addr, val, bytes, exception,
> -					   vcpu, ops);
> +	rc = emulator_read_write_onepage(addr, val, bytes, exception,
> +					 vcpu, ops);
> +	if (rc != X86EMUL_CONTINUE)
> +		return rc;
> +
> +	if (!vcpu->mmio_nr_fragments)
> +		return rc;
> +
> +	gpa = vcpu->mmio_fragments[0].gpa;
> +
> +	vcpu->mmio_needed = 1;
> +	vcpu->mmio_cur_fragment = 0;
> +
> +	vcpu->run->mmio.len = vcpu->mmio_fragments[0].len;
> +	vcpu->run->mmio.is_write = vcpu->mmio_is_write = ops->write;
> +	vcpu->run->exit_reason = KVM_EXIT_MMIO;
> +	vcpu->run->mmio.phys_addr = gpa;
> +
> +	return ops->read_write_exit_mmio(vcpu, gpa, val, bytes);
>  }
>  
>  static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
> @@ -5446,33 +5472,55 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>  	return r;
>  }
>  
> +/*
> + * Implements the following, as a state machine:
> + *
> + * read:
> + *   for each fragment
> + *     write gpa, len
> + *     exit
> + *     copy data
> + *   execute insn
> + *
> + * write:
> + *   for each fragment
> + *      write gpa, len
> + *      copy data
> + *      exit
> + */
>  static int complete_mmio(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> +	struct kvm_mmio_fragment *frag;
>  	int r;
>  
>  	if (!(vcpu->arch.pio.count || vcpu->mmio_needed))
>  		return 1;
>  
>  	if (vcpu->mmio_needed) {
> -		vcpu->mmio_needed = 0;
> +		/* Complete previous fragment */
> +		frag = &vcpu->mmio_fragments[vcpu->mmio_cur_fragment++];
>  		if (!vcpu->mmio_is_write)
> -			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;
> -			run->mmio.phys_addr = vcpu->mmio_phys_addr + vcpu->mmio_index;
> -			memcpy(run->mmio.data, vcpu->mmio_data + vcpu->mmio_index, 8);
> -			run->mmio.len = min(vcpu->mmio_size - vcpu->mmio_index, 8);
> -			run->mmio.is_write = vcpu->mmio_is_write;
> -			vcpu->mmio_needed = 1;
> -			return 0;
> +			memcpy(frag->data, run->mmio.data, frag->len);
> +		if (vcpu->mmio_cur_fragment == vcpu->mmio_nr_fragments) {
> +			vcpu->mmio_needed = 0;
> +			if (vcpu->mmio_is_write)
> +				return 1;
> +			vcpu->mmio_read_completed = 1;
> +			goto done;
>  		}
> +		/* Initiate next fragment */
> +		++frag;
> +		run->exit_reason = KVM_EXIT_MMIO;
> +		run->mmio.phys_addr = frag->gpa;
>  		if (vcpu->mmio_is_write)
> -			return 1;
> -		vcpu->mmio_read_completed = 1;
> +			memcpy(run->mmio.data, frag->data, frag->len);
> +		run->mmio.len = frag->len;
> +		run->mmio.is_write = vcpu->mmio_is_write;
> +		return 0;
> +
>  	}
> +done:
>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>  	r = emulate_instruction(vcpu, EMULTYPE_NO_DECODE);
>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 49c2f2f..738b5ed 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -35,6 +35,20 @@
>  #endif
>  
>  /*
> + * If we support unaligned MMIO, at most one fragment will be split into two:
> + */
> +#ifdef KVM_UNALIGNED_MMIO
> +#  define KVM_EXTRA_MMIO_FRAGMENTS 1
> +#else
> +#  define KVM_EXTRA_MMIO_FRAGMENTS 0
> +#endif
> +
> +#define KVM_USER_MMIO_SIZE 8
> +
> +#define KVM_MAX_MMIO_FRAGMENTS \
> +	(KVM_MMIO_SIZE / KVM_USER_MMIO_SIZE + KVM_EXTRA_MMIO_FRAGMENTS)
> +
> +/*
>   * vcpu->requests bit members
>   */
>  #define KVM_REQ_TLB_FLUSH          0
> @@ -117,6 +131,16 @@ enum {
>  	EXITING_GUEST_MODE
>  };
>  
> +/*
> + * Sometimes a large or cross-page mmio needs to be broken up into separate
> + * exits for userspace servicing.
> + */
> +struct kvm_mmio_fragment {
> +	gpa_t gpa;
> +	void *data;
> +	unsigned len;
> +};
> +
>  struct kvm_vcpu {
>  	struct kvm *kvm;
>  #ifdef CONFIG_PREEMPT_NOTIFIERS
> @@ -144,10 +168,9 @@ struct kvm_vcpu {
>  	int mmio_needed;
>  	int mmio_read_completed;
>  	int mmio_is_write;
> -	int mmio_size;
> -	int mmio_index;
> -	unsigned char mmio_data[KVM_MMIO_SIZE];
> -	gpa_t mmio_phys_addr;
> +	int mmio_cur_fragment;
> +	int mmio_nr_fragments;
> +	struct kvm_mmio_fragment mmio_fragments[KVM_MAX_MMIO_FRAGMENTS];
>  #endif
>  
>  #ifdef CONFIG_KVM_ASYNC_PF
> -- 
> 1.7.10
> 
> --
> 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] KVM: Fix page-crossing MMIO

[Avi Kivity]

On 04/19/2012 11:59 AM, Gleb Natapov wrote:
> On Wed, Apr 18, 2012 at 07:22:47PM +0300, Avi Kivity wrote:
> > MMIO that are split across a page boundary are currently broken - the
> > code does not expect to be aborted by the exit to userspace for the
> > first MMIO fragment.
> > 
> > This patch fixes the problem by generalizing the current code for handling
> > 16-byte MMIOs to handle a number of "fragments", and changes the MMIO
> > code to create those fragments.
> > 
> For 16 bit IO userspace will see two 8bit writes. Is this OK? 

I believe so.  The Pentium bus was 64-bit wide with no address lines
A0-A2, instead it had byte enables BE0-BE7.  So a write that crosses a
quadword boundary would be split.  Similarly the 32-bit PCI bus has 4
byte enables, so anything that crosses a dword boundary is split. 
LOCKed ops are implemented by asserting a signal during the two
operations.  No idea how it's implemented with modern busless
processors, but the same semantics have been kept, probably.

Even more interesting is that a 32-bit write is seen as an 8 bit write
followed by a 24-bit write.

Note also that the two accesses can target different devices.

> Is there
> real code that does this kind of IO, or the patch is for correctness
> only?

I should have mentioned it in the changelog - it's Windows 95.  There is
also a Red Hat Linux version that does something like this, I remember
adding some code in qemu-kvm to handle the 3-byte write.  So I guess
this fixes a regression.

(not sure - in the Windows 95 case we have a write to VGA that crosses a
page boundary (but the pages are physically contiguous, nothing strange
is going on; the RHL case was writing partially to RAM and partially to
unallocated memory, so it might work with the current code).

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

Re: [PATCH] KVM: Fix page-crossing MMIO

[Marcelo Tosatti]

On Wed, Apr 18, 2012 at 07:22:47PM +0300, Avi Kivity wrote:
> MMIO that are split across a page boundary are currently broken - the
> code does not expect to be aborted by the exit to userspace for the
> first MMIO fragment.
> 
> This patch fixes the problem by generalizing the current code for handling
> 16-byte MMIOs to handle a number of "fragments", and changes the MMIO
> code to create those fragments.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Applied, thanks.