[RFC PATCH] KVM: arm/arm64: rework kvm_handle_mmio_return

[RFC PATCH] KVM: arm/arm64: rework kvm_handle_mmio_return

[Andre Przywara]

Currently we call kvm_handle_mmio_return when we need to sync back
register content into the guest after a trap.
This function expects its arguments packaged into struct kvm_run,
which we only naturally use when we emulate in userspace. With
in-kernel emulation we have to copy the data into that strcut.
This patch fixes this by explicitly passing the required variables,
so we save the copying in two cases.
Also since this function is actually a NOP for an MMIO write, we
rename it to kvm_writeback_mmio_data and move the !is_write check to
the callers.
This fixes a bug where we were missing a writeback for one in-kernel
emulation case.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi Christoffer,

this is my take on fixing the missing MMIO writeback call.
This rework kind of hides the actual bug-fix, that's why I dumped
it in favour of the one-liner in my series.

Cheers,
Andre.

 arch/arm/include/asm/kvm_mmio.h   |  3 ++-
 arch/arm/kvm/arm.c                | 10 +++++---
 arch/arm/kvm/mmio.c               | 54 ++++++++++++++++++---------------------
 arch/arm64/include/asm/kvm_mmio.h |  3 ++-
 virt/kvm/arm/vgic.c               |  8 ++----
 5 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
index d8e90c8..21f97ac 100644
--- a/arch/arm/include/asm/kvm_mmio.h
+++ b/arch/arm/include/asm/kvm_mmio.h
@@ -28,7 +28,8 @@ struct kvm_decode {
 	bool sign_extend;
 };
 
-int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
+			    gpa_t addr);
 int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		 phys_addr_t fault_ipa);
 
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index b538431..e9f593c 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -555,9 +555,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		return ret;
 
 	if (run->exit_reason == KVM_EXIT_MMIO) {
-		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
-		if (ret)
-			return ret;
+		if (!run->mmio.is_write) {
+			ret = kvm_writeback_mmio_data(vcpu, run->mmio.data,
+						      run->mmio.len,
+						      run->mmio.phys_addr);
+			if (ret)
+				return ret;
+		}
 	}
 
 	if (vcpu->sigset_active)
diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 0f6600f..052aa02 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -86,38 +86,33 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
 }
 
 /**
- * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
+ * kvm_writeback_mmio_data -- Write back emulation data into the guest's
+ *                            target register after return from userspace.
  * @vcpu: The VCPU pointer
- * @run:  The VCPU run struct containing the mmio data
+ * @data: A pointer to the data to be written back
+ * @len:  The size of the read access
+ * @addr: The original MMIO address (for the tracepoint only)
  *
  * This should only be called after returning from userspace for MMIO load
  * emulation.
  */
-int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
+int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data_ptr, int len,
+			    gpa_t addr)
 {
 	unsigned long data;
-	unsigned int len;
 	int mask;
 
-	if (!run->mmio.is_write) {
-		len = run->mmio.len;
-		if (len > sizeof(unsigned long))
-			return -EINVAL;
+	data = mmio_read_buf(data_ptr, len);
 
-		data = mmio_read_buf(run->mmio.data, len);
-
-		if (vcpu->arch.mmio_decode.sign_extend &&
-		    len < sizeof(unsigned long)) {
-			mask = 1U << ((len * 8) - 1);
-			data = (data ^ mask) - mask;
-		}
-
-		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
-			       data);
-		data = vcpu_data_host_to_guest(vcpu, data, len);
-		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
+	if (vcpu->arch.mmio_decode.sign_extend && len < sizeof(unsigned long)) {
+		mask = 1U << ((len * 8) - 1);
+		data = (data ^ mask) - mask;
 	}
 
+	trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, addr, data);
+	data = vcpu_data_host_to_guest(vcpu, data, len);
+	vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
+
 	return 0;
 }
 
@@ -202,22 +197,23 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 				      data_buf);
 	}
 
+	if (!ret) {
+		/* We handled the access successfully in the kernel. */
+		vcpu->stat.mmio_exit_kernel++;
+		if (!is_write)
+			kvm_writeback_mmio_data(vcpu, data_buf, len, fault_ipa);
+		return 1;
+	}
+
 	/* Now prepare kvm_run for the potential return to userland. */
 	run->mmio.is_write	= is_write;
 	run->mmio.phys_addr	= fault_ipa;
 	run->mmio.len		= len;
