|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
>>> "Xu, Quan" <quan.xu@xxxxxxxxx> 03/09/16 8:32 AM >>> >On March 09, 2016 1:19pm, <Tian, Kevin> wrote: >> > Inside set_iommu_interrupt_handler(), pcidevs_lock serializes calls to >> > pci_get_pdev(), which does not require interrupts to be disabled >> > during its execution. In fact, pcidevs_lock is always (except for this >> > case) taken without disabling interrupts, and disabling them in this >> > case would make the BUG_ON() in check_lock() trigger, if it wasn't for >> > the fact that spinlock debugging checks are still disabled, during >> > initialization, when iommu_setup() (which then end up calling >> > set_iommu_interrupt_handler()) is called. >> >> The key problem is that we need consistent lock usage in all places (either >> all in >> IRQ-safe or all in IRQ-unsafe), regardless of whether check_lock is >> activated or >> not (which is just a debug method to help identify such inconsistency >> problem). > >IMO, this is not the key problem, __Wait for Dario's / Jan's opinions__. The inconsistency is one way of look at the problem, so with that it is kind of "key". >> What about revise like below? >> -- >> >> pcidevs_lock should be held with interrupt enabled. > >I am not sure for this point. Well, I'd say something like "pcidevs_lock doesn't require interrupts to be disabled while being acquired". >> However there remains an >> exception in AMD IOMMU code, where the lock is acquired with interrupt >> disabled. This inconsistency can lead to deadlock. >> >> The fix is straightforward to use spin_lock instead. Also interrupt has been >> enabled when this function is invoked, so we're sure consistency around >> pcidevs_lock can be guaranteed after this fix. > >I really appreciate you send out a revised one, but I think it is not only for >consistency. >__Wait for Dario's / Jan's opinions__. > >To be honest, to me, __my_changlog_ is complicated. I think Kevin's text above is okay. Perhaps weaken the "can lead to a deadlock" slightly, because that's just a theoretical concern, not one that's possible in practice on that code path. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |