[PATCH 0/8] iommu: Clean up map/unmap ops

[PATCH 0/8] iommu: Clean up map/unmap ops

[Robin Murphy]

Hi all,

Time to repay a little more from the technical debt pile and actually
finalise another of our half-finished API evolutions, since it turned
out that by now this was dead simple. So much so in fact that I'm in
two minds whether to squash all the driver patches into one or not, as
they're so very mechanical.

Note that the sun50i patch is just a placeholder to make a consistent
series for now, as I'm anticipating a "proper" implementation from
Jernej.

Thanks,
Robin.


Robin Murphy (8):
  iommu/exynos: Update to {map,unmap}_pages
  iommu/omap: Update to {map,unmap}_pages
  iommu/rockchip: Update to {map,unmap}_pages
  iommu/sun50i: Update to {map,unmap}_pages
  iommu/tegra-gart: Update to {map,unmap}_pages
  iommu/tegra-smmu: Update to {map,unmap}_pages
  iommu: Retire map/unmap ops
  iommu: Improve map/unmap sanity checks

 drivers/iommu/exynos-iommu.c   | 10 +++---
 drivers/iommu/iommu.c          | 66 +++++++++-------------------------
 drivers/iommu/omap-iommu.c     | 11 +++---
 drivers/iommu/rockchip-iommu.c | 11 +++---
 drivers/iommu/sun50i-iommu.c   | 10 +++---
 drivers/iommu/tegra-gart.c     | 12 ++++---
 drivers/iommu/tegra-smmu.c     | 12 ++++---
 include/linux/iommu.h          |  6 ----
 8 files changed, 58 insertions(+), 80 deletions(-)

-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] iommu/exynos: Update to {map,unmap}_pages

[Robin Murphy]

Trivially update map/unmap to the new interface, which is quite happy
for drivers to still process just one page per call.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/exynos-iommu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index c275fe71c4db..8892f5fb6bf0 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1219,7 +1219,7 @@ static int lv2set_page(sysmmu_pte_t *pent, phys_addr_t paddr, size_t size,
  */
 static int exynos_iommu_map(struct iommu_domain *iommu_domain,
 			    unsigned long l_iova, phys_addr_t paddr, size_t size,
-			    int prot, gfp_t gfp)
+			    size_t count, int prot, gfp_t gfp, size_t *mapped)
 {
 	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
 	sysmmu_pte_t *entry;
@@ -1253,6 +1253,8 @@ static int exynos_iommu_map(struct iommu_domain *iommu_domain,
 	if (ret)
 		pr_err("%s: Failed(%d) to map %#zx bytes @ %#x\n",
 			__func__, ret, size, iova);
+	else
+		*mapped = size;
 
 	spin_unlock_irqrestore(&domain->pgtablelock, flags);
 
@@ -1274,7 +1276,7 @@ static void exynos_iommu_tlb_invalidate_entry(struct exynos_iommu_domain *domain
 }
 
 static size_t exynos_iommu_unmap(struct iommu_domain *iommu_domain,
-				 unsigned long l_iova, size_t size,
+				 unsigned long l_iova, size_t size, size_t count,
 				 struct iommu_iotlb_gather *gather)
 {
 	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
@@ -1482,8 +1484,8 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.of_xlate = exynos_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= exynos_iommu_attach_device,
-		.map		= exynos_iommu_map,
-		.unmap		= exynos_iommu_unmap,
+		.map_pages	= exynos_iommu_map,
+		.unmap_pages	= exynos_iommu_unmap,
 		.iova_to_phys	= exynos_iommu_iova_to_phys,
 		.free		= exynos_iommu_domain_free,
 	}
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] iommu/omap: Update to {map,unmap}_pages

[Robin Murphy]

Trivially update map/unmap to the new interface, which is quite happy
for drivers to still process just one page per call.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/omap-iommu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 537e402f9bba..9e07dc94af76 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1318,7 +1318,8 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int pgsz)
 }
 
 static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
-			  phys_addr_t pa, size_t bytes, int prot, gfp_t gfp)
+			  phys_addr_t pa, size_t bytes, size_t count,
+			  int prot, gfp_t gfp, size_t *mapped)
 {
 	struct omap_iommu_domain *omap_domain = to_omap_domain(domain);
 	struct device *dev = omap_domain->dev;
@@ -1356,13 +1357,15 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 			oiommu = iommu->iommu_dev;
 			iopgtable_clear_entry(oiommu, da);
 		}
+	} else {
+		*mapped = bytes;
 	}
 
 	return ret;
 }
 
 static size_t omap_iommu_unmap(struct iommu_domain *domain, unsigned long da,
-			       size_t size, struct iommu_iotlb_gather *gather)
+			       size_t size, size_t count, struct iommu_iotlb_gather *gather)
 {
 	struct omap_iommu_domain *omap_domain = to_omap_domain(domain);
 	struct device *dev = omap_domain->dev;
@@ -1740,8 +1743,8 @@ static const struct iommu_ops omap_iommu_ops = {
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= omap_iommu_attach_dev,
-		.map		= omap_iommu_map,
-		.unmap		= omap_iommu_unmap,
+		.map_pages	= omap_iommu_map,
+		.unmap_pages	= omap_iommu_unmap,
 		.iova_to_phys	= omap_iommu_iova_to_phys,
 		.free		= omap_iommu_domain_free,
 	}
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] iommu/rockchip: Update to {map,unmap}_pages

