diff mbox

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

Message ID 1247476355-27284-2-git-send-email-gleb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov July 13, 2009, 9:12 a.m. UTC
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(-)

Comments

Gregory Haskins July 13, 2009, 2:29 p.m. UTC | #1
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.

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 Natapov July 13, 2009, 2:39 p.m. UTC | #2
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.
--
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
Michael S. Tsirkin July 13, 2009, 2:55 p.m. UTC | #3
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.

> > 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
--
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 Natapov July 13, 2009, 3:01 p.m. UTC | #4
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.
--
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
Gregory Haskins July 13, 2009, 3:03 p.m. UTC | #5
Gleb Natapov wrote:
> On Mon, Jul 13, 2009 at 10:29:02AM -0400, Gregory Haskins wrote:
>   
>> Gleb Natapov wrote:
>>     
>>> 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.
>   

Well, that is perhaps unfortunate, but not relevant.  I am not reviewing
those patches ;)

> Besides the chances are good that even without locking around this
> function nothing will break, so why kill host kernel?
>   

The question to ask is: Is it legal to continue to run if the mutex is
found unlocked?  If not, the offending caller should be found/fixed as
early as possible IMO, and an oops should be sufficient to do so.  I
think WARN_ON tends to gets overused/abused, so lets not perpetuate it
simply because of precedence.

Kind Regards,
-Greg
Gregory Haskins July 13, 2009, 3:11 p.m. UTC | #6
Gregory Haskins wrote:
> Gleb Natapov wrote:
>   
>> On Mon, Jul 13, 2009 at 10:29:02AM -0400, Gregory Haskins wrote:
>>   
>>     
>>> Gleb Natapov wrote:
>>>     
>>>       
>>>> 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.
>>   
>>     
>
> Well, that is perhaps unfortunate, but not relevant.  I am not reviewing
> those patches ;)
>
>   
>> Besides the chances are good that even without locking around this
>> function nothing will break, so why kill host kernel?
>>   
>>     
>
> The question to ask is: Is it legal to continue to run if the mutex is
> found unlocked?  If not, the offending caller should be found/fixed as
> early as possible IMO, and an oops should be sufficient to do so.  I
> think WARN_ON tends to gets overused/abused, so lets not perpetuate it
> simply because of precedence.
>   

Err..precedent, I mean.  Heh.

/me needs more coffee.

-Greg

> Kind Regards,
> -Greg
>
>
>
Gleb Natapov July 13, 2009, 3:19 p.m. UTC | #7
On Mon, Jul 13, 2009 at 11:03:56AM -0400, Gregory Haskins wrote:
> Gleb Natapov wrote:
> > On Mon, Jul 13, 2009 at 10:29:02AM -0400, Gregory Haskins wrote:
> >   
> >> Gleb Natapov wrote:
> >>     
> >>> 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.
> >   
> 
> Well, that is perhaps unfortunate, but not relevant.  I am not reviewing
> those patches ;)
> 
> > Besides the chances are good that even without locking around this
> > function nothing will break, so why kill host kernel?
> >   
> 
> The question to ask is: Is it legal to continue to run if the mutex is
> found unlocked?  If not, the offending caller should be found/fixed as
> early as possible IMO, and an oops should be sufficient to do so.  I
> think WARN_ON tends to gets overused/abused, so lets not perpetuate it
> simply because of precedence.
> 
I will have to end this particular thread about WARN_ON by stating
that Avi told me to put it there. I'll let him decide.

--
			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
diff mbox

Patch

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));
+
 	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,