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 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?

> 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.