[Robin Murphy]

Trivially update map/unmap to the new interface, which is quite happy
for drivers to still process just one page per call.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 8ff69fbf9f65..bb00c67c1015 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -817,7 +817,8 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr,
 }
 
 static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova,
-			phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+			phys_addr_t paddr, size_t size, size_t count,
+			int prot, gfp_t gfp, size_t *mapped)
 {
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
 	unsigned long flags;
@@ -850,12 +851,14 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova,
 				paddr, size, prot);
 
 	spin_unlock_irqrestore(&rk_domain->dt_lock, flags);
+	if (!ret)
+		*mapped = size;
 
 	return ret;
 }
 
 static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
-			     size_t size, struct iommu_iotlb_gather *gather)
+			     size_t size, size_t count, struct iommu_iotlb_gather *gather)
 {
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
 	unsigned long flags;
@@ -1197,8 +1200,8 @@ static const struct iommu_ops rk_iommu_ops = {
 	.of_xlate = rk_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= rk_iommu_attach_device,
-		.map		= rk_iommu_map,
-		.unmap		= rk_iommu_unmap,
+		.map_pages	= rk_iommu_map,
+		.unmap_pages	= rk_iommu_unmap,
 		.iova_to_phys	= rk_iommu_iova_to_phys,
 		.free		= rk_iommu_domain_free,
 	}
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] iommu/sun50i: Update to {map,unmap}_pages

[Robin Murphy]

Trivially update map/unmap to the new interface, which is quite happy
for drivers to still process just one page per call.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/sun50i-iommu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 74c5cb93e900..cb4cbba4c22f 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -589,7 +589,8 @@ static u32 *sun50i_dte_get_page_table(struct sun50i_iommu_domain *sun50i_domain,
 }
 
 static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
-			    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+			    phys_addr_t paddr, size_t size, size_t count,
+			    int prot, gfp_t gfp, size_t *mapped)
 {
 	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
 	struct sun50i_iommu *iommu = sun50i_domain->iommu;
@@ -616,13 +617,14 @@ static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
 
 	*pte_addr = sun50i_mk_pte(paddr, prot);
 	sun50i_table_flush(sun50i_domain, pte_addr, 1);
+	*mapped = size;
 
 out:
 	return ret;
 }
 
 static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
-				 size_t size, struct iommu_iotlb_gather *gather)
+				 size_t size, size_t count, struct iommu_iotlb_gather *gather)
 {
 	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
 	phys_addr_t pt_phys;
@@ -838,8 +840,8 @@ static const struct iommu_ops sun50i_iommu_ops = {
 		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
 		.iotlb_sync	= sun50i_iommu_iotlb_sync,
 		.iova_to_phys	= sun50i_iommu_iova_to_phys,
-		.map		= sun50i_iommu_map,
-		.unmap		= sun50i_iommu_unmap,
+		.map_pages	= sun50i_iommu_map,
+		.unmap_pages	= sun50i_iommu_unmap,
 		.free		= sun50i_iommu_domain_free,
 	}
 };
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] iommu/tegra-gart: Update to {map,unmap}_pages

[Robin Murphy]

Trivially update map/unmap to the new interface, which is quite happy
for drivers to still process just one page per call.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/tegra-gart.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a482ff838b53..a0136cd881e6 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -178,7 +178,8 @@ static inline int __gart_iommu_map(struct gart_device *gart, unsigned long iova,
 }
 
 static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
-			  phys_addr_t pa, size_t bytes, int prot, gfp_t gfp)
+			  phys_addr_t pa, size_t bytes, size_t count,
+			  int prot, gfp_t gfp, size_t *mapped)
 {
 	struct gart_device *gart = gart_handle;
 	int ret;
@@ -190,6 +191,9 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	ret = __gart_iommu_map(gart, iova, (unsigned long)pa);
 	spin_unlock(&gart->pte_lock);
 
+	if (!ret)
+		*mapped = bytes;
+
 	return ret;
 }
 
@@ -207,7 +211,7 @@ static inline int __gart_iommu_unmap(struct gart_device *gart,
 }
 
 static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
