From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dario Faggioli <dario.faggioli@citrix.com>
Subject: Re: [PATCH v3 1/2] IOMMU/spinlock: Fix a bug found in
 AMD IOMMU initialization
Date: Fri, 11 Mar 2016 14:58:22 +0100
Message-ID: <1457704702.3102.575.camel@citrix.com>
References: <1457529455-38314-1-git-send-email-quan.xu@intel.com>
 <1457529455-38314-2-git-send-email-quan.xu@intel.com>
 <CAENZ-+=zh-fEC7kx+7S4P86izSJ+ivqnEgkaFeP1oVdajm36CQ@mail.gmail.com>
 <945CA011AD5F084CBEA3E851C0AB28894B861913@SHSMSX101.ccr.corp.intel.com>
 <1457692537.3102.563.camel@citrix.com>
 <945CA011AD5F084CBEA3E851C0AB28894B861C07@SHSMSX101.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============7805003620535859709=="
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894B861C07@SHSMSX101.ccr.corp.intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
 <mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
 <mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Errors-To: xen-devel-bounces@lists.xen.org
Sender: "Xen-devel" <xen-devel-bounces@lists.xen.org>
To: "Xu, Quan" <quan.xu@intel.com>, Meng Xu <mengxu@cis.upenn.edu>
Cc: Jan Beulich <jbeulich@suse.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

--===============7805003620535859709==
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="=-AHG3uCJa3tMx3W9lVvAL"

--=-AHG3uCJa3tMx3W9lVvAL
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 2016-03-11 at 12:36 +0000, Xu, Quan wrote:
> On March 11, 2016 6:36pm, <Dario Faggioli> wrote:
> > On Fri, 2016-03-11 at 06:54 +0000, Xu, Quan wrote:
> > >=C2=A0
> > So, now, if we know for sure that a lock is _never_ever_ever_ taken
> > from
> > interrupt context, can we mix spin_lock() and spin_lock_irq() on it
> > (for whatever
> > reason)? Well, as far as the above reasoning is concerned, yes.
> >=20
> I appreciate Dario's explanation.
> btw, be careful of spin_lock_irq(), which includes
> 'ASSERT(local_irq_is_enabled()'..
>=20
It does. What about it? That is exactly what marks the difference
between spin_lock_irq() and spin_lock_irqsave(). In fact, the former
should not be used if interrupts are already disabled because then,
when calling the corresponding spin_unlock_irq(), they'd be re-enabled
while instead they shouldn't.

But as said, from the point of view of preventing deadlock, for locks
taken both from inside and outside IRQ context, they're equivalent.

> >=20
> > In fact, the deadlock arises because IRQs interrupt asynchronously
> > what the
> > CPU is doing, and that can happen when the CPU has taken the lock
> > already. But
> > if the 'asynchronous' part goes away, we really don't care whether
> > a lock is take
> > at time t1 with IRQ enabled, and at time t2 with IRQ disabled,
> > don't you think?
> >=20
> Yes.
> Consistency may be helpful to avoid some easy-to-avoid lock errors.
> Moreover, without my fix, I think it would not lead dead lock, as the
> pcidevs_lock is not being taken
> In IRQ context. Right?=C2=A0
>=20
>=20
> For deadlock, I think the key problems are:
> =C2=A0 - A lock can be acquired from IRQ context
> =C2=A0 -The interrupt is delivered to the _same_CPU_ that already holds
> the lock.
>=20
In your case, pcidevs_lock is certainly not being taken from both
inside and outside IRQ context. If it where, using spin_lock() --as it
happens basically everywhere in the code-- would be wrong, and using
spin_lock_irq[save]() --as it happens in the only case you're patching-
- would be what would be right!

Regards,
Dario
--=20
<<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)


--=-AHG3uCJa3tMx3W9lVvAL
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEABECAAYFAlbizv4ACgkQk4XaBE3IOsS7qACeNWvop/gVZ+MuHQovVhRGh43O
5DsAoKcMw+HCqoeDG2kZyEQIul0fjlsY
=m4Ox
-----END PGP SIGNATURE-----

--=-AHG3uCJa3tMx3W9lVvAL--


--===============7805003620535859709==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs
IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y
Zy94ZW4tZGV2ZWwK

--===============7805003620535859709==--