From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [UNTESTED] KVM: do not call kvm_set_irq from irq disabled section Date: Wed, 21 Apr 2010 11:32:49 +0300 Message-ID: <4BCEB831.8050007@redhat.com> References: <20100420155401.GA12982@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm , bonenkamp@gmx.de, Chris Wright , "Yang, Sheng" To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37179 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752903Ab0DUIcz (ORCPT ); Wed, 21 Apr 2010 04:32:55 -0400 In-Reply-To: <20100420155401.GA12982@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 04/20/2010 06:54 PM, Marcelo Tosatti wrote: > The assigned device interrupt work handler calls kvm_set_irq, which > can sleep, for example, waiting for the ioapic mutex, from irq disabled > section. > > https://bugzilla.kernel.org/show_bug.cgi?id=15725 > > Fix by dropping assigned_dev_lock (and re-enabling interrupts) > before invoking kvm_set_irq for the KVM_DEV_IRQ_HOST_MSIX case. Other > cases do not require the lock or interrupts disabled (a new work > instance will be queued in case of concurrent interrupt). > > KVM-Stable-Tag. > Signed-off-by: Marcelo Tosatti > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c > index 47ca447..7ac7bbe 100644 > --- a/virt/kvm/assigned-dev.c > +++ b/virt/kvm/assigned-dev.c > @@ -64,24 +64,33 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) > interrupt_work); > kvm = assigned_dev->kvm; > > - spin_lock_irq(&assigned_dev->assigned_dev_lock); > if (assigned_dev->irq_requested_type& KVM_DEV_IRQ_HOST_MSIX) { > struct kvm_guest_msix_entry *guest_entries = > assigned_dev->guest_msix_entries; > + > + spin_lock_irq(&assigned_dev->assigned_dev_lock); > for (i = 0; i< assigned_dev->entries_nr; i++) { > if (!(guest_entries[i].flags& > KVM_ASSIGNED_MSIX_PENDING)) > continue; > guest_entries[i].flags&= ~KVM_ASSIGNED_MSIX_PENDING; > + /* > + * If kvm_assigned_dev_intr sets pending for an > + * entry smaller than this work instance is > + * currently processing, a new work instance > + * will be queued. > + */ > + spin_unlock_irq(&assigned_dev->assigned_dev_lock); > What happens if assigned_dev->entries_nr is changed here? > kvm_set_irq(assigned_dev->kvm, > assigned_dev->irq_source_id, > guest_entries[i].vector, 1); > + spin_lock_irq(&assigned_dev->assigned_dev_lock); > } > + spin_unlock_irq(&assigned_dev->assigned_dev_lock); > } else > kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id, > assigned_dev->guest_irq, 1); > > - spin_unlock_irq(&assigned_dev->assigned_dev_lock); > } > A less dangerous fix is to copy all vectors to be triggered into a local array, drop the lock, and replay the array into kvm_set_irq(). The else branch could do this as well. -- error compiling committee.c: too many arguments to function