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 <[email protected]>
> > ---
> >  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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to