On Wednesday 31 December 2008 00:45:51 Marcelo Tosatti wrote: > On Tue, Dec 30, 2008 at 10:14:09AM +0800, Sheng Yang wrote: > > > There is one remaining issue: kvm_assigned_dev_interrupt_work_handler > > > can re-enable the interrupt for KVM_ASSIGNED_DEV_GUEST_MSI case. > > > Perhaps you need a new flag to indicate shutdown (so the host IRQ won't > > > be reenabled). > > > > Is it already covered by disable_irq_no_sync() before cancel_work_sync()? > > I've noted this in my comment: the irq may be disabled nested(once for > > MSI and twice for INTx), but I think it's fine for we're going to free > > it. > > The problem is that the irq can be re-enabled in > kvm_assigned_dev_interrupt_work_handler: > > > context 1 | context 2 > > disable_irq_nosync > kvm_assigned_dev_interrupt_work_handler > enable_irq > cancel_work_sync > > free_irq >
Um... My understanding is a little different... Before context1 execute disable_irq_nosync(), in irq handler, disable_irq_nosync() would also been executed. So only one enable_irq() is not really enable irq here, which can cover the window at all. That's what I means nested disable irq... -- regards Yang, Sheng > So between cancel_work_sync and free_irq kvm_assigned_dev_intr can run > and schedule work. > > I guess it is OK to take the kvm mutex in kvm_free_assigned_irq (but > better verify), so it can: > > mutex_lock > assigned_dev->irq_requested_type = 0; > mutex_unlock > > disable_irq_nosync > cancel_work_sync > free_irq > > So that the work handler won't re-enable the interrupt. > > Other than that the latest patchset looks good. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