-			       size_t bytes, struct iommu_iotlb_gather *gather)
+			       size_t bytes, size_t count, struct iommu_iotlb_gather *gather)
 {
 	struct gart_device *gart = gart_handle;
 	int err;
@@ -275,8 +279,8 @@ static const struct iommu_ops gart_iommu_ops = {
 	.of_xlate	= gart_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= gart_iommu_attach_dev,
-		.map		= gart_iommu_map,
-		.unmap		= gart_iommu_unmap,
+		.map_pages	= gart_iommu_map,
+		.unmap_pages	= gart_iommu_unmap,
 		.iova_to_phys	= gart_iommu_iova_to_phys,
 		.iotlb_sync_map	= gart_iommu_sync_map,
 		.iotlb_sync	= gart_iommu_sync,
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] iommu/tegra-smmu: Update to {map,unmap}_pages

[Robin Murphy]

Trivially update map/unmap to the new interface, which is quite happy
for drivers to still process just one page per call.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/tegra-smmu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index e445f80d0226..4cd2153bb076 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -751,7 +751,8 @@ __tegra_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 }
 
 static int tegra_smmu_map(struct iommu_domain *domain, unsigned long iova,
-			  phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+			  phys_addr_t paddr, size_t size, size_t count,
+			  int prot, gfp_t gfp, size_t *mapped)
 {
 	struct tegra_smmu_as *as = to_smmu_as(domain);
 	unsigned long flags;
@@ -761,11 +762,14 @@ static int tegra_smmu_map(struct iommu_domain *domain, unsigned long iova,
 	ret = __tegra_smmu_map(domain, iova, paddr, size, prot, gfp, &flags);
 	spin_unlock_irqrestore(&as->lock, flags);
 
+	if (!ret)
+		*mapped = size;
+
 	return ret;
 }
 
 static size_t tegra_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
-			       size_t size, struct iommu_iotlb_gather *gather)
+			       size_t size, size_t count, struct iommu_iotlb_gather *gather)
 {
 	struct tegra_smmu_as *as = to_smmu_as(domain);
 	unsigned long flags;
@@ -971,8 +975,8 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= tegra_smmu_attach_dev,
-		.map		= tegra_smmu_map,
-		.unmap		= tegra_smmu_unmap,
+		.map_pages	= tegra_smmu_map,
+		.unmap_pages	= tegra_smmu_unmap,
 		.iova_to_phys	= tegra_smmu_iova_to_phys,
 		.free		= tegra_smmu_domain_free,
 	}
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] iommu: Retire map/unmap ops

[Robin Murphy]

With everyone now implementing the new interfaces, clean up the last
remnants of the old map/unmap ops and simplify the calling logic again.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 60 ++++++++++---------------------------------
 include/linux/iommu.h |  6 -----
 2 files changed, 13 insertions(+), 53 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3bfc56df4f78..c83f2e4c56f5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2417,30 +2417,6 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
 	return pgsize;
 }
 