+	run->exit_reason	= KVM_EXIT_MMIO;
 	if (is_write)
 		memcpy(run->mmio.data, data_buf, len);
 
-	if (!ret) {
-		/* We handled the access successfully in the kernel. */
-		vcpu->stat.mmio_exit_kernel++;
-		kvm_handle_mmio_return(vcpu, run);
-		return 1;
-	} else {
-		vcpu->stat.mmio_exit_user++;
-	}
+	vcpu->stat.mmio_exit_user++;
 
-	run->exit_reason	= KVM_EXIT_MMIO;
 	return 0;
 }
diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
index fe612a9..00a57bd 100644
--- a/arch/arm64/include/asm/kvm_mmio.h
+++ b/arch/arm64/include/asm/kvm_mmio.h
@@ -30,7 +30,8 @@ struct kvm_decode {
 	bool sign_extend;
 };
 
-int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
+			    gpa_t addr);
 int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		 phys_addr_t fault_ipa);
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 00429b3..7a9aa56 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -821,7 +821,6 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_io_device *iodev = container_of(this,
 						    struct vgic_io_device, dev);
-	struct kvm_run *run = vcpu->run;
 	const struct vgic_io_range *range;
 	struct kvm_exit_mmio mmio;
 	bool updated_state;
@@ -850,12 +849,9 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
 		updated_state = false;
 	}
 	spin_unlock(&dist->lock);
-	run->mmio.is_write	= is_write;
-	run->mmio.len		= len;
-	run->mmio.phys_addr	= addr;
-	memcpy(run->mmio.data, val, len);
 
-	kvm_handle_mmio_return(vcpu, run);
+	if (!is_write)
+		kvm_writeback_mmio_data(vcpu, val, len, addr);
 
 	if (updated_state)
 		vgic_kick_vcpus(vcpu->kvm);
-- 
2.7.3

Re: [RFC PATCH] KVM: arm/arm64: rework kvm_handle_mmio_return

[Christoffer Dall]

On Fri, Apr 08, 2016 at 11:43:50AM +0100, Andre Przywara wrote:
> Currently we call kvm_handle_mmio_return when we need to sync back
> register content into the guest after a trap.
> This function expects its arguments packaged into struct kvm_run,
> which we only naturally use when we emulate in userspace. With
> in-kernel emulation we have to copy the data into that strcut.

s/strcut/struct/

> This patch fixes this by explicitly passing the required variables,
> so we save the copying in two cases.

this patch fixes what?

> Also since this function is actually a NOP for an MMIO write, we
> rename it to kvm_writeback_mmio_data and move the !is_write check to
> the callers.
> This fixes a bug where we were missing a writeback for one in-kernel
> emulation case.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi Christoffer,
> 
> this is my take on fixing the missing MMIO writeback call.
> This rework kind of hides the actual bug-fix, that's why I dumped
> it in favour of the one-liner in my series.
> 

Hmmm, I thought I also sent a patch for this (off list), and I'm not
sure why your approach is better, but ok, I guess I can review your
patch...

> 
>  arch/arm/include/asm/kvm_mmio.h   |  3 ++-
>  arch/arm/kvm/arm.c                | 10 +++++---
>  arch/arm/kvm/mmio.c               | 54 ++++++++++++++++++---------------------
>  arch/arm64/include/asm/kvm_mmio.h |  3 ++-
>  virt/kvm/arm/vgic.c               |  8 ++----
>  5 files changed, 38 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
> index d8e90c8..21f97ac 100644
> --- a/arch/arm/include/asm/kvm_mmio.h
> +++ b/arch/arm/include/asm/kvm_mmio.h
> @@ -28,7 +28,8 @@ struct kvm_decode {
>  	bool sign_extend;
>  };
>  
> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,

I think this should be called kvm_write_back_mmio_data() instead.

