Re: [PATCH 1/7] libkvm: delayed MMIO support (common part)

[Laurent Vivier <Laurent.Vivier@bull.net>]

Le lundi 26 mai 2008 à 16:55 +0300, Avi Kivity a écrit :
> Laurent Vivier wrote:
> > This patch introduces in libkvm the common part of MMIO batching interface.
> >
> > It checks the MMIO batching availability with ioctl(KVM_CHECK_EXTENSION).
> > If KVM_CAP_DELAYED_MMIO is available, it processes the MMIO ring buffer
> > at the return of ioctl(KVM_RUN).
> > It defines kvm_register_delayed_mmio() to register a delayed MMIO zone, and 
> > kvm_unregister_delayed_mmio() to unregister it.
> >
> >  
> >  void init_slots(void);
> > diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
> > index d1e95a4..718f4c6 100644
> > --- a/libkvm/libkvm.c
> > +++ b/libkvm/libkvm.c
> > @@ -879,6 +879,20 @@ again:
> >  
> >  	post_kvm_run(kvm, vcpu);
> >  
> > +#if defined(KVM_CAP_DELAYED_MMIO)
> > +	if (kvm->delayed_mmio) {
> > +	        struct kvm_batch *batch = (void *)run +
> > +					  kvm->delayed_mmio * PAGE_SIZE;
> > +		while (batch->first != batch->last) {
> > +			kvm->callbacks->mmio_write(kvm->opaque,
> > +					    batch->mmio[batch->first].phys_addr,
> > +					   &batch->mmio[batch->first].data[0],
> > +					    batch->mmio[batch->first].len);
> > +			batch->first = (batch->first + 1) % KVM_MAX_BATCH;
> >   
> 
> There needs to be a write barrier after this (smp_wmb()), to avoid 
> compiler and cpu reordering.  Especially important for non-x86.  Also 
> need a read barrier before reading batch->first.
> 
> Or maybe not, likely the pthread_mutex_lock() acts as the correct 
> barrier.  But we do need the equivalent barriers in the kernel code 
> (missed it on that review).

OK

> > +		}
> > +	}
> > +#endif
> > +
> >  	if (r == -1) {
> >  		r = handle_io_window(kvm);
> >  		goto more;
> >   
> 
> > +
> > +int kvm_register_delayed_mmio(kvm_context_t kvm, uint64_t addr, uint32_t size)
> > +{
> > +#ifdef KVM_CAP_DELAYED_MMIO
> > +	struct kvm_delayed_mmio_zone zone;
> > +	int r;
> > +
> > +	if (kvm->delayed_mmio) {
> > +
> > +		zone.addr = addr;
> > +		zone.size = size;
> > +
> > +		r = ioctl(kvm->vm_fd, KVM_REGISTER_DELAYED_MMIO, &zone);
> > +		if (r == -1) {
> > +			perror("kvm_register_delayed_mmio_zone");
> > +			return -errno;
> > +		}
> > +		return 0;
> > +	}
> > +#endif
> > +	return -1;
> >   
> 
> -ENOSYS, or something.  -1 is -ENOPERM.

OK

> 
> > +}
> > +
> > +int kvm_unregister_delayed_mmio(kvm_context_t kvm, uint64_t addr, uint32_t size)
> > +{
> > +#ifdef KVM_CAP_DELAYED_MMIO
> > +	struct kvm_delayed_mmio_zone zone;
> > +	int r;
> > +
> > +	if (kvm->delayed_mmio) {
> > +
> > +		zone.addr = addr;
> > +		zone.size = size;
> > +
> > +		r = ioctl(kvm->vm_fd, KVM_UNREGISTER_DELAYED_MMIO, &zone);
> > +		if (r == -1) {
> > +			perror("kvm_unregister_delayed_mmio_zone");
> > +			return -errno;
> > +		}
> > +		return 0;
> > +	}
> > +#endif
> > +	return -1;
> >   
> 
> ditto.

OK
-- 
------------- Laurent.Vivier@bull.net ---------------
"The best way to predict the future is to invent it."
- Alan Kay