From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock Date: Tue, 12 May 2009 16:17:30 -0600 Message-ID: <1242166650.4788.16.camel@2710p.home> References: <200905121705.53176.sheng.yang@intel.com> <1242120729-2280-1-git-send-email-sheng@linux.intel.com> <20090512115524.GB10901@amt.cnet> <200905122213.36833.sheng.yang@intel.com> <20090512143021.GB12888@amt.cnet> <20090512194432.GA19969@amt.cnet> <1242164187.4788.4.camel@2710p.home> <20090512220908.GA22626@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: "Yang, Sheng" , Avi Kivity , kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from g1t0028.austin.hp.com ([15.216.28.35]:28522 "EHLO g1t0028.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755632AbZELWRh (ORCPT ); Tue, 12 May 2009 18:17:37 -0400 In-Reply-To: <20090512220908.GA22626@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, 2009-05-12 at 19:09 -0300, Marcelo Tosatti wrote: > KVM: workaround workqueue / deassign_host_irq deadlock > > I think I'm running into the following deadlock in the kvm kernel module > when trying to use device assignment: > > CPU A CPU B > kvm_vm_ioctl_deassign_dev_irq() > mutex_lock(&kvm->lock); worker_thread() > -> kvm_deassign_irq() -> > kvm_assigned_dev_interrupt_work_handler() > -> deassign_host_irq() mutex_lock(&kvm->lock); > -> cancel_work_sync() [blocked] > > Workaround the issue by dropping kvm->lock for cancel_work_sync(). > > Reported-by: Alex Williamson > From: Sheng Yang > Signed-off-by: Marcelo Tosatti Perfect, thanks. Acked-by: Alex Williamson > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 4d00942..d4af719 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm, > disable_irq_nosync(assigned_dev-> > host_msix_entries[i].vector); > > + /* > + * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock > + * with cancel_work_sync, since it requires kvm->lock for irq > + * injection. This is a hack, the irq code must use > + * a separate lock. Same below for MSI. > + */ > + mutex_unlock(&kvm->lock); > cancel_work_sync(&assigned_dev->interrupt_work); > + mutex_lock(&kvm->lock); > > for (i = 0; i < assigned_dev->entries_nr; i++) > free_irq(assigned_dev->host_msix_entries[i].vector, > @@ -263,7 +271,9 @@ static void deassign_host_irq(struct kvm *kvm, > } else { > /* Deal with MSI and INTx */ > disable_irq_nosync(assigned_dev->host_irq); > + mutex_unlock(&kvm->lock); > cancel_work_sync(&assigned_dev->interrupt_work); > + mutex_lock(&kvm->lock); > > free_irq(assigned_dev->host_irq, (void *)assigned_dev); >