> +			    gpa_t addr);
>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		 phys_addr_t fault_ipa);
>  
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index b538431..e9f593c 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -555,9 +555,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		return ret;
>  
>  	if (run->exit_reason == KVM_EXIT_MMIO) {
> -		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> -		if (ret)
> -			return ret;
> +		if (!run->mmio.is_write) {
> +			ret = kvm_writeback_mmio_data(vcpu, run->mmio.data,
> +						      run->mmio.len,
> +						      run->mmio.phys_addr);
> +			if (ret)
> +				return ret;
> +		}

I preferred all the logic be encapsulated in the function instead of
extracting values in the caller, but it's purely a cosmetic comment.

>  	}
>  
>  	if (vcpu->sigset_active)
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 0f6600f..052aa02 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -86,38 +86,33 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
>  }
>  
>  /**
> - * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
> + * kvm_writeback_mmio_data -- Write back emulation data into the guest's
> + *                            target register after return from userspace.
>   * @vcpu: The VCPU pointer
> - * @run:  The VCPU run struct containing the mmio data
> + * @data: A pointer to the data to be written back
> + * @len:  The size of the read access
> + * @addr: The original MMIO address (for the tracepoint only)
>   *
>   * This should only be called after returning from userspace for MMIO load
>   * emulation.
>   */
> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data_ptr, int len,
> +			    gpa_t addr)
>  {
>  	unsigned long data;
> -	unsigned int len;
>  	int mask;
>  
> -	if (!run->mmio.is_write) {
> -		len = run->mmio.len;
> -		if (len > sizeof(unsigned long))
> -			return -EINVAL;
> +	data = mmio_read_buf(data_ptr, len);
>  
> -		data = mmio_read_buf(run->mmio.data, len);
> -
> -		if (vcpu->arch.mmio_decode.sign_extend &&
> -		    len < sizeof(unsigned long)) {
> -			mask = 1U << ((len * 8) - 1);
> -			data = (data ^ mask) - mask;
> -		}
> -
> -		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> -			       data);
> -		data = vcpu_data_host_to_guest(vcpu, data, len);
> -		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> +	if (vcpu->arch.mmio_decode.sign_extend && len < sizeof(unsigned long)) {
> +		mask = 1U << ((len * 8) - 1);
> +		data = (data ^ mask) - mask;
>  	}
>  
> +	trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, addr, data);
> +	data = vcpu_data_host_to_guest(vcpu, data, len);
> +	vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> +
>  	return 0;
>  }
>  
> @@ -202,22 +197,23 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  				      data_buf);
>  	}
>  
> +	if (!ret) {
> +		/* We handled the access successfully in the kernel. */
> +		vcpu->stat.mmio_exit_kernel++;
> +		if (!is_write)
> +			kvm_writeback_mmio_data(vcpu, data_buf, len, fault_ipa);

are you not doing this writeback twice?  Once in the vgic and once here?
That was the very thing I tried to address in my patch.

> +		return 1;
> +	}
> +
>  	/* Now prepare kvm_run for the potential return to userland. */
>  	run->mmio.is_write	= is_write;
>  	run->mmio.phys_addr	= fault_ipa;
>  	run->mmio.len		= len;
> +	run->exit_reason	= KVM_EXIT_MMIO;
>  	if (is_write)
>  		memcpy(run->mmio.data, data_buf, len);
>  
> -	if (!ret) {
> -		/* We handled the access successfully in the kernel. */
> -		vcpu->stat.mmio_exit_kernel++;
> -		kvm_handle_mmio_return(vcpu, run);
> -		return 1;
> -	} else {
> -		vcpu->stat.mmio_exit_user++;
> -	}
> +	vcpu->stat.mmio_exit_user++;
>  
> -	run->exit_reason	= KVM_EXIT_MMIO;
>  	return 0;
>  }
> diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
> index fe612a9..00a57bd 100644
> --- a/arch/arm64/include/asm/kvm_mmio.h
> +++ b/arch/arm64/include/asm/kvm_mmio.h
> @@ -30,7 +30,8 @@ struct kvm_decode {
>  	bool sign_extend;
>  };
>  
> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
> +			    gpa_t addr);
>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		 phys_addr_t fault_ipa);
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 00429b3..7a9aa56 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -821,7 +821,6 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct vgic_io_device *iodev = container_of(this,
>  						    struct vgic_io_device, dev);
> -	struct kvm_run *run = vcpu->run;
>  	const struct vgic_io_range *range;
>  	struct kvm_exit_mmio mmio;
>  	bool updated_state;
> @@ -850,12 +849,9 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>  		updated_state = false;
>  	}
>  	spin_unlock(&dist->lock);
> -	run->mmio.is_write	= is_write;
> -	run->mmio.len		= len;
> -	run->mmio.phys_addr	= addr;
> -	memcpy(run->mmio.data, val, len);
>  
> -	kvm_handle_mmio_return(vcpu, run);
> +	if (!is_write)
> +		kvm_writeback_mmio_data(vcpu, val, len, addr);

