From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v6 3/5] IOMMU: Make the pcidevs_lock a recursive one Date: Fri, 4 Mar 2016 00:59:12 +0100 Message-ID: <1457049552.2959.493.camel@citrix.com> References: <1456929089-17414-1-git-send-email-quan.xu@intel.com> <1456929089-17414-4-git-send-email-quan.xu@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4975979456651015267==" Return-path: In-Reply-To: <1456929089-17414-4-git-send-email-quan.xu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Quan Xu , jbeulich@suse.com, kevin.tian@intel.com Cc: feng.wu@intel.com, jun.nakajima@intel.com, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, suravee.suthikulpanit@amd.com, jinsong.liu@alibaba-inc.com List-Id: xen-devel@lists.xenproject.org --===============4975979456651015267== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-U81s5/thJqNn1usN78rf" --=-U81s5/thJqNn1usN78rf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable [I've removed some of the people that shouldn't be involved in this discussion any longer from the Cc-list] On Wed, 2016-03-02 at 22:31 +0800, Quan Xu wrote: > Signed-off-by: Quan Xu >=20 So, this patch looks ok to me. I spotted something, though, that I think needs some attention. Since I'm jumping on this series only now, if this has been discussed before and I missed it, sorry for the noise. > @@ -788,10 +787,10 @@ static bool_t __init > set_iommu_interrupt_handler(struct amd_iommu *iommu) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave(&pcidevs_lock, flags); > +=C2=A0=C2=A0=C2=A0=C2=A0pcidevs_lock(); > So, spin_lock_irqsave() does: =C2=A0 local_irq_save() =C2=A0 =C2=A0=C2=A0local_save_flags() =C2=A0 =C2=A0=C2=A0local_irq_disable() =C2=A0 _spin_lock() i.e., it saves the flags and disable interrupts. pcidevs_lock() does: =C2=A0 spin_lock_recursive() =C2=A0 =C2=A0 ... //handle recursion =C2=A0 =C2=A0 _spin_lock() i.e., it does not disable interrupts. And therefore it is possible that we are actually skipping disabling interrupts (if they're not disabled already), isn't it? And, of course, the same reasoning --mutatis mutandis-- applies to the unlock side of things. > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iommu->msi.dev =3D pci_get_pdev(iommu->seg,= PCI_BUS(iommu->bdf), > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PCI_DEVFN2(i= ommu->bdf)); > -=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(&pcidevs_lock, flags); > +=C2=A0=C2=A0=C2=A0=C2=A0pcidevs_unlock(); > i.e., spin_unlock_irqrestore() restore the flags, including the interrupt enabled/disabled status, which means it can re-enable the interrupts or not, depending on whether they were enabled at the time of the previous spin_lock_irqsave(); pcidevs_unlock() just don't affect interrupt enabling/disabling at all. So, if the original code is correct in using spin_lock_irqsave()/spin_unlock_irqrestore(), I think that we need _irqsave() and _irqrestore() variants of recursive spinlocks, in order to deal with this case. However, from a quick inspection, it looks to me that: =C2=A0- this can only be called (during initialization), with interrupt =C2=A0 =C2=A0enabled, so least saving & restoring flags shouldn't be necess= ary =C2=A0 =C2=A0(unless I missed where they can be disabled in the call chain =C2=A0 =C2=A0from=C2=A0iommu_setup() toward=C2=A0set_iommu_interrupt_handle= r()). =C2=A0- This protects pci_get_dev(); looking at other places where=C2=A0 =C2=A0 =C2=A0pci_get_dev() is called, I don't think it is really necessary = to =C2=A0 =C2=A0disable interrupts. If I'm right, it means that the original code could well have been using just plain spin_lock() and spin_unlock(), and it would then be fine to turn them into pcidevs_lock() and pcidevs_unlock(), and so no need to add more spin_[un]lock_recursive() variants. That would also mean that the patch is indeed ok, but I'd add a mention of this in the changelog. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-U81s5/thJqNn1usN78rf 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 iEYEABECAAYFAlbYz9EACgkQk4XaBE3IOsTvOwCfZUiDSdNDmBPzVoLIRG2Us50n 0XoAoKO4Y6eyMMCIcDVnefjEixrPlAu+ =cfRT -----END PGP SIGNATURE----- --=-U81s5/thJqNn1usN78rf-- --===============4975979456651015267== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============4975979456651015267==--