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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Wed, 2016-03-09 at 12:52 +0000, Xu, Quan wrote:
> On March 09, 2016 6:25pm, <dario.faggioli@xxxxxxxxxx> 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.

This sentence you seem to be asking about, is what we've been
"debating", in this thread about the changelog, since almost the
beginning: it's bad to mix, because it leads to inconsistencies can are
bad because in general it would trigger BUG_ON, in debug builds, and
cause deadlock, in non-debug builds, even if in this case neither
happen. What is that you need more insights about?

Here it is (again) what I would use as commit message:
---
pci_get_pdev() doesn't require interrupts to be disabled when taking
the pcidevs_lock, which protects its execution. So, spin_lock() can be
used for that, and that is what is done almost everywhere.

Currenlty, there is an exception, in early boot IOMMU initialization on
AMD systems, where spin_lock_irqsave() and restore are used. However,
since, in that case too:
 - we don't need to disable interrupts (for same reasons stated above),
 - interrupts are enabled already, so there is no need to save and
   restore flags,
just change it into using spin_lock(), as everywhere.

Note that, mixing interrupt disabled and enabled spinlocks is something
we disallow, except for very special situations. And since this one
does not qualify as such, using IRQ disabling variants can be
considered a bug (which manages to not trigger the safety checking we
have in place only because they're not yet enabled).
---

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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