Re: [PATCH 1/5] Protect irq_sources_bitmap by kvm->lock instead of kvm->irq_lock

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 13, 2009 at 05:55:09PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2009 at 05:39:41PM +0300, Gleb Natapov wrote:
> > On Mon, Jul 13, 2009 at 10:29:02AM -0400, Gregory Haskins wrote:
> > > Gleb Natapov wrote:
> > > > It is already protected by kvm->lock on device assignment path. Just
> > > > take the same lock in the PIT code.
> > > >
> > > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > > ---
> > > >  arch/x86/kvm/i8254.c |    2 ++
> > > >  virt/kvm/irq_comm.c  |    8 ++++----
> > > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> > > > index 05e00a8..e1b9016 100644
> > > > --- a/arch/x86/kvm/i8254.c
> > > > +++ b/arch/x86/kvm/i8254.c
> > > > @@ -596,7 +596,9 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
> > > >  	if (!pit)
> > > >  		return NULL;
> > > >  
> > > > +	mutex_lock(&kvm->lock);
> > > >  	pit->irq_source_id = kvm_request_irq_source_id(kvm);
> > > > +	mutex_unlock(&kvm->lock);
> > > >  	if (pit->irq_source_id < 0) {
> > > >  		kfree(pit);
> > > >  		return NULL;
> > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > index 6c57e46..ce8fcd3 100644
> > > > --- a/virt/kvm/irq_comm.c
> > > > +++ b/virt/kvm/irq_comm.c
> > > > @@ -210,7 +210,8 @@ int kvm_request_irq_source_id(struct kvm *kvm)
> > > >  	unsigned long *bitmap = &kvm->arch.irq_sources_bitmap;
> > > >  	int irq_source_id;
> > > >  
> > > > -	mutex_lock(&kvm->irq_lock);
> > > > +	WARN_ON(!mutex_is_locked(&kvm->lock));
> > > >   
> > > 
> > > Shouldn't this be fatal?  (e.g. BUG_ON).  I know the usage between
> > > BUG/WARN is controversial, but it seems to me that something is
> > > completely broken if you expect it to be locked and its not.  Might as
> > > well fail the system, IMO.
> > > 
> > Well I don't really care but we have WARN_ON() in the code currently.
> > Besides the chances are good that even without locking around this
> > function nothing will break, so why kill host kernel?
> 
> Yea. Might as well replace with a comment saying the function expects
> the mutex to be locked.
> 
No. The we will not get bug reports if there are problems. Otherwise you
can say the same about each and every WARN_ON() in the code.

> > > Regards,
> > > -Greg
> > > 
> > > > +
> > > >  	irq_source_id = find_first_zero_bit(bitmap,
> > > >  				sizeof(kvm->arch.irq_sources_bitmap));
> > > >  
> > > > @@ -221,7 +222,6 @@ int kvm_request_irq_source_id(struct kvm *kvm)
> > > >  
> > > >  	ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > > >  	set_bit(irq_source_id, bitmap);
> > > > -	mutex_unlock(&kvm->irq_lock);
> > > >  
> > > >  	return irq_source_id;
> > > >  }
> > > > @@ -230,9 +230,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > >  {
> > > >  	int i;
> > > >  
> > > > +	/* during vm destruction this function is called without locking */
> > > > +	WARN_ON(!mutex_is_locked(&kvm->lock) && atomic_read(&kvm->users_count));
> > > >  	ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > > >  
> > > > -	mutex_lock(&kvm->irq_lock);
> > > >  	if (irq_source_id < 0 ||
> > > >  	    irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
> > > >  		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
> > > > @@ -241,7 +242,6 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > >  	for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > >  		clear_bit(irq_source_id, &kvm->arch.irq_states[i]);
> > > >  	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
> > > > -	mutex_unlock(&kvm->irq_lock);
> > > >  }
> > > >  
> > > >  void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
> > > >   
> > > 
> > > 
> > 
> > 
> > 
> > --
> > 			Gleb.
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.