From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq Date: Tue, 23 Dec 2008 13:18:43 -0200 Message-ID: <20081223151843.GA4077@amt.cnet> References: <1230019231-16543-1-git-send-email-sheng@linux.intel.com> <1230019231-16543-3-git-send-email-sheng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , kvm@vger.kernel.org To: Sheng Yang Return-path: Received: from mx2.redhat.com ([66.187.237.31]:54671 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750782AbYLWPSy (ORCPT ); Tue, 23 Dec 2008 10:18:54 -0500 Content-Disposition: inline In-Reply-To: <1230019231-16543-3-git-send-email-sheng@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: Hi Sheng, On Tue, Dec 23, 2008 at 04:00:25PM +0800, Sheng Yang wrote: > Which is more convenient... > > Signed-off-by: Sheng Yang > --- > virt/kvm/kvm_main.c | 10 ++-------- > 1 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index ffd261d..cd84b3e 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm *kvm, > return 0; > > if (irqchip_in_kernel(kvm)) { > - if (!msi2intx && > - adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) { > - free_irq(adev->host_irq, (void *)kvm); > - pci_disable_msi(adev->dev); > - } > + kvm_free_assigned_irq(kvm, adev); > > if (!capable(CAP_SYS_RAWIO)) > return -EPERM; > @@ -339,9 +335,7 @@ static int assigned_device_update_msi(struct kvm *kvm, > > if (irqchip_in_kernel(kvm)) { > if (!msi2intx) { > - if (adev->irq_requested_type & > - KVM_ASSIGNED_DEV_HOST_INTX) > - free_irq(adev->host_irq, (void *)adev); > + kvm_free_assigned_irq(kvm, adev); > > r = pci_enable_msi(adev->dev); > if (r) Regarding kvm_free_assigned_irq and assigned_device_update_msi/update_intx: if (cancel_work_sync(&assigned_dev->interrupt_work)) /* We had pending work. That means we will have to take * care of kvm_put_kvm. */ kvm_put_kvm(kvm); free_irq(assigned_dev->host_irq, (void *)assigned_dev); What prevents the host IRQ from being triggered between kvm_put_kvm and free_irq? Also, if the kvm_put_kvm(kvm) from kvm_assigned_dev_interrupt_work_handler happens to be the last one, can't this happen: - kvm_assigned_dev_interrupt_work_handler - kvm_put_kvm - kvm_destroy_vm - kvm_arch_destroy_vm - kvm_free_all_assigned_devices - kvm_free_assigned_device - kvm_free_assigned_irq - cancel_work_sync(&assigned_dev->interrupt_work) deadlock. The msi2intx parameter is somewhat odd. Wouldnt it be more versatile if controlled from userspace (and per guest) ?