-static int __iommu_map_pages(struct iommu_domain *domain, unsigned long iova,
-			     phys_addr_t paddr, size_t size, int prot,
-			     gfp_t gfp, size_t *mapped)
-{
-	const struct iommu_domain_ops *ops = domain->ops;
-	size_t pgsize, count;
-	int ret;
-
-	pgsize = iommu_pgsize(domain, iova, paddr, size, &count);
-
-	pr_debug("mapping: iova 0x%lx pa %pa pgsize 0x%zx count %zu\n",
-		 iova, &paddr, pgsize, count);
-
-	if (ops->map_pages) {
-		ret = ops->map_pages(domain, iova, paddr, pgsize, count, prot,
-				     gfp, mapped);
-	} else {
-		ret = ops->map(domain, iova, paddr, pgsize, prot, gfp);
-		*mapped = ret ? 0 : pgsize;
-	}
-
-	return ret;
-}
-
 static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
 		       phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
 {
@@ -2451,8 +2427,7 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
 	phys_addr_t orig_paddr = paddr;
 	int ret = 0;
 
-	if (unlikely(!(ops->map || ops->map_pages) ||
-		     domain->pgsize_bitmap == 0UL))
+	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
@@ -2475,10 +2450,14 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
 	pr_debug("map: iova 0x%lx pa %pa size 0x%zx\n", iova, &paddr, size);
 
 	while (size) {
-		size_t mapped = 0;
+		size_t pgsize, count, mapped = 0;
 
-		ret = __iommu_map_pages(domain, iova, paddr, size, prot, gfp,
-					&mapped);
+		pgsize = iommu_pgsize(domain, iova, paddr, size, &count);
+
+		pr_debug("mapping: iova 0x%lx pa %pa pgsize 0x%zx count %zu\n",
+			 iova, &paddr, pgsize, count);
+		ret = ops->map_pages(domain, iova, paddr, pgsize, count, prot,
+				     gfp, &mapped);
 		/*
 		 * Some pages may have been mapped, even if an error occurred,
 		 * so we should account for those so they can be unmapped.
@@ -2522,19 +2501,6 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 }
 EXPORT_SYMBOL_GPL(iommu_map);
 
-static size_t __iommu_unmap_pages(struct iommu_domain *domain,
-				  unsigned long iova, size_t size,
-				  struct iommu_iotlb_gather *iotlb_gather)
-{
-	const struct iommu_domain_ops *ops = domain->ops;
-	size_t pgsize, count;
-
-	pgsize = iommu_pgsize(domain, iova, iova, size, &count);
-	return ops->unmap_pages ?
-	       ops->unmap_pages(domain, iova, pgsize, count, iotlb_gather) :
-	       ops->unmap(domain, iova, pgsize, iotlb_gather);
-}
-
 static size_t __iommu_unmap(struct iommu_domain *domain,
 			    unsigned long iova, size_t size,
 			    struct iommu_iotlb_gather *iotlb_gather)
@@ -2544,8 +2510,7 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
 	unsigned long orig_iova = iova;
 	unsigned int min_pagesz;
 
-	if (unlikely(!(ops->unmap || ops->unmap_pages) ||
-		     domain->pgsize_bitmap == 0UL))
+	if (unlikely(!ops->unmap_pages || domain->pgsize_bitmap == 0UL))
 		return 0;
 
 	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
@@ -2572,9 +2537,10 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
 	 * or we hit an area that isn't mapped.
 	 */
 	while (unmapped < size) {
-		unmapped_page = __iommu_unmap_pages(domain, iova,
-						    size - unmapped,
-						    iotlb_gather);
+		size_t pgsize, count;
+
+		pgsize = iommu_pgsize(domain, iova, iova, size - unmapped, &count);
+		unmapped_page = ops->unmap_pages(domain, iova, pgsize, count, iotlb_gather);
 		if (!unmapped_page)
 			break;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c50a769d569a..5c347029cf70 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -312,10 +312,8 @@ struct iommu_ops {
  * * ENODEV	- device specific errors, not able to be attached
  * * <others>	- treated as ENODEV by the caller. Use is discouraged
  * @set_dev_pasid: set an iommu domain to a pasid of device
- * @map: map a physically contiguous memory region to an iommu domain
  * @map_pages: map a physically contiguous set of pages of the same size to
  *             an iommu domain.
- * @unmap: unmap a physically contiguous memory region from an iommu domain
  * @unmap_pages: unmap a number of pages of the same size from an iommu domain
  * @flush_iotlb_all: Synchronously flush all hardware TLBs for this domain
  * @iotlb_sync_map: Sync mappings created recently using @map to the hardware
@@ -334,13 +332,9 @@ struct iommu_domain_ops {
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
 			     ioasid_t pasid);
 
-	int (*map)(struct iommu_domain *domain, unsigned long iova,
-		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
 	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			 int prot, gfp_t gfp, size_t *mapped);
-	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
-			size_t size, struct iommu_iotlb_gather *iotlb_gather);
 	size_t (*unmap_pages)(struct iommu_domain *domain, unsigned long iova,
 			      size_t pgsize, size_t pgcount,
 			      struct iommu_iotlb_gather *iotlb_gather);
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] iommu: Retire map/unmap ops

[Jason Gunthorpe]

On Tue, Sep 12, 2023 at 05:18:43PM +0100, Robin Murphy wrote:
> With everyone now implementing the new interfaces, clean up the last
> remnants of the old map/unmap ops and simplify the calling logic again.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/iommu.c | 60 ++++++++++---------------------------------
>  include/linux/iommu.h |  6 -----
>  2 files changed, 13 insertions(+), 53 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] iommu: Improve map/unmap sanity checks

[Robin Murphy]

The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
bit stifled, since it is quite likely now that a non-paging domain
won't have a pgsize_bitmap and/or mapping ops, and thus get caught
by the earlier condition anyway. Swap them around to test the more
fundamental condition first, then we can reasonably also upgrade
the other to a WARN_ON, since if a driver does ever expose a paging
domain without the means to actually page, it's clearly very broken.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c83f2e4c56f5..391bcae4d02d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
 	phys_addr_t orig_paddr = paddr;
 	int ret = 0;
 
-	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
-		return -ENODEV;
-
 	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
 		return -EINVAL;
 
+	if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
+		return -ENODEV;
+
 	/* find out the minimum page size supported */
 	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
@@ -2510,10 +2510,10 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
 	unsigned long orig_iova = iova;
 	unsigned int min_pagesz;
 
-	if (unlikely(!ops->unmap_pages || domain->pgsize_bitmap == 0UL))
+	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
 		return 0;
 
-	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
+	if (WARN_ON(!ops->unmap_pages || domain->pgsize_bitmap == 0UL))
 		return 0;
 
 	/* find out the minimum page size supported */
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Jason Gunthorpe]

On Tue, Sep 12, 2023 at 05:18:44PM +0100, Robin Murphy wrote:
> The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
> bit stifled, since it is quite likely now that a non-paging domain
> won't have a pgsize_bitmap and/or mapping ops, and thus get caught
> by the earlier condition anyway. Swap them around to test the more
> fundamental condition first, then we can reasonably also upgrade
> the other to a WARN_ON, since if a driver does ever expose a paging
> domain without the means to actually page, it's clearly very broken.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/iommu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c83f2e4c56f5..391bcae4d02d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
>  	phys_addr_t orig_paddr = paddr;
>  	int ret = 0;
>  
> -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> -		return -ENODEV;
> -
>  	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
>  		return -EINVAL;