see above.

>  
>  	if (updated_state)
>  		vgic_kick_vcpus(vcpu->kvm);
> -- 
> 2.7.3
> 

Re: [RFC PATCH] KVM: arm/arm64: rework kvm_handle_mmio_return

[Andre Przywara]

Hi,

On 08/04/16 12:05, Christoffer Dall wrote:
> On Fri, Apr 08, 2016 at 11:43:50AM +0100, Andre Przywara wrote:
>> Currently we call kvm_handle_mmio_return when we need to sync back
>> register content into the guest after a trap.
>> This function expects its arguments packaged into struct kvm_run,
>> which we only naturally use when we emulate in userspace. With
>> in-kernel emulation we have to copy the data into that strcut.
> 
> s/strcut/struct/
> 
>> This patch fixes this by explicitly passing the required variables,
>> so we save the copying in two cases.
> 
> this patch fixes what?

The strcut ;-)

It fixes the unnecessary copying. Indeed worded badly.

>> Also since this function is actually a NOP for an MMIO write, we
>> rename it to kvm_writeback_mmio_data and move the !is_write check to
>> the callers.
>> This fixes a bug where we were missing a writeback for one in-kernel
>> emulation case.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> Hi Christoffer,
>>
>> this is my take on fixing the missing MMIO writeback call.
>> This rework kind of hides the actual bug-fix, that's why I dumped
>> it in favour of the one-liner in my series.
>>
> 
> Hmmm, I thought I also sent a patch for this (off list), and I'm not
> sure why your approach is better, but ok, I guess I can review your
> patch...

My approach is not better, just different. I just wanted some opinions
because I wasn't satisfied either.
This run-> wrapping did annoy me already some months ago, so I just took
the opportunity to kill it.

>>
>>  arch/arm/include/asm/kvm_mmio.h   |  3 ++-
>>  arch/arm/kvm/arm.c                | 10 +++++---
>>  arch/arm/kvm/mmio.c               | 54 ++++++++++++++++++---------------------
>>  arch/arm64/include/asm/kvm_mmio.h |  3 ++-
>>  virt/kvm/arm/vgic.c               |  8 ++----
>>  5 files changed, 38 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
>> index d8e90c8..21f97ac 100644
>> --- a/arch/arm/include/asm/kvm_mmio.h
>> +++ b/arch/arm/include/asm/kvm_mmio.h
>> @@ -28,7 +28,8 @@ struct kvm_decode {
>>  	bool sign_extend;
>>  };
>>  
>> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
> 
> I think this should be called kvm_write_back_mmio_data() instead.
> 
>> +			    gpa_t addr);
>>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>  		 phys_addr_t fault_ipa);
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index b538431..e9f593c 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -555,9 +555,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		return ret;
>>  
>>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>> -		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>> -		if (ret)
>> -			return ret;
>> +		if (!run->mmio.is_write) {
>> +			ret = kvm_writeback_mmio_data(vcpu, run->mmio.data,
>> +						      run->mmio.len,
>> +						      run->mmio.phys_addr);
>> +			if (ret)
>> +				return ret;
>> +		}
> 
> I preferred all the logic be encapsulated in the function instead of
> extracting values in the caller, but it's purely a cosmetic comment.

You mean the "if (!is_write)" or wrapping the values into the kvm_run
struct?
As mentioned in the commit message, only actual userland emulation is
using run->, so for in-kernel emulation we have to set it up, which is
unnecessary copying IMHO.
By un-wrapping the parameters we just would pass is_write to exit the
function immediately if it was false, that's why the move to the callers.
But I agree it is probably bike shedding.

