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

[Avi Kivity <avi@qumranet.com>]

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 <Laurent.Vivier@bull.net>
> ---
>  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);
>   

This breaks ordering on smp guests. batch_data needs to be a kvm thing, 
not a vcpu thing, and locked, of course.

Also, you don't want to queue writes which trigger I/O since that will 
affect latency.  Userspace should tell the kernel which mmio addresses 
are queuable.

> +
> +		goto batch;
>   

If you move this to within __vcpu_run, you won't need to loop.  Maybe 
the best place is where we actually decide it's an mmio write.

You also need to flush the queue each time you have an in-kernel mmio 
write.  For example:

vcpu0             vcpu1

mmio write (queued)
apic write -> IPI
                     doesn't see effects of write

> +	}
>  out:
>  	if (vcpu->sigset_active)
>  		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> @@ -3830,6 +3843,13 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  	}
>  	vcpu->arch.pio_data = page_address(page);
>  
> +	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> +	if (!page) {
> +		r = -ENOMEM;
> +		goto fail;
> +	}
> +	vcpu->arch.batch_data = page_address(page);
> +
>  	r = kvm_mmu_create(vcpu);
>  	if (r < 0)
>  		goto fail_free_pio_data;
> @@ -3857,6 +3877,7 @@ void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
>  	kvm_mmu_destroy(vcpu);
>  	up_read(&vcpu->kvm->slots_lock);
>  	free_page((unsigned long)vcpu->arch.pio_data);
> +	free_page((unsigned long)vcpu->arch.batch_data);
>  }
>  
>  #define CR3_PAE_RESERVED_BITS ((X86_CR3_PWT | X86_CR3_PCD) - 1)
>  #define CR3_NONPAE_RESERVED_BITS ((PAGE_SIZE-1) & ~(X86_CR3_PWT | X86_CR3_PCD))
> @@ -255,6 +256,7 @@ struct kvm_vcpu_arch {
>  	gva_t mmio_fault_cr2;
>  	struct kvm_pio_request pio;
>  	void *pio_data;
> +	void *batch_data;
>  
>   

It's an array of structs, no?  So it shouldn't be a void *.

>  
> +#define KVM_MAX_BATCH (PAGE_SIZE / sizeof(struct kvm_batch))
> +struct kvm_batch {
> +	__u64 phys_addr;
> +	__u32 len;
> +	__u8  data[8];
> +};
>   

Size is 24 on 64-bit and 20 on 32-bit.  Please pad (after len, so data 
is nicely aligned).


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone