Re: [PATCH v2 1/2] KVM: MMIO: Lock coalesced device when checking for available entry

[Sasha Levin <levinsasha928@gmail.com>]

On Mon, 2011-07-18 at 11:11 +0300, Avi Kivity wrote:
> On 07/15/2011 02:37 PM, Sasha Levin wrote:
> > Move the check whether there are available entries to within the spinlock.
> > This allows working with larger amount of VCPUs and reduces premature
> > exits when using a large number of VCPUs.
> >
> > Cc: Avi Kivity<avi@redhat.com>
> > Cc: Ingo Molnar<mingo@elte.hu>
> > Cc: Marcelo Tosatti<mtosatti@redhat.com>
> > Cc: Pekka Enberg<penberg@kernel.org>
> > Signed-off-by: Sasha Levin<levinsasha928@gmail.com>
> > ---
> >   virt/kvm/coalesced_mmio.c |    9 ++++++---
> >   1 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> > index fc84875..34188db 100644
> > --- a/virt/kvm/coalesced_mmio.c
> > +++ b/virt/kvm/coalesced_mmio.c
> > @@ -37,7 +37,7 @@ static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev,
> >   	 */
> >   	ring = dev->kvm->coalesced_mmio_ring;
> >   	avail = (ring->first - ring->last - 1) % KVM_COALESCED_MMIO_MAX;
> > -	if (avail<  KVM_MAX_VCPUS) {
> > +	if (avail == 0) {
> >   		/* full */
> >   		return 0;
> >   	}
> > @@ -63,11 +63,14 @@ static int coalesced_mmio_write(struct kvm_io_device *this,
> >   {
> >   	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
> >   	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
> > -	if (!coalesced_mmio_in_range(dev, addr, len))
> > -		return -EOPNOTSUPP;
> >
> >   	spin_lock(&dev->lock);
> >
> > +	if (!coalesced_mmio_in_range(dev, addr, len)) {
> > +		spin_unlock(&dev->lock);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> >   	/* copy data in first free entry of the ring */
> 
> Hmm.  This means we take the lock for every I/O, whether it hits 
> coalesced mmio or not.
> 
> We need to do the range check before taking the lock and the space check 
> after taking the lock.
> 

I'll fix that.

Shouldn't the range check be also locked somehow? Currently it is
possible that a coalesced region was removed while we are checking the
ranges, and we won't issue a mmio exit as the host expects.

-- 

Sasha.