Why isn't this a WARN_ON? The caller is clearly lost its mind if it
is calling map on a non paging domain..

> +	if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> +		return -ENODEV;
> +

And these could be moved to after attach so they are not in any fast
path, and eventually to after alloc..

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Robin Murphy]

On 2023-09-13 15:39, Jason Gunthorpe wrote:
> On Tue, Sep 12, 2023 at 05:18:44PM +0100, Robin Murphy wrote:
>> The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
>> bit stifled, since it is quite likely now that a non-paging domain
>> won't have a pgsize_bitmap and/or mapping ops, and thus get caught
>> by the earlier condition anyway. Swap them around to test the more
>> fundamental condition first, then we can reasonably also upgrade
>> the other to a WARN_ON, since if a driver does ever expose a paging
>> domain without the means to actually page, it's clearly very broken.
> 
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/iommu.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index c83f2e4c56f5..391bcae4d02d 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
>>   	phys_addr_t orig_paddr = paddr;
>>   	int ret = 0;
>>   
>> -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
>> -		return -ENODEV;
>> -
>>   	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
>>   		return -EINVAL;
> 
> Why isn't this a WARN_ON? The caller is clearly lost its mind if it
> is calling map on a non paging domain..

Sure it's a dumb thing to do, however I don't think we can reasonably 
say without question that an external caller being dumb represents an 
unexpected and serious loss of internal consistency.

>> +	if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
>> +		return -ENODEV;
>> +
> 
> And these could be moved to after attach so they are not in any fast
> path, and eventually to after alloc..

Perhaps, although TBH I can't imagine it making any appreciable 
difference - they're both values we need to load and use soon anyway, so 
on a sensible CPU, the additional overhead should only really be a 
couple of not-taken branch-if-zero instructions, which is nothing at all 
compared to how much goes on in the main loop and the driver op call itself.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Jason Gunthorpe]

On Wed, Sep 13, 2023 at 07:46:45PM +0100, Robin Murphy wrote:
> On 2023-09-13 15:39, Jason Gunthorpe wrote:
> > On Tue, Sep 12, 2023 at 05:18:44PM +0100, Robin Murphy wrote:
> > > The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
> > > bit stifled, since it is quite likely now that a non-paging domain
> > > won't have a pgsize_bitmap and/or mapping ops, and thus get caught
> > > by the earlier condition anyway. Swap them around to test the more
> > > fundamental condition first, then we can reasonably also upgrade
> > > the other to a WARN_ON, since if a driver does ever expose a paging
> > > domain without the means to actually page, it's clearly very broken.
> > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/iommu/iommu.c | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index c83f2e4c56f5..391bcae4d02d 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> > >   	phys_addr_t orig_paddr = paddr;
> > >   	int ret = 0;
> > > -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> > > -		return -ENODEV;
> > > -
> > >   	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
> > >   		return -EINVAL;
> > 
> > Why isn't this a WARN_ON? The caller is clearly lost its mind if it
> > is calling map on a non paging domain..
> 
> Sure it's a dumb thing to do, however I don't think we can reasonably say
> without question that an external caller being dumb represents an unexpected
> and serious loss of internal consistency.

WARN_ON is not just for "serious loss of internal consistency" it
should be use in all places where invariant are violated. We can't
guess why the caller is using this wrong (most likely it is UAF or
memory corruption), but if this fires something definately has gone
wrong with the kernel.

eg if syzkaller somehow hits this we want the WARN_ON so it reports
it.

> > > +	if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> > > +		return -ENODEV;
> > > +
> > 
> > And these could be moved to after attach so they are not in any fast
> > path, and eventually to after alloc..
> 
> Perhaps, although TBH I can't imagine it making any appreciable difference -
> they're both values we need to load and use soon anyway, so on a sensible
> CPU, the additional overhead should only really be a couple of not-taken
> branch-if-zero instructions, which is nothing at all compared to how much
> goes on in the main loop and the driver op call itself.

IMHO it is a cleaner pattern to check the objects the driver creates
when it creates them, not when we go to try to use them..

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Robin Murphy]

