Re: [Xen-devel] [PATCH v2 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On March 09, 2016 6:10pm, <jbeulich@xxxxxxxx> wrote:
> >>> "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".
> 

Thanks for correcting me. 
Before this email, I really think inconsistency is one of the problem, and
The key problem is the case would make the BUG_ON() in check_lock() trigger. :(

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

Thanks.
I would use Kevin's text above and change 'can lead to a deadlock' to 'might 
lead to a deadlock' as changelog.

Quan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel