Re: [kvm-devel] [PATCH 1/2] kvm: Batch writes to MMIO

[Anthony L. <an...@co...>]

Laurent Vivier wrote:
> This patch is the kernel part of the "batch writes to MMIO" patch.
>
> When kernel has to send MMIO writes to userspace, it stores them
> in memory until it has to pass the hand to userspace for another
> reason. This avoids to have too many context switches on operations
> that can wait.
>
> WARNING: this breaks compatibility with old userspace part.
>
> Signed-off-by: Laurent Vivier <Lau...@bu...>
> ---
>  arch/x86/kvm/x86.c         |   21 +++++++++++++++++++++
>  include/asm-x86/kvm_host.h |    2 ++
>  include/linux/kvm.h        |   10 +++++++++-
>  virt/kvm/kvm_main.c        |    3 +++
>  4 files changed, 35 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0ce5563..3881056 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2942,8 +2942,21 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		kvm_x86_ops->decache_regs(vcpu);
>  	}
>  
> +batch:
>  	r = __vcpu_run(vcpu, kvm_run);
>  
> +	if (!r && vcpu->mmio_is_write &&
> +	    kvm_run->exit_reason == KVM_EXIT_MMIO &&
> +	    kvm_run->batch_count < KVM_MAX_BATCH) {
> +		struct kvm_batch *batch = vcpu->arch.batch_data;
> +		int i = kvm_run->batch_count++;
> +
> +		batch[i].phys_addr = vcpu->mmio_phys_addr;
> +		batch[i].len = vcpu->mmio_size;
> +		memcpy(batch[i].data, vcpu->mmio_data, batch[i].len);
> +
> +		goto batch;
> +	}
>   

I wonder if this is sufficient for dynticks enabled guests.  __vcpu_run 
could last a very long time.

Ignoring that for a moment, Avi's comment about having userspace tell 
you which addresses to batch is an important one.  MMIO writes may have 
side effects and the next vcpu_run may rely on those side effects.  For 
instance, MMIO based IRQ acknowledgement.

You need a white list not only for performances purposes but also for 
correctness.

Regards,

Anthony Liguori