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

[Gregory Haskins <gregory.haskins@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]