>>  	}
>>  
>>  	if (vcpu->sigset_active)
>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>> index 0f6600f..052aa02 100644
>> --- a/arch/arm/kvm/mmio.c
>> +++ b/arch/arm/kvm/mmio.c
>> @@ -86,38 +86,33 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
>>  }
>>  
>>  /**
>> - * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
>> + * kvm_writeback_mmio_data -- Write back emulation data into the guest's
>> + *                            target register after return from userspace.
>>   * @vcpu: The VCPU pointer
>> - * @run:  The VCPU run struct containing the mmio data
>> + * @data: A pointer to the data to be written back
>> + * @len:  The size of the read access
>> + * @addr: The original MMIO address (for the tracepoint only)
>>   *
>>   * This should only be called after returning from userspace for MMIO load
>>   * emulation.
>>   */
>> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data_ptr, int len,
>> +			    gpa_t addr)
>>  {
>>  	unsigned long data;
>> -	unsigned int len;
>>  	int mask;
>>  
>> -	if (!run->mmio.is_write) {
>> -		len = run->mmio.len;
>> -		if (len > sizeof(unsigned long))
>> -			return -EINVAL;
>> +	data = mmio_read_buf(data_ptr, len);
>>  
>> -		data = mmio_read_buf(run->mmio.data, len);
>> -
>> -		if (vcpu->arch.mmio_decode.sign_extend &&
>> -		    len < sizeof(unsigned long)) {
>> -			mask = 1U << ((len * 8) - 1);
>> -			data = (data ^ mask) - mask;
>> -		}
>> -
>> -		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
>> -			       data);
>> -		data = vcpu_data_host_to_guest(vcpu, data, len);
>> -		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>> +	if (vcpu->arch.mmio_decode.sign_extend && len < sizeof(unsigned long)) {
>> +		mask = 1U << ((len * 8) - 1);
>> +		data = (data ^ mask) - mask;
>>  	}
>>  
>> +	trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, addr, data);
>> +	data = vcpu_data_host_to_guest(vcpu, data, len);
>> +	vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -202,22 +197,23 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>  				      data_buf);
>>  	}
>>  
>> +	if (!ret) {
>> +		/* We handled the access successfully in the kernel. */
>> +		vcpu->stat.mmio_exit_kernel++;
>> +		if (!is_write)
>> +			kvm_writeback_mmio_data(vcpu, data_buf, len, fault_ipa);
> 
> are you not doing this writeback twice?  Once in the vgic and once here?
> That was the very thing I tried to address in my patch.

Right, I missed that one (which was the actual reason the code worked
before although we missed the write-back.
Not sure why it was actually in in the first place.

I guess I will just go with your patch, because it's simpler.

Cheers,
Andre.

>> +		return 1;
>> +	}
>> +
>>  	/* Now prepare kvm_run for the potential return to userland. */
>>  	run->mmio.is_write	= is_write;
>>  	run->mmio.phys_addr	= fault_ipa;
>>  	run->mmio.len		= len;
>> +	run->exit_reason	= KVM_EXIT_MMIO;
>>  	if (is_write)
>>  		memcpy(run->mmio.data, data_buf, len);
>>  
>> -	if (!ret) {
>> -		/* We handled the access successfully in the kernel. */
>> -		vcpu->stat.mmio_exit_kernel++;
>> -		kvm_handle_mmio_return(vcpu, run);
>> -		return 1;
>> -	} else {
>> -		vcpu->stat.mmio_exit_user++;
>> -	}
>> +	vcpu->stat.mmio_exit_user++;
>>  
>> -	run->exit_reason	= KVM_EXIT_MMIO;
>>  	return 0;
>>  }
>> diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
>> index fe612a9..00a57bd 100644
>> --- a/arch/arm64/include/asm/kvm_mmio.h
>> +++ b/arch/arm64/include/asm/kvm_mmio.h
>> @@ -30,7 +30,8 @@ struct kvm_decode {
>>  	bool sign_extend;
>>  };
>>  
>> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
>> +			    gpa_t addr);
>>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>  		 phys_addr_t fault_ipa);
>>  
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 00429b3..7a9aa56 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -821,7 +821,6 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>  	struct vgic_io_device *iodev = container_of(this,
>>  						    struct vgic_io_device, dev);
>> -	struct kvm_run *run = vcpu->run;
>>  	const struct vgic_io_range *range;
>>  	struct kvm_exit_mmio mmio;
>>  	bool updated_state;
>> @@ -850,12 +849,9 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>>  		updated_state = false;
>>  	}
>>  	spin_unlock(&dist->lock);
>> -	run->mmio.is_write	= is_write;
>> -	run->mmio.len		= len;
>> -	run->mmio.phys_addr	= addr;
>> -	memcpy(run->mmio.data, val, len);
>>  
>> -	kvm_handle_mmio_return(vcpu, run);
>> +	if (!is_write)
>> +		kvm_writeback_mmio_data(vcpu, val, len, addr);
> 
> see above.
> 
>>  
>>  	if (updated_state)
>>  		vgic_kick_vcpus(vcpu->kvm);
>> -- 
>> 2.7.3
>>
> 

