[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 
>__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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.