[RFC PATCH 01/45] KVM: arm/arm64: add missing MMIO data write-back

[Andre Przywara]

On 29/03/16 13:33, Christoffer Dall wrote:
> On Fri, Mar 25, 2016 at 02:04:24AM +0000, Andre Przywara wrote:
>> When the kernel was handling a guest MMIO access internally, we need
>> to copy the emulation result into the run->mmio structure in order
>> for the kvm_handle_mmio_return() function to pick it up and inject
>> the result back into the guest.
>> Currently the only user of kvm_io_bus for ARM is the VGIC, which did
>> this copying itself, so this was not causing issues so far.
>> But with upcoming kvm_io_bus users we need to do the copying here.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>>  arch/arm/kvm/mmio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>> index 0f6600f..d5c2727 100644
>> --- a/arch/arm/kvm/mmio.c
>> +++ b/arch/arm/kvm/mmio.c
>> @@ -206,7 +206,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>  	run->mmio.is_write	= is_write;
>>  	run->mmio.phys_addr	= fault_ipa;
>>  	run->mmio.len		= len;
>> -	if (is_write)
>> +	if (is_write || !ret)
>>  		memcpy(run->mmio.data, data_buf, len);
> 
> I had a really hard time understanding this, how about this instead:

Admittedly this is the shortest possible fix. I also had a rework closer
to your version, which also avoided copying the arguments in some cases,
but I thought a smaller diff would be more suitable, since this is
actually a fix.

Shall I add a comment here or post my version of the fix instead?

Cheers,
Andre.

> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 0f6600f..0158e9e 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -87,11 +87,10 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
>  
>  /**
>   * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
> + *			     or in-kernel IO emulation
> + *
>   * @vcpu: The VCPU pointer
>   * @run:  The VCPU run struct containing the mmio data
> - *
> - * 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)
>  {
> @@ -206,18 +205,19 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	run->mmio.is_write	= is_write;
>  	run->mmio.phys_addr	= fault_ipa;
>  	run->mmio.len		= len;
> -	if (is_write)
> -		memcpy(run->mmio.data, data_buf, len);
>  
>  	if (!ret) {
>  		/* We handled the access successfully in the kernel. */
> +		if (!is_write)
> +			memcpy(run->mmio.data, data_buf, len);
>  		vcpu->stat.mmio_exit_kernel++;
>  		kvm_handle_mmio_return(vcpu, run);
>  		return 1;
> -	} else {
> -		vcpu->stat.mmio_exit_user++;
>  	}
>  
> +	if (is_write)
> +		memcpy(run->mmio.data, data_buf, len);
> +	vcpu->stat.mmio_exit_user++;
>  	run->exit_reason	= KVM_EXIT_MMIO;
>  	return 0;
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 00429b3..63e99cb 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -850,12 +850,6 @@ 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 (updated_state)
>  		vgic_kick_vcpus(vcpu->kvm);
>