On 2023-09-14 13:48, Jason Gunthorpe wrote:
> On Wed, Sep 13, 2023 at 07:46:45PM +0100, Robin Murphy wrote:
>> On 2023-09-13 15:39, Jason Gunthorpe wrote:
>>> On Tue, Sep 12, 2023 at 05:18:44PM +0100, Robin Murphy wrote:
>>>> The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
>>>> bit stifled, since it is quite likely now that a non-paging domain
>>>> won't have a pgsize_bitmap and/or mapping ops, and thus get caught
>>>> by the earlier condition anyway. Swap them around to test the more
>>>> fundamental condition first, then we can reasonably also upgrade
>>>> the other to a WARN_ON, since if a driver does ever expose a paging
>>>> domain without the means to actually page, it's clearly very broken.
>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index c83f2e4c56f5..391bcae4d02d 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
>>>>    	phys_addr_t orig_paddr = paddr;
>>>>    	int ret = 0;
>>>> -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
>>>> -		return -ENODEV;
>>>> -
>>>>    	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
>>>>    		return -EINVAL;
>>>
>>> Why isn't this a WARN_ON? The caller is clearly lost its mind if it
>>> is calling map on a non paging domain..
>>
>> Sure it's a dumb thing to do, however I don't think we can reasonably say
>> without question that an external caller being dumb represents an unexpected
>> and serious loss of internal consistency.
> 
> WARN_ON is not just for "serious loss of internal consistency" it
> should be use in all places where invariant are violated. We can't
> guess why the caller is using this wrong (most likely it is UAF or
> memory corruption), but if this fires something definately has gone
> wrong with the kernel.

Has it? It's not *functionally* incorrect to obtain a valid domain by 
calling iommu_get_domain_for_dev(), pass that domain to iommu_map(), and 
handle any failure returned. Sure, it's *semantically* questionable, but 
so is calling iommu_iova_to_phys() on non-paging domains, and we have to 
support that, because callers are dumb, and "callers aren't dumb" is not 
a realistic invariant which we can uphold.

> eg if syzkaller somehow hits this we want the WARN_ON so it reports
> it.

But then an "IOMMU bug" is reported to us, and we say "yeah, that's not 
ours, that's some code somewhere down in the middle of that callstack 
being dumb", and then what? I know I don't have the time or inclination 
to go off debugging and redesigning random other bits of the kernel for 
calling our API in ways that look sketchy but aren't technically 
invalid, do you?

iommu_map() can already fail for numerous reasons that may or may not 
represent a bug in the caller, like the IOVA or PA being out-of-range, 
or the IOVA already being mapped, or whatever. The wrong type of domain 
is just another reason to fail (in, as far as we're concerned, an 
entirely safe and manageable way). Callers have to be prepared to handle 
failure, but it's up to them how unexpected or serious it is; we can't 
second-guess them.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Jason Gunthorpe]

On Thu, Sep 14, 2023 at 03:23:46PM +0100, Robin Murphy wrote:
> > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > index c83f2e4c56f5..391bcae4d02d 100644
> > > > > --- a/drivers/iommu/iommu.c
> > > > > +++ b/drivers/iommu/iommu.c
> > > > > @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
> > > > >    	phys_addr_t orig_paddr = paddr;
> > > > >    	int ret = 0;
> > > > > -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
> > > > > -		return -ENODEV;
> > > > > -
> > > > >    	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
> > > > >    		return -EINVAL;
> > > > 
> > > > Why isn't this a WARN_ON? The caller is clearly lost its mind if it
> > > > is calling map on a non paging domain..
> > > 
> > > Sure it's a dumb thing to do, however I don't think we can reasonably say
> > > without question that an external caller being dumb represents an unexpected
> > > and serious loss of internal consistency.
> > 
> > WARN_ON is not just for "serious loss of internal consistency" it
> > should be use in all places where invariant are violated. We can't
> > guess why the caller is using this wrong (most likely it is UAF or
> > memory corruption), but if this fires something definately has gone
> > wrong with the kernel.
> 
> Has it? It's not *functionally* incorrect to obtain a valid domain by
> calling iommu_get_domain_for_dev(), pass that domain to iommu_map(), and
> handle any failure returned. 

This is not a theoretical question - does any in-kernel code actually
do that and expect it to work? I didn't notice any..

It doesn't matter that someone *could*, our task is not to make an
overly general API. We can, and should, have tight invarients because
it allows discovery of more bug classes, and prevents "creative" use
of APIs.

> is calling iommu_iova_to_phys() on non-paging domains, and we have to
> support that, because callers are dumb, and "callers aren't dumb" is not a
> realistic invariant which we can uphold.

Someone does that? :(

> > eg if syzkaller somehow hits this we want the WARN_ON so it reports
> > it.
> 
> But then an "IOMMU bug" is reported to us, and we say "yeah, that's not
> ours, that's some code somewhere down in the middle of that callstack being
> dumb", and then what?

The report gets handed off to the part of the callstack that is making
the problem.

> iommu_map() can already fail for numerous reasons that may or may not
> represent a bug in the caller, like the IOVA or PA being out-of-range, or
> the IOVA already being mapped, or whatever. 

IMHO some of those certainly could be WARN too, depends what you think
the API should be.

eg iommufd and vfio both are designed to never double map/use out of
aperture IOVA/etc, if they do it is a significant bug and a WARN_ON
would be welcomed.

> another reason to fail (in, as far as we're concerned, an entirely safe and
> manageable way). Callers have to be prepared to handle failure, but it's up
> to them how unexpected or serious it is; we can't second-guess them.

We can always convert things into error codes, but that can also hide
bugs if no existing caller needs that functionality. IMHO it is always
better to have tighter invarients, and be noisy when they fail. This
gives us better understandablity, bug discoverability and discourages
people from introducing new code doing crazy things.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Robin Murphy]

