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

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

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@citrix.com]
> Sent: Wednesday, March 09, 2016 9:20 PM
> To: Xu, Quan
> Cc: Suravee Suthikulpanit; xen-devel@lists.xen.org; Jan Beulich; Tian, Kevin
> Subject: Re: [Xen-devel] [PATCH v2 1/2] IOMMU/spinlock: Fix a bug found in
> AMD IOMMU initialization.
> 
> On Wed, 2016-03-09 at 12:52 +0000, Xu, Quan wrote:
> > On March 09, 2016 6:25pm, <dario.faggioli@citrix.com> wrote:
> > > On Wed, 2016-03-09 at 07:31 +0000, Xu, Quan wrote:
> > > > On March 09, 2016 1:19pm, <Tian, Kevin> wrote:
> > > > >
> > > If we are absolutely sure that they are enabled, i.e., there is no
> > > _risk_ that they are disabled, it would be ok to just use _irq() (if
> > > disabling interrupt is necessary, which is not in this case, but
> > > that's another thing) and avoid saving the flags.
> > >
> > Dario, thanks for your share.
> > I appreciate you always share some knowledge. :):) btw, the "Linux
> > Device Drivers, 3rd Edition" book also describe it clearly,
> > http://www.makelinux.net/ldd3/chp-5-sect-5
> >
> Yes, with respect to that, us and Linux are similar, and the concerns on whether
> interrupts should be disabled or not when taking a spinlock are generic, and can
> be applied to any OS/hypervisor.
> 
> AFAICR, Linux does not have any check in place similar to our check_lock(), but
> that does not mean much.
> 
> > > > > However there remains an
> > > > > exception in AMD IOMMU code, where the lock is acquired with
> > > > > interrupt disabled. This inconsistency can lead to deadlock.
> > > > >
> > > Can it? In the case of the occurrence being changed by this patch, I
> > > don't think it can.
> > Before this patch, it might. As Jan mentioned, that's just a
> > theoretical concern:
> >  -spin_lock_irqsave() disables interrupts (on the local processor
> > only) before taking the spinlock.
> >   Supposed in MP system (pCPU1, pCPU2, ...), when the pCPU1 call the
> > spin_lock_irqsave(),
> >   It does only disable interrupts for pCPU1, and _not_ for other
> > pCPUn.
> >  -Then, it is mixing interrupt disabled and enabled spinlocks.
> >
> I was commenting on the "can lead to deadlock" part, which is something that,
> in general, we risk if we mix, but that can't possibly occur in this specific case.
> This is also what Jan is saying, and the reason why is also asking to mitigate this
> exact claim... So, I'm not sure what your point is here...
> 
> > > Note that, mixing interrupt disabled and enabled spinlocks is
> > > something we disallow,
> >
> > Dario, could you share something in detail?
> >
> Sorry, I again don't understand... share something about what? I was proposing
> myself a text to be used as changelog, which I'm pasting again here below, for
> completeness/clarity.
> 

Now I am still not clear for this point- "this inconsistency might lead to deadlock".
I think it is similar to 'mixing interrupt disabled and enabled spinlocks is something we disallow'.
I hope you can give me an example about how to lead to deadlock. 

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