Re: [RFC PATCH] KVM: arm/arm64: rework kvm_handle_mmio_return

[Christoffer Dall]

On Fri, Apr 08, 2016 at 05:50:03PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 08/04/16 12:05, Christoffer Dall wrote:
> > On Fri, Apr 08, 2016 at 11:43:50AM +0100, Andre Przywara wrote:
> >> Currently we call kvm_handle_mmio_return when we need to sync back
> >> register content into the guest after a trap.
> >> This function expects its arguments packaged into struct kvm_run,
> >> which we only naturally use when we emulate in userspace. With
> >> in-kernel emulation we have to copy the data into that strcut.
> > 
> > s/strcut/struct/
> > 
> >> This patch fixes this by explicitly passing the required variables,
> >> so we save the copying in two cases.
> > 
> > this patch fixes what?
> 
> The strcut ;-)
> 
> It fixes the unnecessary copying. Indeed worded badly.
> 
> >> Also since this function is actually a NOP for an MMIO write, we
> >> rename it to kvm_writeback_mmio_data and move the !is_write check to
> >> the callers.
> >> This fixes a bug where we were missing a writeback for one in-kernel
> >> emulation case.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >> Hi Christoffer,
> >>
> >> this is my take on fixing the missing MMIO writeback call.
> >> This rework kind of hides the actual bug-fix, that's why I dumped
> >> it in favour of the one-liner in my series.
> >>
> > 
> > Hmmm, I thought I also sent a patch for this (off list), and I'm not
> > sure why your approach is better, but ok, I guess I can review your
> > patch...
> 
> My approach is not better, just different. I just wanted some opinions
> because I wasn't satisfied either.

I just felt like I had implemented and tested a fix for a problem, and
then I also had to review an alternative approach, but oh well, no
worries.