On 2023-09-14 17:48, Jason Gunthorpe wrote:
> On Thu, Sep 14, 2023 at 03:23:46PM +0100, Robin Murphy wrote:
>>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>>> index c83f2e4c56f5..391bcae4d02d 100644
>>>>>> --- a/drivers/iommu/iommu.c
>>>>>> +++ b/drivers/iommu/iommu.c
>>>>>> @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>>     	phys_addr_t orig_paddr = paddr;
>>>>>>     	int ret = 0;
>>>>>> -	if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
>>>>>> -		return -ENODEV;
>>>>>> -
>>>>>>     	if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
>>>>>>     		return -EINVAL;
>>>>>
>>>>> Why isn't this a WARN_ON? The caller is clearly lost its mind if it
>>>>> is calling map on a non paging domain..
>>>>
>>>> Sure it's a dumb thing to do, however I don't think we can reasonably say
>>>> without question that an external caller being dumb represents an unexpected
>>>> and serious loss of internal consistency.
>>>
>>> WARN_ON is not just for "serious loss of internal consistency" it
>>> should be use in all places where invariant are violated. We can't
>>> guess why the caller is using this wrong (most likely it is UAF or
>>> memory corruption), but if this fires something definately has gone
>>> wrong with the kernel.
>>
>> Has it? It's not *functionally* incorrect to obtain a valid domain by
>> calling iommu_get_domain_for_dev(), pass that domain to iommu_map(), and
>> handle any failure returned.
> 
> This is not a theoretical question - does any in-kernel code actually
> do that and expect it to work? I didn't notice any..

I was hoping to have to say I'd only seen it in unmerged (e.g. [1]) and 
downstream code, but then I found drivers/net/ipa/ :(

(and amusingly, the line I happened to look at first blamed you, but 
that was only for that mechanical change of the iommu_map() prototype)

> It doesn't matter that someone *could*, our task is not to make an
> overly general API. We can, and should, have tight invarients because
> it allows discovery of more bug classes, and prevents "creative" use
> of APIs.
> 
>> is calling iommu_iova_to_phys() on non-paging domains, and we have to
>> support that, because callers are dumb, and "callers aren't dumb" is not a
>> realistic invariant which we can uphold.
> 
> Someone does that? :(

Only every user of iommu_iova_to_phys() outside of drivers/iommu/, VFIO, 
and the arch/arm DMA ops...

>>> eg if syzkaller somehow hits this we want the WARN_ON so it reports
>>> it.
>>
>> But then an "IOMMU bug" is reported to us, and we say "yeah, that's not
>> ours, that's some code somewhere down in the middle of that callstack being
>> dumb", and then what?
> 
> The report gets handed off to the part of the callstack that is making
> the problem.

And if it's critical to them that a mapping succeeds, they should 
probably have their own warning already. An inappropriate domain type is 
far from critical to us, and is something we've already allowed to fail 
cleanly and gracefully for the best part of a decade.

>> iommu_map() can already fail for numerous reasons that may or may not
>> represent a bug in the caller, like the IOVA or PA being out-of-range, or
>> the IOVA already being mapped, or whatever.
> 
> IMHO some of those certainly could be WARN too, depends what you think
> the API should be.

We don't even have consistent behaviour across IOMMU drivers for most of 
them, and some callers are written around particular driver-specific 
behaviours (yes, even VFIO), so IMO there's no way we're in a position 
to suddenly decree that anyone's fundamentally wrong for doing something 
that we've long considered a non-critical failure, but have now decided 
looks sketchy.

> eg iommufd and vfio both are designed to never double map/use out of
> aperture IOVA/etc, if they do it is a significant bug and a WARN_ON
> would be welcomed.
> 
>> another reason to fail (in, as far as we're concerned, an entirely safe and
>> manageable way). Callers have to be prepared to handle failure, but it's up
>> to them how unexpected or serious it is; we can't second-guess them.
> 
> We can always convert things into error codes, but that can also hide
> bugs if no existing caller needs that functionality. IMHO it is always
> better to have tighter invarients, and be noisy when they fail. This
> gives us better understandablity, bug discoverability and discourages
> people from introducing new code doing crazy things.
All that said, I don't mind if you want to propose a separate patch to 
turn certain external caller conditions into noisy warnings, but I 
wouldn't want to tie it up with any other changes which would risk 
getting caught in the crossfire if someone does then hit a 
newly-introduced warning and start arguing to revert it. And I'm not 
going to do so in any of my patches since as above I don't personally 
believe it's a valuable thing to do.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] iommu: Improve map/unmap sanity checks

[Jason Gunthorpe]

On Tue, Sep 19, 2023 at 01:18:31PM +0100, Robin Murphy wrote:

> > This is not a theoretical question - does any in-kernel code actually
> > do that and expect it to work? I didn't notice any..
> 
> I was hoping to have to say I'd only seen it in unmerged (e.g. [1]) and
> downstream code, but then I found drivers/net/ipa/ :(

Aiiie, what is that doing? Mapping something into the dma-api owned
domain!?!

See this is why I think stronger invarients are a good idea, we can't
police the entire kernel but we can make the entry points explode if
someone is doing something WRONG.

> All that said, I don't mind if you want to propose a separate patch to turn
> certain external caller conditions into noisy warnings, but I wouldn't want
> to tie it up with any other changes which would risk getting caught in the
> crossfire if someone does then hit a newly-introduced warning and start
> arguing to revert it. And I'm not going to do so in any of my patches since
> as above I don't personally believe it's a valuable thing to do.

Ok, that make sense

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] iommu: Clean up map/unmap ops

[Joerg Roedel]

On Tue, Sep 12, 2023 at 05:18:36PM +0100, Robin Murphy wrote:
> Robin Murphy (8):
>   iommu/exynos: Update to {map,unmap}_pages
>   iommu/omap: Update to {map,unmap}_pages
>   iommu/rockchip: Update to {map,unmap}_pages
>   iommu/sun50i: Update to {map,unmap}_pages
>   iommu/tegra-gart: Update to {map,unmap}_pages
>   iommu/tegra-smmu: Update to {map,unmap}_pages
>   iommu: Retire map/unmap ops
>   iommu: Improve map/unmap sanity checks

Applied all except the tegra-gart patch. Tegra-gart has been removed by
Jason in a previous patch. Thanks Robin.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] iommu: Clean up map/unmap ops

[Jernej Škrabec]

Dne ponedeljek, 25. september 2023 ob 12:05:35 CEST je Joerg Roedel napisal(a):
> On Tue, Sep 12, 2023 at 05:18:36PM +0100, Robin Murphy wrote:
> > Robin Murphy (8):
> >   iommu/exynos: Update to {map,unmap}_pages
> >   iommu/omap: Update to {map,unmap}_pages
> >   iommu/rockchip: Update to {map,unmap}_pages
> >   iommu/sun50i: Update to {map,unmap}_pages
> >   iommu/tegra-gart: Update to {map,unmap}_pages
> >   iommu/tegra-smmu: Update to {map,unmap}_pages
> >   iommu: Retire map/unmap ops
> >   iommu: Improve map/unmap sanity checks
> 
> Applied all except the tegra-gart patch. Tegra-gart has been removed by
> Jason in a previous patch. Thanks Robin.

sun50i is meant to be placeholder (as mentioned in cover letter). It's
replaced by:
https://lore.kernel.org/linux-sunxi/20230912181941.2971036-1-jernej.skrabec@gmail.com/

But I didn't get any feedback if it's ok or there is anything to fix.

Best regards,
Jernej




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] iommu: Clean up map/unmap ops

