From: Tomasz Figa <tfiga@chromium.org>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Yong Wu <yong.wu@mediatek.com>, Joerg Roedel <joro@8bytes.org>,
Will Deacon <will@kernel.org>,
youlin.pei@mediatek.com, anan.sun@mediatek.com,
Nicolas Boichat <drinkcat@chromium.org>,
srv_heupstream <srv_heupstream@mediatek.com>,
chao.hao@mediatek.com,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Krzysztof Kozlowski <krzk@kernel.org>,
"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,"
<iommu@lists.linux-foundation.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Greg Kroah-Hartman <gregkh@google.com>,
kernel-team@android.com,
"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 6/7] iommu/mediatek: Gather iova in iommu_unmap to achieve tlb sync once
Date: Fri, 8 Jan 2021 18:56:59 +0900 [thread overview]
Message-ID: <CAAFQd5Cwjq2E+jZopcV2SP-6fzOvh=_uisd3JhWdwwp4zwO=zw@mail.gmail.com> (raw)
In-Reply-To: <1de76b46-d9c1-4011-c087-1df236f442c3@arm.com>
On Wed, Dec 23, 2020 at 8:00 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-12-23 08:56, Tomasz Figa wrote:
> > On Wed, Dec 16, 2020 at 06:36:06PM +0800, Yong Wu wrote:
> >> In current iommu_unmap, this code is:
> >>
> >> iommu_iotlb_gather_init(&iotlb_gather);
> >> ret = __iommu_unmap(domain, iova, size, &iotlb_gather);
> >> iommu_iotlb_sync(domain, &iotlb_gather);
> >>
> >> We could gather the whole iova range in __iommu_unmap, and then do tlb
> >> synchronization in the iommu_iotlb_sync.
> >>
> >> This patch implement this, Gather the range in mtk_iommu_unmap.
> >> then iommu_iotlb_sync call tlb synchronization for the gathered iova range.
> >> we don't call iommu_iotlb_gather_add_page since our tlb synchronization
> >> could be regardless of granule size.
> >>
> >> In this way, gather->start is impossible ULONG_MAX, remove the checking.
> >>
> >> This patch aims to do tlb synchronization *once* in the iommu_unmap.
> >>
> >> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >> ---
> >> drivers/iommu/mtk_iommu.c | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> >> index db7d43adb06b..89cec51405cd 100644
> >> --- a/drivers/iommu/mtk_iommu.c
> >> +++ b/drivers/iommu/mtk_iommu.c
> >> @@ -506,7 +506,12 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
> >> struct iommu_iotlb_gather *gather)
> >> {
> >> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> >> + unsigned long long end = iova + size;
> >>
> >> + if (gather->start > iova)
> >> + gather->start = iova;
> >> + if (gather->end < end)
> >> + gather->end = end;
> >
> > I don't know how common the case is, but what happens if
> > gather->start...gather->end is a disjoint range from iova...end? E.g.
> >
> > | gather | ..XXX... | iova |
> > | | | |
> > gather->start | iova |
> > gather->end end
> >
> > We would also end up invalidating the TLB for the XXX area, which could
> > affect the performance.
>
> Take a closer look at iommu_unmap() - the gather data is scoped to each
> individual call, so that can't possibly happen.
>
> > Also, why is the existing code in __arm_v7s_unmap() not enough? It seems
> > to call io_pgtable_tlb_add_page() already, so it should be batching the
> > flushes.
>
> Because if we leave io-pgtable in charge of maintenance it will also
> inject additional invalidations and syncs for the sake of strictly
> correct walk cache maintenance. Apparently we can get away without that
> on this hardware, so the fundamental purpose of this series is to
> sidestep it.
>
> It's proven to be cleaner overall to devolve this kind of "non-standard"
> TLB maintenance back to drivers rather than try to cram yet more
> special-case complexity into io-pgtable itself. I'm planning to clean up
> the remains of the TLBI_ON_MAP quirk entirely after this.
(Sorry, I sent an empty email accidentally.)
I see, thanks for clarifying. The patch looks good to me then.
Best regards,
Tomasz
>
> Robin.
>
> >> return dom->iop->unmap(dom->iop, iova, size, gather);
> >> }
> >>
> >> @@ -523,9 +528,6 @@ static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
> >> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> >> size_t length = gather->end - gather->start;
> >>
> >> - if (gather->start == ULONG_MAX)
> >> - return;
> >> -
> >> mtk_iommu_tlb_flush_range_sync(gather->start, length, gather->pgsize,
> >> dom->data);
> >> }
> >> --
> >> 2.18.0
> >>
> >> _______________________________________________
> >> iommu mailing list
> >> iommu@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-01-08 9:58 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-16 10:36 [PATCH v3 0/7] MediaTek IOMMU improve tlb flush performance in map/unmap Yong Wu
2020-12-16 10:36 ` [PATCH v3 1/7] iommu: Move iotlb_sync_map out from __iommu_map Yong Wu
2020-12-23 8:51 ` Christoph Hellwig
2020-12-24 11:27 ` Yong Wu
2020-12-16 10:36 ` [PATCH v3 2/7] iommu: Add iova and size as parameters in iotlb_sync_map Yong Wu
2020-12-16 10:36 ` [PATCH v3 3/7] iommu/mediatek: Add iotlb_sync_map to sync whole the iova range Yong Wu
2020-12-16 10:36 ` [PATCH v3 4/7] iommu: Switch gather->end to unsigned long long Yong Wu
2020-12-16 11:03 ` David Laight
2020-12-17 2:26 ` Yong Wu
2020-12-16 12:10 ` Robin Murphy
2020-12-17 2:26 ` Yong Wu
2020-12-16 10:36 ` [PATCH v3 5/7] iommu: Allow io_pgtable_tlb ops optional Yong Wu
2021-01-18 16:18 ` Robin Murphy
2020-12-16 10:36 ` [PATCH v3 6/7] iommu/mediatek: Gather iova in iommu_unmap to achieve tlb sync once Yong Wu
2020-12-23 8:56 ` Tomasz Figa
2020-12-23 11:00 ` Robin Murphy
2021-01-08 9:56 ` Tomasz Figa
2021-01-08 9:56 ` Tomasz Figa [this message]
2021-01-18 16:35 ` Robin Murphy
2021-01-18 16:58 ` Will Deacon
2021-01-18 17:14 ` Robin Murphy
2020-12-16 10:36 ` [PATCH v3 7/7] iommu/mediatek: Remove the tlb-ops for v7s Yong Wu
2021-01-18 16:39 ` Robin Murphy
Reply instructions:
You may reply publicly 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='CAAFQd5Cwjq2E+jZopcV2SP-6fzOvh=_uisd3JhWdwwp4zwO=zw@mail.gmail.com' \
--to=tfiga@chromium.org \
--cc=anan.sun@mediatek.com \
--cc=chao.hao@mediatek.com \
--cc=drinkcat@chromium.org \
--cc=gregkh@google.com \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=kernel-team@android.com \
--cc=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=robin.murphy@arm.com \
--cc=srv_heupstream@mediatek.com \
--cc=will@kernel.org \
--cc=yong.wu@mediatek.com \
--cc=youlin.pei@mediatek.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).