> This run-> wrapping did annoy me already some months ago, so I just took
> the opportunity to kill it.
> 
> >>
> >>  arch/arm/include/asm/kvm_mmio.h   |  3 ++-
> >>  arch/arm/kvm/arm.c                | 10 +++++---
> >>  arch/arm/kvm/mmio.c               | 54 ++++++++++++++++++---------------------
> >>  arch/arm64/include/asm/kvm_mmio.h |  3 ++-
> >>  virt/kvm/arm/vgic.c               |  8 ++----
> >>  5 files changed, 38 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
> >> index d8e90c8..21f97ac 100644
> >> --- a/arch/arm/include/asm/kvm_mmio.h
> >> +++ b/arch/arm/include/asm/kvm_mmio.h
> >> @@ -28,7 +28,8 @@ struct kvm_decode {
> >>  	bool sign_extend;
> >>  };
> >>  
> >> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
> > 
> > I think this should be called kvm_write_back_mmio_data() instead.
> > 
> >> +			    gpa_t addr);
> >>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >>  		 phys_addr_t fault_ipa);
> >>  
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index b538431..e9f593c 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -555,9 +555,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >>  		return ret;
> >>  
> >>  	if (run->exit_reason == KVM_EXIT_MMIO) {
> >> -		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> >> -		if (ret)
> >> -			return ret;
> >> +		if (!run->mmio.is_write) {
> >> +			ret = kvm_writeback_mmio_data(vcpu, run->mmio.data,
> >> +						      run->mmio.len,
> >> +						      run->mmio.phys_addr);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> > 
> > I preferred all the logic be encapsulated in the function instead of
> > extracting values in the caller, but it's purely a cosmetic comment.
> 
> You mean the "if (!is_write)" or wrapping the values into the kvm_run
> struct?
> As mentioned in the commit message, only actual userland emulation is
> using run->, so for in-kernel emulation we have to set it up, which is
> unnecessary copying IMHO.
> By un-wrapping the parameters we just would pass is_write to exit the
> function immediately if it was false, that's why the move to the callers.
> But I agree it is probably bike shedding.
> 
> >>  	}
> >>  
> >>  	if (vcpu->sigset_active)
> >> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> >> index 0f6600f..052aa02 100644
> >> --- a/arch/arm/kvm/mmio.c
> >> +++ b/arch/arm/kvm/mmio.c
> >> @@ -86,38 +86,33 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
> >>  }
> >>  
> >>  /**
> >> - * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
> >> + * kvm_writeback_mmio_data -- Write back emulation data into the guest's
> >> + *                            target register after return from userspace.
> >>   * @vcpu: The VCPU pointer
> >> - * @run:  The VCPU run struct containing the mmio data
> >> + * @data: A pointer to the data to be written back
> >> + * @len:  The size of the read access
> >> + * @addr: The original MMIO address (for the tracepoint only)
> >>   *
> >>   * This should only be called after returning from userspace for MMIO load
> >>   * emulation.
> >>   */
> >> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data_ptr, int len,
> >> +			    gpa_t addr)
> >>  {
> >>  	unsigned long data;
> >> -	unsigned int len;
> >>  	int mask;
> >>  
> >> -	if (!run->mmio.is_write) {
> >> -		len = run->mmio.len;
> >> -		if (len > sizeof(unsigned long))
> >> -			return -EINVAL;
> >> +	data = mmio_read_buf(data_ptr, len);
> >>  
> >> -		data = mmio_read_buf(run->mmio.data, len);
> >> -
> >> -		if (vcpu->arch.mmio_decode.sign_extend &&
> >> -		    len < sizeof(unsigned long)) {
> >> -			mask = 1U << ((len * 8) - 1);
> >> -			data = (data ^ mask) - mask;
> >> -		}
> >> -
> >> -		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> >> -			       data);
> >> -		data = vcpu_data_host_to_guest(vcpu, data, len);
> >> -		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> >> +	if (vcpu->arch.mmio_decode.sign_extend && len < sizeof(unsigned long)) {
> >> +		mask = 1U << ((len * 8) - 1);
> >> +		data = (data ^ mask) - mask;
> >>  	}
> >>  
> >> +	trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, addr, data);
> >> +	data = vcpu_data_host_to_guest(vcpu, data, len);
> >> +	vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -202,22 +197,23 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >>  				      data_buf);
> >>  	}
> >>  
> >> +	if (!ret) {
> >> +		/* We handled the access successfully in the kernel. */
> >> +		vcpu->stat.mmio_exit_kernel++;
> >> +		if (!is_write)
> >> +			kvm_writeback_mmio_data(vcpu, data_buf, len, fault_ipa);
> > 
> > are you not doing this writeback twice?  Once in the vgic and once here?
> > That was the very thing I tried to address in my patch.
> 
> Right, I missed that one (which was the actual reason the code worked
> before although we missed the write-back.
> Not sure why it was actually in in the first place.
> 
> I guess I will just go with your patch, because it's simpler.
> 

I do agree about the weirdness of using the mmio struct designed for
userspace in the kernel.  On the other hand, that struct encapsulates
all that's needed, which why I think the code was done the way it is
currently.

I think we originally considered only the vgic as an in-kernel thing
that needed to do the write-back, hence the 'bug' or situation or
whatever we have today.

I don't care strongly which way we go about it.  If you prefer building
the new vgic on top of your patch, then just address the double-copying
in the gic.  It's really up to you, whatever makes the shitty job of
managing the giant new vgic implementation for you.

Thanks,
-Christoffer