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 1:19pm, <Tian, Kevin> wrote:
> > From: Quan Xu
> > Sent: Wednesday, March 09, 2016 11:08 AM
> > When iommu_setup() is called in __start_xen(), interrupts have already
> > been enabled, and nothing disables (without reenabling) them again in
> > the path that leads to calling set_iommu_interrupt_handler(). There's
> > therefore no risk of them being disabled and needing remaining so, and
> > hence no need for
> 
> no risk of them being 'enabled' since no one disables them again?
> 

Yes,

> > saving and restoring the flags. This means that, even if interrupt
> > would need to be disabled when taking pcidevs_lock, spin_lock_irq()
> > and spin_unlock_irq() could be used.
> 
> I didn't see how this explanation relates to below change. You are changing
> from spin_lock_irqsave to spin_lock. But here you explains the reason to
> spin_lock_irq...
> 
Yes, you are right. I think I'd better remove or add a '()' for:

   "This means that, even if interrupt
    would need to be disabled when taking pcidevs_lock, spin_lock_irq()
    and spin_unlock_irq() could be used."

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


> I think it should be clear enough to state that pci_get_pdev doesn't require
> holding lock with interrupt disabled (so we should use spin_lock in this AMD
> case), and add the fact that when this function is invoked the interrupt is 
> indeed
> enabled.
> 
> 
> >
> > In order to fix this, we can use just plain spin_lock() and
> > spin_unlock(), instead of spin_lock_irqsave() and spin_unlock_irqrestore().
> 
> What about revise like below?
> --
> 
> pcidevs_lock should be held with interrupt enabled.

I am not sure for this point.


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


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