Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Nadav Amit <namit@vmware.com>
To:     Robin Murphy <robin.murphy@arm.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Will Deacon <will@kernel.org>,
        "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
        Jiajun Cao <caojiajun@vmware.com>
Subject: Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes
Thread-Topic: [PATCH 3/4] iommu/amd: Do not sync on page size changes
Thread-Index: AQHXVv8h/VT5hYiBaUmB/VN59+xv0ar/W4EAgAANS4CAABj+AA==
Date:   Tue, 1 Jun 2021 18:56:57 +0000
Message-ID: <46F62EF5-8463-468B-B9D5-30B4F0572491@vmware.com>
References: <20210502070001.1559127-1-namit@vmware.com>
 <20210502070001.1559127-5-namit@vmware.com>
 <f00bd0ce-e4a7-93c6-39ae-db19779b9331@arm.com>
 <F5D25BE7-FA34-4017-AA22-DDAB24F634BC@vmware.com>
 <7e0b4b12-c68a-ff90-5d86-4ab88ddd7991@arm.com>
In-Reply-To: <7e0b4b12-c68a-ff90-5d86-4ab88ddd7991@arm.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: multipart/signed;
        boundary="Apple-Mail=_B357E3CE-B050-4CEC-BE04-BB7FB11573D5";
        protocol="application/pgp-signature";
        micalg=pgp-sha256
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ebe0944-6a75-4eeb-6bd2-08d9252f07ce
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Jun 2021 18:56:57.3040
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: +AZxhCKS5fpmdm/nJOzUuwrgfOjbegZhWpxEzpBwLP7J88/CVrRhWvTXqUOSYfCAY3ZejNsGL7tUrGbUNKnJeg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR05MB5333
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

--Apple-Mail=_B357E3CE-B050-4CEC-BE04-BB7FB11573D5
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On Jun 1, 2021, at 10:27 AM, Robin Murphy <robin.murphy@arm.com> =
wrote:
>=20
> On 2021-06-01 17:39, Nadav Amit wrote:
>>> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> =
wrote:
>>>=20
>>> On 2021-05-02 07:59, Nadav Amit wrote:
>>>> From: Nadav Amit <namit@vmware.com>
>>>> Some IOMMU architectures perform invalidations regardless of the =
page
>>>> size. In such architectures there is no need to sync when the page =
size
>>>> changes or to regard pgsize when making interim flush in
>>>> iommu_iotlb_gather_add_page().
>>>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>>>> whether gather's pgsize is tracked and triggers a flush.
>>>=20
>>> I've objected before[1], and I'll readily object again ;)
>>>=20
>>> I still think it's very silly to add a bunch of indirection all over =
the place to make a helper function not do the main thing it's intended =
to help with. If you only need trivial address gathering then it's far =
simpler to just implement trivial address gathering. I suppose if you =
really want to you could factor out another helper to share the 5 lines =
of code which ended up in mtk-iommu (see commit f21ae3b10084).
>> Thanks, Robin.
>> I read your comments but I cannot fully understand the alternative =
that you propose, although I do understand your objection to the =
indirection =E2=80=9Cignore_gather_pgsize=E2=80=9D. Would it be ok if =
=E2=80=9Cignore_gather_pgsize" was provided as an argument for =
iommu_iotlb_gather_add_page()?
>=20
> No, I mean if iommu_iotlb_gather_add_page() doesn't have the behaviour =
your driver wants, just don't call it. Write or factor out a suitable =
helper that *does* do what you want and call that, or just implement the =
logic directly inline. Indirect argument or not, it just doesn't make =
much sense to have a helper function call which says "do this except =
don't do most of it".
>=20
>> In general, I can live without this patch. It probably would have =
negligent impact on performance anyhow.
>=20
> As I implied, it sounds like your needs are the same as the Mediatek =
driver had, so you could readily factor out a new page-size-agnostic =
gather helper from that. I fully support making the functional change to =
amd-iommu *somehow* - nobody likes unnecessary syncs - just not with =
this particular implementation :)

Hm=E2=80=A6 avoid code duplication I need to extract some common code to =
another function.

Is the following resembles what you had in mind (untested)?

-- >8 --

Subject: [PATCH] iommu: add iommu_iotlb_gather_add_page_ignore_pgsize()

---
 drivers/iommu/mtk_iommu.c |  7 ++---
 include/linux/iommu.h     | 55 ++++++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e168a682806a..5890e745bed3 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -520,12 +520,9 @@ static size_t mtk_iommu_unmap(struct iommu_domain =
*domain,
 			      struct iommu_iotlb_gather *gather)
 {
 	struct mtk_iommu_domain *dom =3D to_mtk_domain(domain);
-	unsigned long end =3D iova + size - 1;

-	if (gather->start > iova)
-		gather->start =3D iova;
-	if (gather->end < end)
-		gather->end =3D end;
+	iommu_iotlb_gather_update_range(gather, iova, size);
+
 	return dom->iop->unmap(dom->iop, iova, size, gather);
 }

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ca6e6b8084d..037434b6eb4c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -535,29 +535,58 @@ static inline void iommu_iotlb_sync(struct =
iommu_domain *domain,
 	iommu_iotlb_gather_init(iotlb_gather);
 }

