Re: [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq

[Sheng Yang <sheng@linux.intel.com>]

On Tuesday 23 December 2008 23:18:43 Marcelo Tosatti wrote:
> Hi Sheng,
>
> On Tue, Dec 23, 2008 at 04:00:25PM +0800, Sheng Yang wrote:
> > Which is more convenient...
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  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.
>
Nice catch! I've updated the patchset to address this, take a look? :)

-- 
regards
Yang, Sheng