IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: Will Deacon <will@kernel.org>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	iommu@lists.linux-foundation.org
Subject: Re: [PATCH 3/4] iommu/arm-smmu-v3: Fix ATC invalidation ordering wrt main TLBs
Date: Tue, 20 Aug 2019 18:07:38 +0100
Message-ID: <20190820170737.xdfbg4p4bneau4uv@willie-the-truck> (raw)
In-Reply-To: <983be1bc-1b51-77e7-5aee-8395a4f3724e@arm.com>

On Tue, Aug 20, 2019 at 05:50:06PM +0100, Robin Murphy wrote:
> On 20/08/2019 16:45, Will Deacon wrote:
> > When invalidating the ATC for an PCIe endpoint using ATS, we must take
> > care to complete invalidation of the main SMMU TLBs beforehand, otherwise
> > the device could immediately repopulate its ATC with stale translations.
> > 
> > Hooking the ATC invalidation into ->unmap() as we currently do does the
> > exact opposite: it ensures that the ATC is invalidated *before*  the
> > main TLBs, which is bogus.
> > 
> > Move ATC invalidation into the actual (leaf) invalidation routines so
> > that it is always called after completing main TLB invalidation.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >   drivers/iommu/arm-smmu-v3.c | 12 +++++-------
> >   1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 9096eca0c480..183a1c121179 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -1961,6 +1961,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> >   	 */
> >   	arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> >   	arm_smmu_cmdq_issue_sync(smmu);
> > +	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
> >   }
> >   static void arm_smmu_tlb_inv_range(unsigned long iova, size_t size,
> > @@ -1969,7 +1970,7 @@ static void arm_smmu_tlb_inv_range(unsigned long iova, size_t size,
> >   {
> >   	u64 cmds[CMDQ_BATCH_ENTRIES * CMDQ_ENT_DWORDS];
> >   	struct arm_smmu_device *smmu = smmu_domain->smmu;
> > -	unsigned long end = iova + size;
> > +	unsigned long start = iova, end = iova + size;
> >   	int i = 0;
> >   	struct arm_smmu_cmdq_ent cmd = {
> >   		.tlbi = {
> > @@ -1998,6 +1999,8 @@ static void arm_smmu_tlb_inv_range(unsigned long iova, size_t size,
> >   	}
> >   	arm_smmu_cmdq_issue_cmdlist(smmu, cmds, i, true);
> > +	if (leaf)
> > +		arm_smmu_atc_inv_domain(smmu_domain, 0, start, size);
> 
> I still need to get up to speed on your cmdlist and unmap changes, but in
> isolation this "if (leaf)" guard looks a bit dodgy - in the case where
> io-pgtable goes to unmap a 2MB block, finds it's mapped as a table, and
> blows it away in one go, we'll only see a non-leaf TLBI call for that range,
> no?

Yuck, this is quite horrible. I don't think the ATC is permitted to cache
intermediate walks, so we actually don't need the thing to be synchronous
here. But if we update the gather structure as well, then we risk
over-invalidating for the non-ATS case when we get to the sync.

I'll have a think.

> Tangentially, does arm_smmu_atc_inv_domain() really need to sync once for
> each individual master, or could that do better as well? Not something we
> should worry about right now, but now that I'm looking I may as well note it
> for the record.

Indeed -- that function should be rewritten using the cmdlist() stuff I've
done. I'm just reluctant to start optimising for the ATS case when I'm not
able to test it.

Thanks,

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-20 15:45 [PATCH 0/4] Sort out SMMUv3 ATC invalidation and locking Will Deacon
2019-08-20 15:45 ` [PATCH 1/4] iommu/arm-smmu-v3: Document ordering guarantees of command insertion Will Deacon
2019-08-20 15:45 ` [PATCH 2/4] iommu/arm-smmu-v3: Rework enabling/disabling of ATS for PCI masters Will Deacon
2019-08-20 16:12   ` Robin Murphy
2019-08-20 16:31     ` Will Deacon
2019-08-20 15:45 ` [PATCH 3/4] iommu/arm-smmu-v3: Fix ATC invalidation ordering wrt main TLBs Will Deacon
2019-08-20 16:50   ` Robin Murphy
2019-08-20 17:07     ` Will Deacon [this message]
2019-08-20 15:45 ` [PATCH 4/4] iommu/arm-smmu-v3: Avoid locking on invalidation path when not using ATS Will Deacon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190820170737.xdfbg4p4bneau4uv@willie-the-truck \
    --to=will@kernel.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.org \
    --cc=robin.murphy@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org iommu@archiver.kernel.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/ public-inbox