-static inline void iommu_iotlb_gather_add_page(struct iommu_domain =
*domain,
+static inline
+void iommu_iotlb_gather_update_range(struct iommu_iotlb_gather *gather,
+				     unsigned long iova, size_t size)
+{
+	unsigned long start =3D iova, end =3D start + size - 1;
+
+	if (gather->end < end)
+		gather->end =3D end;
+
+	if (gather->start > start)
+		gather->start =3D start;
+
+	gather->pgsize =3D size;
+}
+
+static inline
+bool iommu_iotlb_gather_is_disjoint(struct iommu_iotlb_gather *gather,
+				    unsigned long iova, size_t size)
+{
+	return iova + size < gather->start || iova > gather->end + 1;
+}
+
+static inline
+void iommu_iotlb_gather_add_page_ignore_pgsize(struct iommu_domain =
*domain,
 					       struct iommu_iotlb_gather =
*gather,
 					       unsigned long iova, =
size_t size)
 {
-	unsigned long start =3D iova, end =3D start + size - 1;
+	/*
+	 * Only if the new page is disjoint from the current range, then =
sync
+	 * the TLB so that the gather structure can be rewritten.
+	 */
+	if (iommu_iotlb_gather_is_disjoint(gather, iova, size) && =
gather->pgsize)
+		iommu_iotlb_sync(domain, gather);
+
+	iommu_iotlb_gather_update_range(gather, iova, size);
+}

+static inline void iommu_iotlb_gather_add_page(struct iommu_domain =
*domain,
+					       struct iommu_iotlb_gather =
*gather,
+					       unsigned long iova, =
size_t size)
+{
 	/*
 	 * If the new page is disjoint from the current range or is =
mapped at
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize !=3D size ||
-	    end + 1 < gather->start || start > gather->end + 1) {
-		if (gather->pgsize)
-			iommu_iotlb_sync(domain, gather);
-		gather->pgsize =3D size;
-	}
-
-	if (gather->end < end)
-		gather->end =3D end;
+	if ((gather->pgsize !=3D size ||
+	     iommu_iotlb_gather_is_disjoint(gather, iova, size)) &&
+	    gather->pgsize)
+		iommu_iotlb_sync(domain, gather);

-	if (gather->start > start)
-		gather->start =3D start;
+	iommu_iotlb_gather_update_range(gather, iova, size);
 }

 /* PCI device grouping function */
--
2.25.1

--Apple-Mail=_B357E3CE-B050-4CEC-BE04-BB7FB11573D5
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEESJL3osl5Ymx/w9I1HaAqSabaD1oFAmC2gvgACgkQHaAqSaba
D1rJkQ/+JWJnCMtFtwXQ1fbPf1HQn1pRTUeUlsKCx8wzRIaRYDy/Ocd387H4MluD
LVQ/maYXSmXT60XDRhpG9AAa4z9NBRrvpeFkL+V0AzK0l4Xy0u+vzHJA+d9IRJfv
/FurUSv0A5iARwAzm+rBCYNxig0HpoZ/jSnyrtv4WcTZTbLtaWMGbPG2HgwkDc3w
fZv+cHOwPHZvvdFwXu3kaVEmCAnOz3t169D8DQyikC/ua/YVC5BH+USUWZ/ki9f3
Rir6xQLUM9fpYa5Cmr5M1QpBxU50T8OvjpSna/78kchU5/XCySPlFjgXCpBgpxQd
BJzcKCGhuM7Nsaun1AYTGAzfV7Gu80cRxyqtsQf7SxzTuU7qoSGYjy+FAK6fCOYW
Hk6KTyznXk9EgUy85Waki34fgKb9hTocFPnXYRoxpNGw+ig1gtUxXNeztEh4va7i
+67XNikCE/13ah0QuEXv2tmv3Z1OS5HtK5hvSrmtDIMw9He3v7ot+PjRSLU4epAG
XAm1lyrpiBIW8odasMdhX5zu3jB1OpQ+xanxX3IritosVNAeC3hS+lmrsKoJZmkw
s6orm8Ir0rZnJkoE9WKJ0vJkptaveYCoghZ5cKp+mxOvRlBMN2bPoPn1g0x+itUp
fEJxwkOnaFL/x2DuN9GVlSdOHSTRQhrvBB66FD1GISV+2tM7Mkk=
=tXYN
-----END PGP SIGNATURE-----

--Apple-Mail=_B357E3CE-B050-4CEC-BE04-BB7FB11573D5--