[Joerg Roedel]

On Mon, Sep 25, 2023 at 05:33:58PM +0200, Jernej Škrabec wrote:
> sun50i is meant to be placeholder (as mentioned in cover letter). It's
> replaced by:
> https://lore.kernel.org/linux-sunxi/20230912181941.2971036-1-jernej.skrabec@gmail.com/

Is the placeholder patch actually breaking the driver or just not
optimal yet?

Regards,

	Joerg

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] iommu: Clean up map/unmap ops

[Robin Murphy]

On 26/09/2023 10:23 am, Joerg Roedel wrote:
> On Mon, Sep 25, 2023 at 05:33:58PM +0200, Jernej Škrabec wrote:
>> sun50i is meant to be placeholder (as mentioned in cover letter). It's
>> replaced by:
>> https://lore.kernel.org/linux-sunxi/20230912181941.2971036-1-jernej.skrabec@gmail.com/
> 
> Is the placeholder patch actually breaking the driver or just not
> optimal yet?

No functional issue, and TBH I think there's no harm in avoiding the 
dependency (I was kind of assuming you would pick this series to the 
core branch, so the "placeholder" status might extend that far too) - it 
should just mean that when you pick Jernej's patch to a topic branch 
you'll get a conflict merging it to core, and the correct resolution 
will be to take everything from that branch and replace my patch.

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] iommu: Clean up map/unmap ops

[Joerg Roedel]

On Tue, Sep 26, 2023 at 01:01:28PM +0100, Robin Murphy wrote:
> No functional issue, and TBH I think there's no harm in avoiding the
> dependency (I was kind of assuming you would pick this series to the core
> branch, so the "placeholder" status might extend that far too) - it should
> just mean that when you pick Jernej's patch to a topic branch you'll get a
> conflict merging it to core, and the correct resolution will be to take
> everything from that branch and replace my patch.

Alright, thanks for clarifying. I hereby ask Jernej to base his new
patch on the core branch, once I published it (which should be later
today).

Regards,

	Joerg

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel