[PATCH v4 0/3] iommu: Clean up cookie and sw_msi in struct iommu_domain

[PATCH v4 0/3] iommu: Clean up cookie and sw_msi in struct iommu_domain

[Nicolin Chen]

A few things in iommu_domain structure (such as iova_cookie, iommufd_hwpt,
fault handler, and etc) are exclusive to certain types of iommu_domains.
So, all of them can be seen as domain cookies in different cookie types.

Furthermore, iova_cookie is shared with the msi_cookie use case. It could
be cleaner by decoupling these too.

Define an enum iommu_domain_cookie_type to fit all those cookies in.

Since there are only two sw_msi implementations, check domain->cookie_type
in iommu_dma_prepare_msi() to call the corresponding sw_msi function.

This is a clean-up series for the previous sw_msi Part-1 core series. It's
on github:
https://github.com/nicolinc/iommufd/commits/iommufd_msi_cleanup-v4

Changelog
v4
 * Rebase on top of a bug fix for hwpt_iommufd cookie
   https://lore.kernel.org/all/20250305211800.229465-1-nicolinc@nvidia.com/
 * Add Reviewed-by from Robin and Kevin
 * Add missing "break;" in iommu_domain_free()
 * Drop "linux/msi.h" header from the iommufd/device code
 * Set iommufd_sw_msi_install's SYMBOL to "IOMMUFD_INTERNAL"
 * Update commit messages for typo fixing and size comparison
 * Add a local msi_page_list to cache cookie_msi_pages(domain) return
 * Add an inline iommufd_group_setup_msi() for !CONFIG_IRQ_MSI_IOMMU
v3
 https://lore.kernel.org/all/cover.1741034885.git.nicolinc@nvidia.com/
 * Integrate Robin's patch
   https://lore.kernel.org/linux-iommu/4e68d5820be06adc1b34fc0d1c9399481151daee.1740742271.git.robin.murphy@arm.com/
   replacing private_data_owner with the new cookie_type
 * Move iommu_sw_msi stub from the public header to iommu-priv.h
v2
 https://lore.kernel.org/all/cover.1740705776.git.nicolinc@nvidia.com/
 * Drop sw_msi function pointer
 * Add a new private_data_owner tag in iommu_domain
 * Let iommu core call the two sw_msi implementations directly
v1
 https://lore.kernel.org/all/cover.1740600272.git.nicolinc@nvidia.com/

Thanks
Nicolin

Nicolin Chen (2):
  iommufd: Move iommufd_sw_msi and related functions to driver.c
  iommu: Drop sw_msi from iommu_domain

Robin Murphy (1):
  iommu: Sort out domain user data

 drivers/iommu/dma-iommu.h               |  14 ++
 drivers/iommu/iommu-priv.h              |  13 ++
 drivers/iommu/iommufd/iommufd_private.h |   7 +-
 include/linux/iommu.h                   |  35 ++--
 drivers/iommu/dma-iommu.c               | 208 ++++++++++++------------
 drivers/iommu/iommu-sva.c               |   1 +
 drivers/iommu/iommu.c                   |  35 +++-
 drivers/iommu/iommufd/device.c          | 131 +--------------
 drivers/iommu/iommufd/driver.c          | 125 ++++++++++++++
 drivers/iommu/iommufd/hw_pagetable.c    |   6 +-
 10 files changed, 320 insertions(+), 255 deletions(-)


base-commit: 4b022ff93d8b332891cae818a7331942db50610f
-- 
2.43.0

[PATCH v4 1/3] iommu: Sort out domain user data

[Nicolin Chen]

From: Robin Murphy <robin.murphy@arm.com>

When DMA/MSI cookies were made first-class citizens back in commit
46983fcd67ac ("iommu: Pull IOVA cookie management into the core"), there
was no real need to further expose the two different cookie types.
However, now that IOMMUFD wants to add a third type of MSI-mapping
cookie, we do have a nicely compelling reason to properly dismabiguate
things at the domain level beyond just vaguely guessing from the domain
type.

Meanwhile, we also effectively have another "cookie" in the form of the
anonymous union for other user data, which isn't much better in terms of
being vague and unenforced. The fact is that all these cookie types are
mutually exclusive, in the sense that combining them makes zero sense
and/or would be catastrophic (iommu_set_fault_handler() on an SVA
domain, anyone?) - the only combination which *might* be reasonable is
perhaps a fault handler and an MSI cookie, but nobody's doing that at
the moment, so let's rule it out as well for the sake of being clear and
robust. To that end, we pull DMA and MSI cookies apart a little more,
mostly to clear up the ambiguity at domain teardown, then for clarity
(and to save a little space), move them into the union, whose ownership
we can then properly describe and enforce entirely unambiguously.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
[nicolinc: rebase on latest tree; use prefix IOMMU_COOKIE_; merge unions
           in iommu_domain; add IOMMU_COOKIE_IOMMUFD for iommufd_hwpt]
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/dma-iommu.h            |   5 +
 include/linux/iommu.h                |  20 ++-
 drivers/iommu/dma-iommu.c            | 194 ++++++++++++++-------------
 drivers/iommu/iommu-sva.c            |   1 +
 drivers/iommu/iommu.c                |  18 ++-
 drivers/iommu/iommufd/hw_pagetable.c |   3 +
 6 files changed, 143 insertions(+), 98 deletions(-)

diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
index c12d63457c76..9cca11806e5d 100644
--- a/drivers/iommu/dma-iommu.h
+++ b/drivers/iommu/dma-iommu.h
@@ -13,6 +13,7 @@ void iommu_setup_dma_ops(struct device *dev);
 
 int iommu_get_dma_cookie(struct iommu_domain *domain);
 void iommu_put_dma_cookie(struct iommu_domain *domain);
+void iommu_put_msi_cookie(struct iommu_domain *domain);
 
 int iommu_dma_init_fq(struct iommu_domain *domain);
 
@@ -40,6 +41,10 @@ static inline void iommu_put_dma_cookie(struct iommu_domain *domain)
 {
 }
 
+static inline void iommu_put_msi_cookie(struct iommu_domain *domain)
+{
+}
+
 static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 {
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e93d2e918599..06cc14e9993d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -41,6 +41,7 @@ struct iommu_dirty_ops;
 struct notifier_block;
 struct iommu_sva;
 struct iommu_dma_cookie;
+struct iommu_dma_msi_cookie;
 struct iommu_fault_param;
 struct iommufd_ctx;
 struct iommufd_viommu;
@@ -165,6 +166,15 @@ struct iommu_domain_geometry {
 	bool force_aperture;       /* DMA only allowed in mappable range? */
 };
 
+enum iommu_domain_cookie_type {
+	IOMMU_COOKIE_NONE,
+	IOMMU_COOKIE_DMA_IOVA,
+	IOMMU_COOKIE_DMA_MSI,
+	IOMMU_COOKIE_FAULT_HANDLER,
+	IOMMU_COOKIE_SVA,
+	IOMMU_COOKIE_IOMMUFD,
+};
+
 /* Domain feature flags */
 #define __IOMMU_DOMAIN_PAGING	(1U << 0)  /* Support for iommu_map/unmap */
 #define __IOMMU_DOMAIN_DMA_API	(1U << 1)  /* Domain for use in DMA-API
@@ -211,12 +221,12 @@ struct iommu_domain_geometry {
 
 struct iommu_domain {
 	unsigned type;
+	enum iommu_domain_cookie_type cookie_type;
 	const struct iommu_domain_ops *ops;
 	const struct iommu_dirty_ops *dirty_ops;
 	const struct iommu_ops *owner; /* Whose domain_alloc we came from */
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	struct iommu_domain_geometry geometry;
-	struct iommu_dma_cookie *iova_cookie;
 	int (*iopf_handler)(struct iopf_group *group);
 
 #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
@@ -224,10 +234,10 @@ struct iommu_domain {
 		      phys_addr_t msi_addr);
 #endif
 
-	union { /* Pointer usable by owner of the domain */
-		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
-	};
-	union { /* Fault handler */
+	union { /* cookie */
+		struct iommu_dma_cookie *iova_cookie;
+		struct iommu_dma_msi_cookie *msi_cookie;
+		struct iommufd_hw_pagetable *iommufd_hwpt;
 		struct {
 			iommu_fault_handler_t handler;
 			void *handler_token;
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 94263ed2c564..31a7b4b81656 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -42,11 +42,6 @@ struct iommu_dma_msi_page {
 	phys_addr_t		phys;
 };
 
-enum iommu_dma_cookie_type {
-	IOMMU_DMA_IOVA_COOKIE,
-	IOMMU_DMA_MSI_COOKIE,
-};
-
 enum iommu_dma_queue_type {
 	IOMMU_DMA_OPTS_PER_CPU_QUEUE,
 	IOMMU_DMA_OPTS_SINGLE_QUEUE,
@@ -59,35 +54,31 @@ struct iommu_dma_options {
 };
 
 struct iommu_dma_cookie {
-	enum iommu_dma_cookie_type	type;
+	struct iova_domain iovad;
+	struct list_head msi_page_list;
+	/* Flush queue */
 	union {
-		/* Full allocator for IOMMU_DMA_IOVA_COOKIE */
-		struct {
-			struct iova_domain	iovad;
-			/* Flush queue */
-			union {
-				struct iova_fq	*single_fq;
-				struct iova_fq	__percpu *percpu_fq;
-			};
-			/* Number of TLB flushes that have been started */
-			atomic64_t		fq_flush_start_cnt;
-			/* Number of TLB flushes that have been finished */
-			atomic64_t		fq_flush_finish_cnt;
-			/* Timer to regularily empty the flush queues */
-			struct timer_list	fq_timer;
-			/* 1 when timer is active, 0 when not */
-			atomic_t		fq_timer_on;
-		};
-		/* Trivial linear page allocator for IOMMU_DMA_MSI_COOKIE */
-		dma_addr_t		msi_iova;
+		struct iova_fq *single_fq;
+		struct iova_fq __percpu *percpu_fq;
 	};
-	struct list_head		msi_page_list;
-
+	/* Number of TLB flushes that have been started */
+	atomic64_t fq_flush_start_cnt;
+	/* Number of TLB flushes that have been finished */
+	atomic64_t fq_flush_finish_cnt;
+	/* Timer to regularily empty the flush queues */
+	struct timer_list fq_timer;
+	/* 1 when timer is active, 0 when not */
+	atomic_t fq_timer_on;
 	/* Domain for flush queue callback; NULL if flush queue not in use */
-	struct iommu_domain		*fq_domain;
+	struct iommu_domain *fq_domain;
 	/* Options for dma-iommu use */
-	struct iommu_dma_options	options;
-	struct mutex			mutex;
+	struct iommu_dma_options options;
+	struct mutex mutex;
+};
+
+struct iommu_dma_msi_cookie {
+	dma_addr_t msi_iova;
+	struct list_head msi_page_list;
 };
 
 static DEFINE_STATIC_KEY_FALSE(iommu_deferred_attach_enabled);
@@ -369,40 +360,26 @@ int iommu_dma_init_fq(struct iommu_domain *domain)
 	return 0;
 }
 
-static inline size_t cookie_msi_granule(struct iommu_dma_cookie *cookie)
-{
-	if (cookie->type == IOMMU_DMA_IOVA_COOKIE)
-		return cookie->iovad.granule;
-	return PAGE_SIZE;
-}
-
-static struct iommu_dma_cookie *cookie_alloc(enum iommu_dma_cookie_type type)
-{
-	struct iommu_dma_cookie *cookie;
-
-	cookie = kzalloc(sizeof(*cookie), GFP_KERNEL);
-	if (cookie) {
-		INIT_LIST_HEAD(&cookie->msi_page_list);
-		cookie->type = type;
-	}
-	return cookie;
-}
-
 /**
  * iommu_get_dma_cookie - Acquire DMA-API resources for a domain
  * @domain: IOMMU domain to prepare for DMA-API usage
  */
 int iommu_get_dma_cookie(struct iommu_domain *domain)
 {
-	if (domain->iova_cookie)
+	struct iommu_dma_cookie *cookie;
+
+	if (domain->cookie_type != IOMMU_COOKIE_NONE)
 		return -EEXIST;
 
-	domain->iova_cookie = cookie_alloc(IOMMU_DMA_IOVA_COOKIE);
-	if (!domain->iova_cookie)
+	cookie = kzalloc(sizeof(*cookie), GFP_KERNEL);
+	if (!cookie)
 		return -ENOMEM;
 
-	mutex_init(&domain->iova_cookie->mutex);
+	mutex_init(&cookie->mutex);
+	INIT_LIST_HEAD(&cookie->msi_page_list);
 	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
+	domain->cookie_type = IOMMU_COOKIE_DMA_IOVA;
+	domain->iova_cookie = cookie;
 	return 0;
 }
 
@@ -420,29 +397,30 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
  */
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 {
-	struct iommu_dma_cookie *cookie;
+	struct iommu_dma_msi_cookie *cookie;
 
 	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
 		return -EINVAL;
 
-	if (domain->iova_cookie)
+	if (domain->cookie_type != IOMMU_COOKIE_NONE)
 		return -EEXIST;
 
-	cookie = cookie_alloc(IOMMU_DMA_MSI_COOKIE);
+	cookie = kzalloc(sizeof(*cookie), GFP_KERNEL);
 	if (!cookie)
 		return -ENOMEM;
 
 	cookie->msi_iova = base;
-	domain->iova_cookie = cookie;
+	INIT_LIST_HEAD(&cookie->msi_page_list);
 	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
+	domain->cookie_type = IOMMU_COOKIE_DMA_MSI;
+	domain->msi_cookie = cookie;
 	return 0;
 }
 EXPORT_SYMBOL(iommu_get_msi_cookie);
 
 /**
  * iommu_put_dma_cookie - Release a domain's DMA mapping resources
- * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() or
- *          iommu_get_msi_cookie()
+ * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
  */
 void iommu_put_dma_cookie(struct iommu_domain *domain)
 {
@@ -454,20 +432,27 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 		return;
 #endif
 
-	if (!cookie)
-		return;
-
-	if (cookie->type == IOMMU_DMA_IOVA_COOKIE && cookie->iovad.granule) {
+	if (cookie->iovad.granule) {
 		iommu_dma_free_fq(cookie);
 		put_iova_domain(&cookie->iovad);
 	}
+	list_for_each_entry_safe(msi, tmp, &cookie->msi_page_list, list)
+		kfree(msi);
+	kfree(cookie);
+}
 
-	list_for_each_entry_safe(msi, tmp, &cookie->msi_page_list, list) {
-		list_del(&msi->list);
+/**
+ * iommu_put_msi_cookie - Release a domain's MSI mapping resources
+ * @domain: IOMMU domain previously prepared by iommu_get_msi_cookie()
+ */
+void iommu_put_msi_cookie(struct iommu_domain *domain)
+{
+	struct iommu_dma_msi_cookie *cookie = domain->msi_cookie;
+	struct iommu_dma_msi_page *msi, *tmp;
+
+	list_for_each_entry_safe(msi, tmp, &cookie->msi_page_list, list)
 		kfree(msi);
-	}
 	kfree(cookie);
-	domain->iova_cookie = NULL;
 }
 
 /**
@@ -687,7 +672,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev
 	struct iova_domain *iovad;
 	int ret;
 
-	if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE)
+	if (!cookie || domain->cookie_type != IOMMU_COOKIE_DMA_IOVA)
 		return -EINVAL;
 
 	iovad = &cookie->iovad;
@@ -777,9 +762,9 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 	struct iova_domain *iovad = &cookie->iovad;
 	unsigned long shift, iova_len, iova;
 
-	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
-		cookie->msi_iova += size;
-		return cookie->msi_iova - size;
+	if (domain->cookie_type == IOMMU_COOKIE_DMA_MSI) {
+		domain->msi_cookie->msi_iova += size;
+		return domain->msi_cookie->msi_iova - size;
 	}
 
 	shift = iova_shift(iovad);
@@ -816,16 +801,16 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 	return (dma_addr_t)iova << shift;
 }
 
-static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
-		dma_addr_t iova, size_t size, struct iommu_iotlb_gather *gather)
+static void iommu_dma_free_iova(struct iommu_domain *domain, dma_addr_t iova,
+				size_t size, struct iommu_iotlb_gather *gather)
 {
-	struct iova_domain *iovad = &cookie->iovad;
+	struct iova_domain *iovad = &domain->iova_cookie->iovad;
 
 	/* The MSI case is only ever cleaning up its most recent allocation */
-	if (cookie->type == IOMMU_DMA_MSI_COOKIE)
-		cookie->msi_iova -= size;
+	if (domain->cookie_type == IOMMU_COOKIE_DMA_MSI)
+		domain->msi_cookie->msi_iova -= size;
 	else if (gather && gather->queued)
-		queue_iova(cookie, iova_pfn(iovad, iova),
+		queue_iova(domain->iova_cookie, iova_pfn(iovad, iova),
 				size >> iova_shift(iovad),
 				&gather->freelist);
 	else
@@ -853,7 +838,7 @@ static void __iommu_dma_unmap(struct device *dev, dma_addr_t dma_addr,
 
 	if (!iotlb_gather.queued)
 		iommu_iotlb_sync(domain, &iotlb_gather);
-	iommu_dma_free_iova(cookie, dma_addr, size, &iotlb_gather);
+	iommu_dma_free_iova(domain, dma_addr, size, &iotlb_gather);
 }
 
 static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
@@ -881,7 +866,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 		return DMA_MAPPING_ERROR;
 
 	if (iommu_map(domain, iova, phys - iova_off, size, prot, GFP_ATOMIC)) {
-		iommu_dma_free_iova(cookie, iova, size, NULL);
+		iommu_dma_free_iova(domain, iova, size, NULL);
 		return DMA_MAPPING_ERROR;
 	}
 	return iova + iova_off;
@@ -1018,7 +1003,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 out_free_sg:
 	sg_free_table(sgt);
 out_free_iova:
-	iommu_dma_free_iova(cookie, iova, size, NULL);
+	iommu_dma_free_iova(domain, iova, size, NULL);
 out_free_pages:
 	__iommu_dma_free_pages(pages, count);
 	return NULL;
@@ -1495,7 +1480,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 	return __finalise_sg(dev, sg, nents, iova);
 
 out_free_iova:
-	iommu_dma_free_iova(cookie, iova, iova_len, NULL);
+	iommu_dma_free_iova(domain, iova, iova_len, NULL);
 out_restore_sg:
 	__invalidate_sg(sg, nents);
 out:
@@ -1773,17 +1758,47 @@ void iommu_setup_dma_ops(struct device *dev)
 	dev->dma_iommu = false;
 }
 
+static bool has_msi_cookie(const struct iommu_domain *domain)
+{
+	return domain && (domain->cookie_type == IOMMU_COOKIE_DMA_IOVA ||
+			  domain->cookie_type == IOMMU_COOKIE_DMA_MSI);
+}
+
+static size_t cookie_msi_granule(const struct iommu_domain *domain)
+{
+	switch (domain->cookie_type) {
+	case IOMMU_COOKIE_DMA_IOVA:
+		return domain->iova_cookie->iovad.granule;
+	case IOMMU_COOKIE_DMA_MSI:
+		return PAGE_SIZE;
+	default:
+		unreachable();
+	};
+}
+
+static struct list_head *cookie_msi_pages(const struct iommu_domain *domain)
+{
+	switch (domain->cookie_type) {
+	case IOMMU_COOKIE_DMA_IOVA:
+		return &domain->iova_cookie->msi_page_list;
+	case IOMMU_COOKIE_DMA_MSI:
+		return &domain->msi_cookie->msi_page_list;
+	default:
+		unreachable();
+	};
+}
+
 static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 		phys_addr_t msi_addr, struct iommu_domain *domain)
 {
-	struct iommu_dma_cookie *cookie = domain->iova_cookie;
+	struct list_head *msi_page_list = cookie_msi_pages(domain);
 	struct iommu_dma_msi_page *msi_page;
 	dma_addr_t iova;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
-	size_t size = cookie_msi_granule(cookie);
+	size_t size = cookie_msi_granule(domain);
 
 	msi_addr &= ~(phys_addr_t)(size - 1);
-	list_for_each_entry(msi_page, &cookie->msi_page_list, list)
+	list_for_each_entry(msi_page, msi_page_list, list)
 		if (msi_page->phys == msi_addr)
 			return msi_page;
 
@@ -1801,11 +1816,11 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	INIT_LIST_HEAD(&msi_page->list);
 	msi_page->phys = msi_addr;
 	msi_page->iova = iova;
-	list_add(&msi_page->list, &cookie->msi_page_list);
+	list_add(&msi_page->list, msi_page_list);
 	return msi_page;
 
 out_free_iova:
-	iommu_dma_free_iova(cookie, iova, size, NULL);
+	iommu_dma_free_iova(domain, iova, size, NULL);
 out_free_page:
 	kfree(msi_page);
 	return NULL;
@@ -1817,7 +1832,7 @@ static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
 	struct device *dev = msi_desc_to_dev(desc);
 	const struct iommu_dma_msi_page *msi_page;
 
-	if (!domain->iova_cookie) {
+	if (!has_msi_cookie(domain)) {
 		msi_desc_set_iommu_msi_iova(desc, 0, 0);
 		return 0;
 	}
@@ -1827,9 +1842,8 @@ static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
 	if (!msi_page)
 		return -ENOMEM;
 
-	msi_desc_set_iommu_msi_iova(
-		desc, msi_page->iova,
-		ilog2(cookie_msi_granule(domain->iova_cookie)));
+	msi_desc_set_iommu_msi_iova(desc, msi_page->iova,
+				    ilog2(cookie_msi_granule(domain)));
 	return 0;
 }
 
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 503c5d23c1ea..ab18bc494eef 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -310,6 +310,7 @@ static struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
 	}
 
 	domain->type = IOMMU_DOMAIN_SVA;
+	domain->cookie_type = IOMMU_COOKIE_SVA;
 	mmgrab(mm);
 	domain->mm = mm;
 	domain->owner = ops;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0ee17893810f..c92e47f333cb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1953,8 +1953,10 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 					iommu_fault_handler_t handler,
 					void *token)
 {
-	BUG_ON(!domain);
+	if (WARN_ON(!domain || domain->cookie_type != IOMMU_COOKIE_NONE))
+		return;
 
+	domain->cookie_type = IOMMU_COOKIE_FAULT_HANDLER;
 	domain->handler = handler;
 	domain->handler_token = token;
 }
@@ -2024,9 +2026,19 @@ EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc_flags);
 
 void iommu_domain_free(struct iommu_domain *domain)
 {
-	if (domain->type == IOMMU_DOMAIN_SVA)
+	switch (domain->cookie_type) {
+	case IOMMU_COOKIE_DMA_IOVA:
+		iommu_put_dma_cookie(domain);
+		break;
+	case IOMMU_COOKIE_DMA_MSI:
+		iommu_put_msi_cookie(domain);
+		break;
+	case IOMMU_COOKIE_SVA:
 		mmdrop(domain->mm);
-	iommu_put_dma_cookie(domain);
+		break;
+	default:
+		break;
+	}
 	if (domain->ops->free)
 		domain->ops->free(domain);
 }
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 1d4cfe3677dc..cefe71a4e9e8 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -160,6 +160,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 		}
 	}
 	hwpt->domain->iommufd_hwpt = hwpt;
+	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
 	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
 
 	/*
@@ -257,6 +258,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	}
 	hwpt->domain->owner = ops;
 	hwpt->domain->iommufd_hwpt = hwpt;
+	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
 	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
 
 	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
@@ -315,6 +317,7 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	}
 	hwpt->domain->iommufd_hwpt = hwpt;
 	hwpt->domain->owner = viommu->iommu_dev->ops;
+	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
 	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
 
 	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
-- 
2.43.0

Re: [PATCH v4 1/3] iommu: Sort out domain user data

[Baolu Lu]

On 3/7/25 05:00, Nicolin Chen wrote:
> From: Robin Murphy<robin.murphy@arm.com>
> 
> When DMA/MSI cookies were made first-class citizens back in commit
> 46983fcd67ac ("iommu: Pull IOVA cookie management into the core"), there
> was no real need to further expose the two different cookie types.
> However, now that IOMMUFD wants to add a third type of MSI-mapping
> cookie, we do have a nicely compelling reason to properly dismabiguate
> things at the domain level beyond just vaguely guessing from the domain
> type.
> 
> Meanwhile, we also effectively have another "cookie" in the form of the
> anonymous union for other user data, which isn't much better in terms of
> being vague and unenforced. The fact is that all these cookie types are
> mutually exclusive, in the sense that combining them makes zero sense
> and/or would be catastrophic (iommu_set_fault_handler() on an SVA
> domain, anyone?) - the only combination which*might* be reasonable is
> perhaps a fault handler and an MSI cookie, but nobody's doing that at
> the moment, so let's rule it out as well for the sake of being clear and
> robust. To that end, we pull DMA and MSI cookies apart a little more,
> mostly to clear up the ambiguity at domain teardown, then for clarity
> (and to save a little space), move them into the union, whose ownership
> we can then properly describe and enforce entirely unambiguously.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> [nicolinc: rebase on latest tree; use prefix IOMMU_COOKIE_; merge unions
>             in iommu_domain; add IOMMU_COOKIE_IOMMUFD for iommufd_hwpt]
> Signed-off-by: Nicolin Chen<nicolinc@nvidia.com>
> ---
>   drivers/iommu/dma-iommu.h            |   5 +
>   include/linux/iommu.h                |  20 ++-
>   drivers/iommu/dma-iommu.c            | 194 ++++++++++++++-------------
>   drivers/iommu/iommu-sva.c            |   1 +
>   drivers/iommu/iommu.c                |  18 ++-
>   drivers/iommu/iommufd/hw_pagetable.c |   3 +
>   6 files changed, 143 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
> index c12d63457c76..9cca11806e5d 100644
> --- a/drivers/iommu/dma-iommu.h
> +++ b/drivers/iommu/dma-iommu.h
> @@ -13,6 +13,7 @@ void iommu_setup_dma_ops(struct device *dev);
>   
>   int iommu_get_dma_cookie(struct iommu_domain *domain);
>   void iommu_put_dma_cookie(struct iommu_domain *domain);
> +void iommu_put_msi_cookie(struct iommu_domain *domain);
>   
>   int iommu_dma_init_fq(struct iommu_domain *domain);
>   
> @@ -40,6 +41,10 @@ static inline void iommu_put_dma_cookie(struct iommu_domain *domain)
>   {
>   }
>   
> +static inline void iommu_put_msi_cookie(struct iommu_domain *domain)
> +{
> +}
> +
>   static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
>   {
>   }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e93d2e918599..06cc14e9993d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -41,6 +41,7 @@ struct iommu_dirty_ops;
>   struct notifier_block;
>   struct iommu_sva;
>   struct iommu_dma_cookie;
> +struct iommu_dma_msi_cookie;
>   struct iommu_fault_param;
>   struct iommufd_ctx;
>   struct iommufd_viommu;
> @@ -165,6 +166,15 @@ struct iommu_domain_geometry {
>   	bool force_aperture;       /* DMA only allowed in mappable range? */
>   };
>   
> +enum iommu_domain_cookie_type {
> +	IOMMU_COOKIE_NONE,
> +	IOMMU_COOKIE_DMA_IOVA,
> +	IOMMU_COOKIE_DMA_MSI,
> +	IOMMU_COOKIE_FAULT_HANDLER,
> +	IOMMU_COOKIE_SVA,
> +	IOMMU_COOKIE_IOMMUFD,
> +};
> +
>   /* Domain feature flags */
>   #define __IOMMU_DOMAIN_PAGING	(1U << 0)  /* Support for iommu_map/unmap */
>   #define __IOMMU_DOMAIN_DMA_API	(1U << 1)  /* Domain for use in DMA-API
> @@ -211,12 +221,12 @@ struct iommu_domain_geometry {
>   
>   struct iommu_domain {
>   	unsigned type;
> +	enum iommu_domain_cookie_type cookie_type;
>   	const struct iommu_domain_ops *ops;
>   	const struct iommu_dirty_ops *dirty_ops;
>   	const struct iommu_ops *owner; /* Whose domain_alloc we came from */
>   	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
>   	struct iommu_domain_geometry geometry;
> -	struct iommu_dma_cookie *iova_cookie;
>   	int (*iopf_handler)(struct iopf_group *group);
>   
>   #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
> @@ -224,10 +234,10 @@ struct iommu_domain {
>   		      phys_addr_t msi_addr);
>   #endif
>   
> -	union { /* Pointer usable by owner of the domain */
> -		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
> -	};
> -	union { /* Fault handler */
> +	union { /* cookie */
> +		struct iommu_dma_cookie *iova_cookie;
> +		struct iommu_dma_msi_cookie *msi_cookie;
> +		struct iommufd_hw_pagetable *iommufd_hwpt;
>   		struct {
>   			iommu_fault_handler_t handler;
>   			void *handler_token;

My feeling is that IOMMU_COOKIE_FAULT_HANDLER isn't exclusive to
IOMMU_COOKIE_DMA_IOVA; both might be used for kernel DMA with a paging
domain.

I am afraid that iommu_set_fault_handler() doesn't work anymore as the
domain's cookie type has already been set to IOMMU_COOKIE_DMA_IOVA.

void iommu_set_fault_handler(struct iommu_domain *domain,
                                         iommu_fault_handler_t handler,
                                         void *token)
{
         if (WARN_ON(!domain || domain->cookie_type != IOMMU_COOKIE_NONE))
                 return;

         domain->cookie_type = IOMMU_COOKIE_FAULT_HANDLER;
         domain->handler = handler;
         domain->handler_token = token;
}
EXPORT_SYMBOL_GPL(iommu_set_fault_handler);

Anybody has a chance to test whether above WARN_ON will be triggered?

Thanks,
baolu

Re: [PATCH v4 1/3] iommu: Sort out domain user data

[Nicolin Chen]

Hi Baolu,

On Fri, Mar 07, 2025 at 10:28:20AM +0800, Baolu Lu wrote:
> On 3/7/25 05:00, Nicolin Chen wrote:
> > From: Robin Murphy<robin.murphy@arm.com>

Robin had remarks here, wrt iommu_set_fault_handler():

> > The fact is that all these cookie types are
> > mutually exclusive, in the sense that combining them makes zero sense
> > and/or would be catastrophic (iommu_set_fault_handler() on an SVA
> > domain, anyone?) - the only combination which*might* be reasonable is
> > perhaps a fault handler and an MSI cookie, but nobody's doing that at
> > the moment, so let's rule it out as well for the sake of being clear and
> > robust.
[...]
> > @@ -224,10 +234,10 @@ struct iommu_domain {
> >   		      phys_addr_t msi_addr);
> >   #endif
> > -	union { /* Pointer usable by owner of the domain */
> > -		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
> > -	};
> > -	union { /* Fault handler */
> > +	union { /* cookie */
> > +		struct iommu_dma_cookie *iova_cookie;
> > +		struct iommu_dma_msi_cookie *msi_cookie;
> > +		struct iommufd_hw_pagetable *iommufd_hwpt;
> >   		struct {
> >   			iommu_fault_handler_t handler;
> >   			void *handler_token;
> 
> My feeling is that IOMMU_COOKIE_FAULT_HANDLER isn't exclusive to
> IOMMU_COOKIE_DMA_IOVA; both might be used for kernel DMA with a paging
> domain.
> 
> I am afraid that iommu_set_fault_handler() doesn't work anymore as the
> domain's cookie type has already been set to IOMMU_COOKIE_DMA_IOVA.

All three existing iommu_set_fault_handler() callers in the tree
are UNMANAGED domain users:
   5    451  drivers/gpu/drm/msm/msm_iommu.c <<msm_iommu_gpu_new>>
             iommu_set_fault_handler(iommu->domain, msm_fault_handler, iommu);
   6    453  drivers/infiniband/hw/usnic/usnic_uiom.c <<usnic_uiom_alloc_pd>>
             iommu_set_fault_handler(pd->domain, usnic_uiom_dma_fault, NULL);
   8    118  drivers/remoteproc/remoteproc_core.c <<rproc_enable_iommu>>
             iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);

On the other hand, IOMMU_COOKIE_DMA_IOVA is a private cookie for
dma-iommu only.

So, I think we are probably fine?

Thanks
Nicolin

Re: [PATCH v4 1/3] iommu: Sort out domain user data

[Baolu Lu]

On 2025/3/7 13:57, Nicolin Chen wrote:
> On Fri, Mar 07, 2025 at 10:28:20AM +0800, Baolu Lu wrote:
>> On 3/7/25 05:00, Nicolin Chen wrote:
>>> From: Robin Murphy<robin.murphy@arm.com>
> Robin had remarks here, wrt iommu_set_fault_handler():
> 
>>> The fact is that all these cookie types are
>>> mutually exclusive, in the sense that combining them makes zero sense
>>> and/or would be catastrophic (iommu_set_fault_handler() on an SVA
>>> domain, anyone?) - the only combination which*might* be reasonable is
>>> perhaps a fault handler and an MSI cookie, but nobody's doing that at
>>> the moment, so let's rule it out as well for the sake of being clear and
>>> robust.
> [...]
>>> @@ -224,10 +234,10 @@ struct iommu_domain {
>>>    		      phys_addr_t msi_addr);
>>>    #endif
>>> -	union { /* Pointer usable by owner of the domain */
>>> -		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
>>> -	};
>>> -	union { /* Fault handler */
>>> +	union { /* cookie */
>>> +		struct iommu_dma_cookie *iova_cookie;
>>> +		struct iommu_dma_msi_cookie *msi_cookie;
>>> +		struct iommufd_hw_pagetable *iommufd_hwpt;
>>>    		struct {
>>>    			iommu_fault_handler_t handler;
>>>    			void *handler_token;exs
>> My feeling is that IOMMU_COOKIE_FAULT_HANDLER isn't exclusive to
>> IOMMU_COOKIE_DMA_IOVA; both might be used for kernel DMA with a paging
>> domain.
>>
>> I am afraid that iommu_set_fault_handler() doesn't work anymore as the
>> domain's cookie type has already been set to IOMMU_COOKIE_DMA_IOVA.
> All three existing iommu_set_fault_handler() callers in the tree
> are UNMANAGED domain users:
>     5    451  drivers/gpu/drm/msm/msm_iommu.c <<msm_iommu_gpu_new>>
>               iommu_set_fault_handler(iommu->domain, msm_fault_handler, iommu);
>     6    453  drivers/infiniband/hw/usnic/usnic_uiom.c <<usnic_uiom_alloc_pd>>
>               iommu_set_fault_handler(pd->domain, usnic_uiom_dma_fault, NULL);
>     8    118  drivers/remoteproc/remoteproc_core.c <<rproc_enable_iommu>>
>               iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> 
> On the other hand, IOMMU_COOKIE_DMA_IOVA is a private cookie for
> dma-iommu only.
> 
> So, I think we are probably fine?
If all existing use cases are for UNMANAGED domains, that's fine. And
when iommu_set_fault_handler() is miss-used, we already have a WARN_ON()
there.

Thanks,
baolu

Re: [PATCH v4 1/3] iommu: Sort out domain user data

[Robin Murphy]

On 2025-03-07 7:03 am, Baolu Lu wrote:
> On 2025/3/7 13:57, Nicolin Chen wrote:
>> On Fri, Mar 07, 2025 at 10:28:20AM +0800, Baolu Lu wrote:
>>> On 3/7/25 05:00, Nicolin Chen wrote:
>>>> From: Robin Murphy<robin.murphy@arm.com>
>> Robin had remarks here, wrt iommu_set_fault_handler():
>>
>>>> The fact is that all these cookie types are
>>>> mutually exclusive, in the sense that combining them makes zero sense
>>>> and/or would be catastrophic (iommu_set_fault_handler() on an SVA
>>>> domain, anyone?) - the only combination which*might* be reasonable is
>>>> perhaps a fault handler and an MSI cookie, but nobody's doing that at
>>>> the moment, so let's rule it out as well for the sake of being clear 
>>>> and
>>>> robust.
>> [...]
>>>> @@ -224,10 +234,10 @@ struct iommu_domain {
>>>>                  phys_addr_t msi_addr);
>>>>    #endif
>>>> -    union { /* Pointer usable by owner of the domain */
>>>> -        struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
>>>> -    };
>>>> -    union { /* Fault handler */
>>>> +    union { /* cookie */
>>>> +        struct iommu_dma_cookie *iova_cookie;
>>>> +        struct iommu_dma_msi_cookie *msi_cookie;
>>>> +        struct iommufd_hw_pagetable *iommufd_hwpt;
>>>>            struct {
>>>>                iommu_fault_handler_t handler;
>>>>                void *handler_token;exs
>>> My feeling is that IOMMU_COOKIE_FAULT_HANDLER isn't exclusive to
>>> IOMMU_COOKIE_DMA_IOVA; both might be used for kernel DMA with a paging
>>> domain.
>>>
>>> I am afraid that iommu_set_fault_handler() doesn't work anymore as the
>>> domain's cookie type has already been set to IOMMU_COOKIE_DMA_IOVA.
>> All three existing iommu_set_fault_handler() callers in the tree
>> are UNMANAGED domain users:
>>     5    451  drivers/gpu/drm/msm/msm_iommu.c <<msm_iommu_gpu_new>>
>>               iommu_set_fault_handler(iommu->domain, 
>> msm_fault_handler, iommu);
>>     6    453  drivers/infiniband/hw/usnic/usnic_uiom.c 
>> <<usnic_uiom_alloc_pd>>
>>               iommu_set_fault_handler(pd->domain, 
>> usnic_uiom_dma_fault, NULL);
>>     8    118  drivers/remoteproc/remoteproc_core.c <<rproc_enable_iommu>>
>>               iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
>>
>> On the other hand, IOMMU_COOKIE_DMA_IOVA is a private cookie for
>> dma-iommu only.
>>
>> So, I think we are probably fine?
> If all existing use cases are for UNMANAGED domains, that's fine. And
> when iommu_set_fault_handler() is miss-used, we already have a WARN_ON()
> there.

Right, it would be illogical for a driver to set a fault handler on a 
DMA domain since it doesn't control the IOVA space to be able to do any 
fault-handling, and iommu-dma itself isn't ever going to use a fault 
handler because it expects the DMA API to be used correctly and thus no 
faults to occur.

TBH at this point I view the fault_handler stuff as a legacy interface 
which we don't really want to encourage use of anyway - it's already 
proven not to be great for any true fault handling since many drivers 
can only call report_iommu_fault() in IRQ context. If some new case does 
come up in future where this mutual exclusion gets in the way, I would 
say that's the point where we then look at reworking the whole thing 
into a dedicated "fault notifier" mechanism instead, which could then 
logically be orthogonal to the IOVA-space-owner cookie.

Thanks,
Robin.

Re: [PATCH v4 1/3] iommu: Sort out domain user data

[Jason Gunthorpe]

On Fri, Mar 07, 2025 at 11:49:36AM +0000, Robin Murphy wrote:
 
> TBH at this point I view the fault_handler stuff as a legacy interface which
> we don't really want to encourage use of anyway - it's already proven not to
> be great for any true fault handling since many drivers can only call
> report_iommu_fault() in IRQ context. If some new case does come up in future
> where this mutual exclusion gets in the way, I would say that's the point
> where we then look at reworking the whole thing into a dedicated "fault
> notifier" mechanism instead, which could then logically be orthogonal to the
> IOVA-space-owner cookie.

I was under the impression we would go forward with the PRI focused
interface we already have:

	int (*iopf_handler)(struct iopf_group *group);

And this olld iommu_fault_handler_t is just because nobody wanted to
go in any update the old drivers and DRM to use the new scheme?

Is there something fundamental that would prevent iommu drivers from
using the new reporting path?

Jason

Re: [PATCH v4 1/3] iommu: Sort out domain user data

[Jason Gunthorpe]

On Thu, Mar 06, 2025 at 01:00:47PM -0800, Nicolin Chen wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> 
> When DMA/MSI cookies were made first-class citizens back in commit
> 46983fcd67ac ("iommu: Pull IOVA cookie management into the core"), there
> was no real need to further expose the two different cookie types.
> However, now that IOMMUFD wants to add a third type of MSI-mapping
> cookie, we do have a nicely compelling reason to properly dismabiguate
> things at the domain level beyond just vaguely guessing from the domain
> type.
> 
> Meanwhile, we also effectively have another "cookie" in the form of the
> anonymous union for other user data, which isn't much better in terms of
> being vague and unenforced. The fact is that all these cookie types are
> mutually exclusive, in the sense that combining them makes zero sense
> and/or would be catastrophic (iommu_set_fault_handler() on an SVA
> domain, anyone?) - the only combination which *might* be reasonable is
> perhaps a fault handler and an MSI cookie, but nobody's doing that at
> the moment, so let's rule it out as well for the sake of being clear and
> robust. To that end, we pull DMA and MSI cookies apart a little more,
> mostly to clear up the ambiguity at domain teardown, then for clarity
> (and to save a little space), move them into the union, whose ownership
> we can then properly describe and enforce entirely unambiguously.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> [nicolinc: rebase on latest tree; use prefix IOMMU_COOKIE_; merge unions
>            in iommu_domain; add IOMMU_COOKIE_IOMMUFD for iommufd_hwpt]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/dma-iommu.h            |   5 +
>  include/linux/iommu.h                |  20 ++-
>  drivers/iommu/dma-iommu.c            | 194 ++++++++++++++-------------
>  drivers/iommu/iommu-sva.c            |   1 +
>  drivers/iommu/iommu.c                |  18 ++-
>  drivers/iommu/iommufd/hw_pagetable.c |   3 +
>  6 files changed, 143 insertions(+), 98 deletions(-)

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

Jason

[PATCH v4 2/3] iommufd: Move iommufd_sw_msi and related functions to driver.c

[Nicolin Chen]

To provide the iommufd_sw_msi() to the iommu core that is under a different
Kconfig, move it and its related functions to driver.c. Then, stub it into
the iommu-priv header. The iommufd_sw_msi_install() continues to be used by
iommufd internal, so put it in the private header.

Since this affects the module size, here is before-n-after size comparison:
[Before]
   text	   data	    bss	    dec	    hex	filename
  18797	    848	     56	  19701	   4cf5	drivers/iommu/iommufd/device.o
    722	     44	      0	    766	    2fe	drivers/iommu/iommufd/driver.o
[After]
   text	   data	    bss	    dec	    hex	filename
  17671	    808	     56	  18535	   4867	drivers/iommu/iommufd/device.o
   1900	    100	      0	   2000	    7d0	drivers/iommu/iommufd/driver.o

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu-priv.h              |  13 +++
 drivers/iommu/iommufd/iommufd_private.h |   7 +-
 drivers/iommu/iommufd/device.c          | 131 ++----------------------
 drivers/iommu/iommufd/driver.c          | 125 ++++++++++++++++++++++
 4 files changed, 151 insertions(+), 125 deletions(-)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index b4508423e13b..c74fff25be78 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -5,6 +5,7 @@
 #define __LINUX_IOMMU_PRIV_H
 
 #include <linux/iommu.h>
+#include <linux/msi.h>
 
 static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 {
@@ -43,4 +44,16 @@ void iommu_detach_group_handle(struct iommu_domain *domain,
 int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle);
+
+#if IS_ENABLED(CONFIG_IOMMUFD_DRIVER_CORE) && IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
+int iommufd_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
+		   phys_addr_t msi_addr);
+#else /* !CONFIG_IOMMUFD_DRIVER_CORE || !CONFIG_IRQ_MSI_IOMMU */
+static inline int iommufd_sw_msi(struct iommu_domain *domain,
+				 struct msi_desc *desc, phys_addr_t msi_addr)
+{
+	return -EOPNOTSUPP;
+}
+#endif /* CONFIG_IOMMUFD_DRIVER_CORE && CONFIG_IRQ_MSI_IOMMU */
+
 #endif /* __LINUX_IOMMU_PRIV_H */
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 246297452a44..b6897a9991ee 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -32,8 +32,11 @@ struct iommufd_sw_msi_maps {
 	DECLARE_BITMAP(bitmap, 64);
 };
 
-int iommufd_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
-		   phys_addr_t msi_addr);
+#ifdef CONFIG_IRQ_MSI_IOMMU
+int iommufd_sw_msi_install(struct iommufd_ctx *ictx,
+			   struct iommufd_hwpt_paging *hwpt_paging,
+			   struct iommufd_sw_msi_map *msi_map);
+#endif
 
 struct iommufd_ctx {
 	struct file *file;
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index b2f0cb909e6d..814d059e333a 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -5,7 +5,6 @@
 #include <linux/iommufd.h>
 #include <linux/slab.h>
 #include <uapi/linux/iommufd.h>
-#include <linux/msi.h>
 
 #include "../iommu-priv.h"
 #include "io_pagetable.h"
@@ -294,129 +293,7 @@ u32 iommufd_device_to_id(struct iommufd_device *idev)
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, "IOMMUFD");
 
-/*
- * Get a iommufd_sw_msi_map for the msi physical address requested by the irq
- * layer. The mapping to IOVA is global to the iommufd file descriptor, every
- * domain that is attached to a device using the same MSI parameters will use
- * the same IOVA.
- */
-static __maybe_unused struct iommufd_sw_msi_map *
-iommufd_sw_msi_get_map(struct iommufd_ctx *ictx, phys_addr_t msi_addr,
-		       phys_addr_t sw_msi_start)
-{
-	struct iommufd_sw_msi_map *cur;
-	unsigned int max_pgoff = 0;
-
-	lockdep_assert_held(&ictx->sw_msi_lock);
-
-	list_for_each_entry(cur, &ictx->sw_msi_list, sw_msi_item) {
-		if (cur->sw_msi_start != sw_msi_start)
-			continue;
-		max_pgoff = max(max_pgoff, cur->pgoff + 1);
-		if (cur->msi_addr == msi_addr)
-			return cur;
-	}
-
-	if (ictx->sw_msi_id >=
-	    BITS_PER_BYTE * sizeof_field(struct iommufd_sw_msi_maps, bitmap))
-		return ERR_PTR(-EOVERFLOW);
-
-	cur = kzalloc(sizeof(*cur), GFP_KERNEL);
-	if (!cur)
-		return ERR_PTR(-ENOMEM);
-
-	cur->sw_msi_start = sw_msi_start;
-	cur->msi_addr = msi_addr;
-	cur->pgoff = max_pgoff;
-	cur->id = ictx->sw_msi_id++;
-	list_add_tail(&cur->sw_msi_item, &ictx->sw_msi_list);
-	return cur;
-}
-
-static int iommufd_sw_msi_install(struct iommufd_ctx *ictx,
-				  struct iommufd_hwpt_paging *hwpt_paging,
-				  struct iommufd_sw_msi_map *msi_map)
-{
-	unsigned long iova;
-
-	lockdep_assert_held(&ictx->sw_msi_lock);
-
-	iova = msi_map->sw_msi_start + msi_map->pgoff * PAGE_SIZE;
-	if (!test_bit(msi_map->id, hwpt_paging->present_sw_msi.bitmap)) {
-		int rc;
-
-		rc = iommu_map(hwpt_paging->common.domain, iova,
-			       msi_map->msi_addr, PAGE_SIZE,
-			       IOMMU_WRITE | IOMMU_READ | IOMMU_MMIO,
-			       GFP_KERNEL_ACCOUNT);
-		if (rc)
-			return rc;
-		__set_bit(msi_map->id, hwpt_paging->present_sw_msi.bitmap);
-	}
-	return 0;
-}
-
-/*
- * Called by the irq code if the platform translates the MSI address through the
- * IOMMU. msi_addr is the physical address of the MSI page. iommufd will
- * allocate a fd global iova for the physical page that is the same on all
- * domains and devices.
- */
 #ifdef CONFIG_IRQ_MSI_IOMMU
-int iommufd_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
-		   phys_addr_t msi_addr)
-{
-	struct device *dev = msi_desc_to_dev(desc);
-	struct iommufd_hwpt_paging *hwpt_paging;
-	struct iommu_attach_handle *raw_handle;
-	struct iommufd_attach_handle *handle;
-	struct iommufd_sw_msi_map *msi_map;
-	struct iommufd_ctx *ictx;
-	unsigned long iova;
-	int rc;
-
-	/*
-	 * It is safe to call iommu_attach_handle_get() here because the iommu
-	 * core code invokes this under the group mutex which also prevents any
-	 * change of the attach handle for the duration of this function.
-	 */
-	iommu_group_mutex_assert(dev);
-
-	raw_handle =
-		iommu_attach_handle_get(dev->iommu_group, IOMMU_NO_PASID, 0);
-	if (IS_ERR(raw_handle))
-		return 0;
-	hwpt_paging = find_hwpt_paging(domain->iommufd_hwpt);
-
-	handle = to_iommufd_handle(raw_handle);
-	/* No IOMMU_RESV_SW_MSI means no change to the msi_msg */
-	if (handle->idev->igroup->sw_msi_start == PHYS_ADDR_MAX)
-		return 0;
-
-	ictx = handle->idev->ictx;
-	guard(mutex)(&ictx->sw_msi_lock);
-	/*
-	 * The input msi_addr is the exact byte offset of the MSI doorbell, we
-	 * assume the caller has checked that it is contained with a MMIO region
-	 * that is secure to map at PAGE_SIZE.
-	 */
-	msi_map = iommufd_sw_msi_get_map(handle->idev->ictx,
-					 msi_addr & PAGE_MASK,
-					 handle->idev->igroup->sw_msi_start);
-	if (IS_ERR(msi_map))
-		return PTR_ERR(msi_map);
-
-	rc = iommufd_sw_msi_install(ictx, hwpt_paging, msi_map);
-	if (rc)
-		return rc;
-	__set_bit(msi_map->id, handle->idev->igroup->required_sw_msi.bitmap);
-
-	iova = msi_map->sw_msi_start + msi_map->pgoff * PAGE_SIZE;
-	msi_desc_set_iommu_msi_iova(desc, iova, PAGE_SHIFT);
-	return 0;
-}
-#endif
-
 static int iommufd_group_setup_msi(struct iommufd_group *igroup,
 				   struct iommufd_hwpt_paging *hwpt_paging)
 {
@@ -443,6 +320,14 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
 	}
 	return 0;
 }
+#else
+static inline int
+iommufd_group_setup_msi(struct iommufd_group *igroup,
+			struct iommufd_hwpt_paging *hwpt_paging)
+{
+	return 0;
+}
+#endif
 
 static int
 iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 2d98b04ff1cb..5132fea9e52a 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -49,5 +49,130 @@ struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_viommu_find_dev, "IOMMUFD");
 
+#ifdef CONFIG_IRQ_MSI_IOMMU
+/*
+ * Get a iommufd_sw_msi_map for the msi physical address requested by the irq
+ * layer. The mapping to IOVA is global to the iommufd file descriptor, every
+ * domain that is attached to a device using the same MSI parameters will use
+ * the same IOVA.
+ */
+static struct iommufd_sw_msi_map *
+iommufd_sw_msi_get_map(struct iommufd_ctx *ictx, phys_addr_t msi_addr,
+		       phys_addr_t sw_msi_start)
+{
+	struct iommufd_sw_msi_map *cur;
+	unsigned int max_pgoff = 0;
+
+	lockdep_assert_held(&ictx->sw_msi_lock);
+
+	list_for_each_entry(cur, &ictx->sw_msi_list, sw_msi_item) {
+		if (cur->sw_msi_start != sw_msi_start)
+			continue;
+		max_pgoff = max(max_pgoff, cur->pgoff + 1);
+		if (cur->msi_addr == msi_addr)
+			return cur;
+	}
+
+	if (ictx->sw_msi_id >=
+	    BITS_PER_BYTE * sizeof_field(struct iommufd_sw_msi_maps, bitmap))
+		return ERR_PTR(-EOVERFLOW);
+
+	cur = kzalloc(sizeof(*cur), GFP_KERNEL);
+	if (!cur)
+		return ERR_PTR(-ENOMEM);
+
+	cur->sw_msi_start = sw_msi_start;
+	cur->msi_addr = msi_addr;
+	cur->pgoff = max_pgoff;
+	cur->id = ictx->sw_msi_id++;
+	list_add_tail(&cur->sw_msi_item, &ictx->sw_msi_list);
+	return cur;
+}
+
+int iommufd_sw_msi_install(struct iommufd_ctx *ictx,
+			   struct iommufd_hwpt_paging *hwpt_paging,
+			   struct iommufd_sw_msi_map *msi_map)
+{
+	unsigned long iova;
+
+	lockdep_assert_held(&ictx->sw_msi_lock);
+
+	iova = msi_map->sw_msi_start + msi_map->pgoff * PAGE_SIZE;
+	if (!test_bit(msi_map->id, hwpt_paging->present_sw_msi.bitmap)) {
+		int rc;
+
+		rc = iommu_map(hwpt_paging->common.domain, iova,
+			       msi_map->msi_addr, PAGE_SIZE,
+			       IOMMU_WRITE | IOMMU_READ | IOMMU_MMIO,
+			       GFP_KERNEL_ACCOUNT);
+		if (rc)
+			return rc;
+		__set_bit(msi_map->id, hwpt_paging->present_sw_msi.bitmap);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_sw_msi_install, "IOMMUFD_INTERNAL");
+
+/*
+ * Called by the irq code if the platform translates the MSI address through the
+ * IOMMU. msi_addr is the physical address of the MSI page. iommufd will
+ * allocate a fd global iova for the physical page that is the same on all
+ * domains and devices.
+ */
+int iommufd_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
+		   phys_addr_t msi_addr)
+{
+	struct device *dev = msi_desc_to_dev(desc);
+	struct iommufd_hwpt_paging *hwpt_paging;
+	struct iommu_attach_handle *raw_handle;
+	struct iommufd_attach_handle *handle;
+	struct iommufd_sw_msi_map *msi_map;
+	struct iommufd_ctx *ictx;
+	unsigned long iova;
+	int rc;
+
+	/*
+	 * It is safe to call iommu_attach_handle_get() here because the iommu
+	 * core code invokes this under the group mutex which also prevents any
+	 * change of the attach handle for the duration of this function.
+	 */
+	iommu_group_mutex_assert(dev);
+
+	raw_handle =
+		iommu_attach_handle_get(dev->iommu_group, IOMMU_NO_PASID, 0);
+	if (IS_ERR(raw_handle))
+		return 0;
+	hwpt_paging = find_hwpt_paging(domain->iommufd_hwpt);
+
+	handle = to_iommufd_handle(raw_handle);
+	/* No IOMMU_RESV_SW_MSI means no change to the msi_msg */
+	if (handle->idev->igroup->sw_msi_start == PHYS_ADDR_MAX)
+		return 0;
+
+	ictx = handle->idev->ictx;
+	guard(mutex)(&ictx->sw_msi_lock);
+	/*
+	 * The input msi_addr is the exact byte offset of the MSI doorbell, we
+	 * assume the caller has checked that it is contained with a MMIO region
+	 * that is secure to map at PAGE_SIZE.
+	 */
+	msi_map = iommufd_sw_msi_get_map(handle->idev->ictx,
+					 msi_addr & PAGE_MASK,
+					 handle->idev->igroup->sw_msi_start);
+	if (IS_ERR(msi_map))
+		return PTR_ERR(msi_map);
+
+	rc = iommufd_sw_msi_install(ictx, hwpt_paging, msi_map);
+	if (rc)
+		return rc;
+	__set_bit(msi_map->id, handle->idev->igroup->required_sw_msi.bitmap);
+
+	iova = msi_map->sw_msi_start + msi_map->pgoff * PAGE_SIZE;
+	msi_desc_set_iommu_msi_iova(desc, iova, PAGE_SHIFT);
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_sw_msi, "IOMMUFD");
+#endif
+
 MODULE_DESCRIPTION("iommufd code shared with builtin modules");
 MODULE_LICENSE("GPL");
-- 
2.43.0

RE: [PATCH v4 2/3] iommufd: Move iommufd_sw_msi and related functions to driver.c

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, March 7, 2025 5:01 AM
> 
> To provide the iommufd_sw_msi() to the iommu core that is under a
> different
> Kconfig, move it and its related functions to driver.c. Then, stub it into
> the iommu-priv header. The iommufd_sw_msi_install() continues to be used
> by
> iommufd internal, so put it in the private header.
> 
> Since this affects the module size, here is before-n-after size comparison:
> [Before]
>    text	   data	    bss	    dec	    hex	filename
>   18797	    848	     56	  19701	   4cf5	drivers/iommu/iommufd/device.o
>     722	     44	      0	    766	    2fe	drivers/iommu/iommufd/driver.o
> [After]
>    text	   data	    bss	    dec	    hex	filename
>   17671	    808	     56	  18535	   4867	drivers/iommu/iommufd/device.o
>    1900	    100	      0	   2000	    7d0	drivers/iommu/iommufd/driver.o
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v4 2/3] iommufd: Move iommufd_sw_msi and related functions to driver.c

[Jason Gunthorpe]

On Thu, Mar 06, 2025 at 01:00:48PM -0800, Nicolin Chen wrote:
> To provide the iommufd_sw_msi() to the iommu core that is under a different
> Kconfig, move it and its related functions to driver.c. Then, stub it into
> the iommu-priv header. The iommufd_sw_msi_install() continues to be used by
> iommufd internal, so put it in the private header.
> 
> Since this affects the module size, here is before-n-after size comparison:
> [Before]
>    text	   data	    bss	    dec	    hex	filename
>   18797	    848	     56	  19701	   4cf5	drivers/iommu/iommufd/device.o
>     722	     44	      0	    766	    2fe	drivers/iommu/iommufd/driver.o
> [After]
>    text	   data	    bss	    dec	    hex	filename
>   17671	    808	     56	  18535	   4867	drivers/iommu/iommufd/device.o
>    1900	    100	      0	   2000	    7d0	drivers/iommu/iommufd/driver.o
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommu-priv.h              |  13 +++
>  drivers/iommu/iommufd/iommufd_private.h |   7 +-
>  drivers/iommu/iommufd/device.c          | 131 ++----------------------
>  drivers/iommu/iommufd/driver.c          | 125 ++++++++++++++++++++++
>  4 files changed, 151 insertions(+), 125 deletions(-)

+1K is more than I would have liked, but OK

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

Jason

[PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nicolin Chen]

There are only two sw_msi implementations in the entire system, thus it's
not very necessary to have an sw_msi pointer.

Instead, check domain->cookie_type to call the two sw_msi implementations
directly from the core code.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/dma-iommu.h            |  9 +++++++++
 include/linux/iommu.h                | 15 ---------------
 drivers/iommu/dma-iommu.c            | 14 ++------------
 drivers/iommu/iommu.c                | 17 +++++++++++++++--
 drivers/iommu/iommufd/hw_pagetable.c |  3 ---
 5 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
index 9cca11806e5d..eca201c1f963 100644
--- a/drivers/iommu/dma-iommu.h
+++ b/drivers/iommu/dma-iommu.h
@@ -19,6 +19,9 @@ int iommu_dma_init_fq(struct iommu_domain *domain);
 
 void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
 
+int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
+		     phys_addr_t msi_addr);
+
 extern bool iommu_dma_forcedac;
 
 #else /* CONFIG_IOMMU_DMA */
@@ -49,5 +52,11 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
 {
 }
 
+static inline int iommu_dma_sw_msi(struct iommu_domain *domain,
+				   struct msi_desc *desc, phys_addr_t msi_addr)
+{
+	return -ENODEV;
+}
+
 #endif	/* CONFIG_IOMMU_DMA */
 #endif	/* __DMA_IOMMU_H */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 06cc14e9993d..e01c855ae8a7 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -229,11 +229,6 @@ struct iommu_domain {
 	struct iommu_domain_geometry geometry;
 	int (*iopf_handler)(struct iopf_group *group);
 
-#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
-	int (*sw_msi)(struct iommu_domain *domain, struct msi_desc *desc,
-		      phys_addr_t msi_addr);
-#endif
-
 	union { /* cookie */
 		struct iommu_dma_cookie *iova_cookie;
 		struct iommu_dma_msi_cookie *msi_cookie;
@@ -254,16 +249,6 @@ struct iommu_domain {
 	};
 };
 
-static inline void iommu_domain_set_sw_msi(
-	struct iommu_domain *domain,
-	int (*sw_msi)(struct iommu_domain *domain, struct msi_desc *desc,
-		      phys_addr_t msi_addr))
-{
-#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
-	domain->sw_msi = sw_msi;
-#endif
-}
-
 static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
 {
 	return domain->type & __IOMMU_DOMAIN_DMA_API;
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 31a7b4b81656..2bd9f80a83fe 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -94,9 +94,6 @@ static int __init iommu_dma_forcedac_setup(char *str)
 }
 early_param("iommu.forcedac", iommu_dma_forcedac_setup);
 
-static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
-			    phys_addr_t msi_addr);
-
 /* Number of entries per flush queue */
 #define IOVA_DEFAULT_FQ_SIZE	256
 #define IOVA_SINGLE_FQ_SIZE	32768
@@ -377,7 +374,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
 
 	mutex_init(&cookie->mutex);
 	INIT_LIST_HEAD(&cookie->msi_page_list);
-	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
 	domain->cookie_type = IOMMU_COOKIE_DMA_IOVA;
 	domain->iova_cookie = cookie;
 	return 0;
@@ -411,7 +407,6 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 
 	cookie->msi_iova = base;
 	INIT_LIST_HEAD(&cookie->msi_page_list);
-	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
 	domain->cookie_type = IOMMU_COOKIE_DMA_MSI;
 	domain->msi_cookie = cookie;
 	return 0;
@@ -427,11 +422,6 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iommu_dma_msi_page *msi, *tmp;
 
-#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
-	if (domain->sw_msi != iommu_dma_sw_msi)
-		return;
-#endif
-
 	if (cookie->iovad.granule) {
 		iommu_dma_free_fq(cookie);
 		put_iova_domain(&cookie->iovad);
@@ -1826,8 +1816,8 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	return NULL;
 }
 
-static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
-			    phys_addr_t msi_addr)
+int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
+		     phys_addr_t msi_addr)
 {
 	struct device *dev = msi_desc_to_dev(desc);
 	const struct iommu_dma_msi_page *msi_page;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c92e47f333cb..0f4cc15ded1c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -18,6 +18,7 @@
 #include <linux/errno.h>
 #include <linux/host1x_context_bus.h>
 #include <linux/iommu.h>
+#include <linux/iommufd.h>
 #include <linux/idr.h>
 #include <linux/err.h>
 #include <linux/pci.h>
@@ -3650,8 +3651,20 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 		return 0;
 
 	mutex_lock(&group->mutex);
-	if (group->domain && group->domain->sw_msi)
-		ret = group->domain->sw_msi(group->domain, desc, msi_addr);
+	if (group->domain) {
+		switch (group->domain->cookie_type) {
+		case IOMMU_COOKIE_DMA_MSI:
+		case IOMMU_COOKIE_DMA_IOVA:
+			ret = iommu_dma_sw_msi(group->domain, desc, msi_addr);
+			break;
+		case IOMMU_COOKIE_IOMMUFD:
+			ret = iommufd_sw_msi(group->domain, desc, msi_addr);
+			break;
+		default:
+			ret = -EOPNOTSUPP;
+			break;
+		}
+	}
 	mutex_unlock(&group->mutex);
 	return ret;
 }
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index cefe71a4e9e8..f97c4e49d486 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -161,7 +161,6 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 	}
 	hwpt->domain->iommufd_hwpt = hwpt;
 	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
-	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
 
 	/*
 	 * Set the coherency mode before we do iopt_table_add_domain() as some
@@ -259,7 +258,6 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	hwpt->domain->owner = ops;
 	hwpt->domain->iommufd_hwpt = hwpt;
 	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
-	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
 
 	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
 		rc = -EINVAL;
@@ -318,7 +316,6 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	hwpt->domain->iommufd_hwpt = hwpt;
 	hwpt->domain->owner = viommu->iommu_dev->ops;
 	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
-	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
 
 	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
 		rc = -EINVAL;
-- 
2.43.0

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Jason Gunthorpe]

On Thu, Mar 06, 2025 at 01:00:49PM -0800, Nicolin Chen wrote:
> There are only two sw_msi implementations in the entire system, thus it's
> not very necessary to have an sw_msi pointer.
> 
> Instead, check domain->cookie_type to call the two sw_msi implementations
> directly from the core code.
> 
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/dma-iommu.h            |  9 +++++++++
>  include/linux/iommu.h                | 15 ---------------
>  drivers/iommu/dma-iommu.c            | 14 ++------------
>  drivers/iommu/iommu.c                | 17 +++++++++++++++--
>  drivers/iommu/iommufd/hw_pagetable.c |  3 ---
>  5 files changed, 26 insertions(+), 32 deletions(-)

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

Jason

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nathan Chancellor]

Hi Nicolin,

On Thu, Mar 06, 2025 at 01:00:49PM -0800, Nicolin Chen wrote:
> There are only two sw_msi implementations in the entire system, thus it's
> not very necessary to have an sw_msi pointer.
> 
> Instead, check domain->cookie_type to call the two sw_msi implementations
> directly from the core code.
> 
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/dma-iommu.h            |  9 +++++++++
>  include/linux/iommu.h                | 15 ---------------
>  drivers/iommu/dma-iommu.c            | 14 ++------------
>  drivers/iommu/iommu.c                | 17 +++++++++++++++--
>  drivers/iommu/iommufd/hw_pagetable.c |  3 ---
>  5 files changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
> index 9cca11806e5d..eca201c1f963 100644
> --- a/drivers/iommu/dma-iommu.h
> +++ b/drivers/iommu/dma-iommu.h
> @@ -19,6 +19,9 @@ int iommu_dma_init_fq(struct iommu_domain *domain);
>  
>  void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
>  
> +int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
> +		     phys_addr_t msi_addr);
> +
>  extern bool iommu_dma_forcedac;
>  
>  #else /* CONFIG_IOMMU_DMA */
> @@ -49,5 +52,11 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
>  {
>  }
>  
> +static inline int iommu_dma_sw_msi(struct iommu_domain *domain,
> +				   struct msi_desc *desc, phys_addr_t msi_addr)
> +{
> +	return -ENODEV;
> +}
> +
>  #endif	/* CONFIG_IOMMU_DMA */
>  #endif	/* __DMA_IOMMU_H */
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 06cc14e9993d..e01c855ae8a7 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -229,11 +229,6 @@ struct iommu_domain {
>  	struct iommu_domain_geometry geometry;
>  	int (*iopf_handler)(struct iopf_group *group);
>  
> -#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
> -	int (*sw_msi)(struct iommu_domain *domain, struct msi_desc *desc,
> -		      phys_addr_t msi_addr);
> -#endif
> -
>  	union { /* cookie */
>  		struct iommu_dma_cookie *iova_cookie;
>  		struct iommu_dma_msi_cookie *msi_cookie;
> @@ -254,16 +249,6 @@ struct iommu_domain {
>  	};
>  };
>  
> -static inline void iommu_domain_set_sw_msi(
> -	struct iommu_domain *domain,
> -	int (*sw_msi)(struct iommu_domain *domain, struct msi_desc *desc,
> -		      phys_addr_t msi_addr))
> -{
> -#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
> -	domain->sw_msi = sw_msi;
> -#endif
> -}
> -
>  static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
>  {
>  	return domain->type & __IOMMU_DOMAIN_DMA_API;
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 31a7b4b81656..2bd9f80a83fe 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -94,9 +94,6 @@ static int __init iommu_dma_forcedac_setup(char *str)
>  }
>  early_param("iommu.forcedac", iommu_dma_forcedac_setup);
>  
> -static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
> -			    phys_addr_t msi_addr);
> -
>  /* Number of entries per flush queue */
>  #define IOVA_DEFAULT_FQ_SIZE	256
>  #define IOVA_SINGLE_FQ_SIZE	32768
> @@ -377,7 +374,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
>  
>  	mutex_init(&cookie->mutex);
>  	INIT_LIST_HEAD(&cookie->msi_page_list);
> -	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
>  	domain->cookie_type = IOMMU_COOKIE_DMA_IOVA;
>  	domain->iova_cookie = cookie;
>  	return 0;
> @@ -411,7 +407,6 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>  
>  	cookie->msi_iova = base;
>  	INIT_LIST_HEAD(&cookie->msi_page_list);
> -	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
>  	domain->cookie_type = IOMMU_COOKIE_DMA_MSI;
>  	domain->msi_cookie = cookie;
>  	return 0;
> @@ -427,11 +422,6 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
>  	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>  	struct iommu_dma_msi_page *msi, *tmp;
>  
> -#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
> -	if (domain->sw_msi != iommu_dma_sw_msi)
> -		return;
> -#endif
> -
>  	if (cookie->iovad.granule) {
>  		iommu_dma_free_fq(cookie);
>  		put_iova_domain(&cookie->iovad);
> @@ -1826,8 +1816,8 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
>  	return NULL;
>  }
>  
> -static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
> -			    phys_addr_t msi_addr)
> +int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,
> +		     phys_addr_t msi_addr)
>  {
>  	struct device *dev = msi_desc_to_dev(desc);
>  	const struct iommu_dma_msi_page *msi_page;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c92e47f333cb..0f4cc15ded1c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -18,6 +18,7 @@
>  #include <linux/errno.h>
>  #include <linux/host1x_context_bus.h>
>  #include <linux/iommu.h>
> +#include <linux/iommufd.h>
>  #include <linux/idr.h>
>  #include <linux/err.h>
>  #include <linux/pci.h>
> @@ -3650,8 +3651,20 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
>  		return 0;
>  
>  	mutex_lock(&group->mutex);
> -	if (group->domain && group->domain->sw_msi)
> -		ret = group->domain->sw_msi(group->domain, desc, msi_addr);
> +	if (group->domain) {
> +		switch (group->domain->cookie_type) {
> +		case IOMMU_COOKIE_DMA_MSI:
> +		case IOMMU_COOKIE_DMA_IOVA:
> +			ret = iommu_dma_sw_msi(group->domain, desc, msi_addr);
> +			break;
> +		case IOMMU_COOKIE_IOMMUFD:
> +			ret = iommufd_sw_msi(group->domain, desc, msi_addr);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +	}
>  	mutex_unlock(&group->mutex);
>  	return ret;
>  }
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index cefe71a4e9e8..f97c4e49d486 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -161,7 +161,6 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>  	}
>  	hwpt->domain->iommufd_hwpt = hwpt;
>  	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
> -	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
>  
>  	/*
>  	 * Set the coherency mode before we do iopt_table_add_domain() as some
> @@ -259,7 +258,6 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
>  	hwpt->domain->owner = ops;
>  	hwpt->domain->iommufd_hwpt = hwpt;
>  	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
> -	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
>  
>  	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
>  		rc = -EINVAL;
> @@ -318,7 +316,6 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
>  	hwpt->domain->iommufd_hwpt = hwpt;
>  	hwpt->domain->owner = viommu->iommu_dev->ops;
>  	hwpt->domain->cookie_type = IOMMU_COOKIE_IOMMUFD;
> -	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
>  
>  	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
>  		rc = -EINVAL;
> -- 
> 2.43.0
> 

I bisected a loss of networking on one of my machines to this change as
commit e009e088d88e ("iommu: Drop sw_msi from iommu_domain") in -next.

With the parent change, I see:

  [  +0.000000] Linux version 6.14.0-rc2-00032-g916a207692ce (nathan@ax162) (aarch64-linux-gcc (GCC) 14.2.0, GNU ld (GNU Binutils) 2.42) #1 SMP PREEMPT_DYNAMIC Mon Mar 24 09:13:34 MST 2025
  ...
  [  +0.002375] fsl_mc_bus NXP0008:00: Adding to iommu group 0
  [  +0.000532] fsl_mc_bus NXP0008:00: MC firmware version: 10.28.1
  [  +0.002566] fsl_mc_dprc dprc.1: DMA mask not set
  [  +0.019213] fsl_mc_dprc dprc.1: Adding to iommu group 1
  [  +0.050801] fsl_mc_allocator dpbp.0: Adding to iommu group 1
  [  +0.006767] fsl_mc_allocator dpmcp.35: Adding to iommu group 1
  ...

At this change, I see:

  [  +0.000000] Linux version 6.14.0-rc2-00033-ge009e088d88e (nathan@ax162) (aarch64-linux-gcc (GCC) 14.2.0, GNU ld (GNU Binutils) 2.42) #1 SMP PREEMPT_DYNAMIC Mon Mar 24 08:57:49 MST 2025
  ...
  [  +0.002355] fsl_mc_bus NXP0008:00: Adding to iommu group 0
  [  +0.000533] fsl_mc_bus NXP0008:00: MC firmware version: 10.28.1
  [  +0.002565] fsl_mc_dprc dprc.1: DMA mask not set
  [  +0.019255] fsl_mc_dprc dprc.1: Adding to iommu group 1
  [  +0.046820] fsl_mc_dprc dprc.1: Failed to allocate IRQs
  [  +0.005798] fsl_mc_dprc dprc.1: fsl_mc_driver_probe failed: -28
  [  +0.005931] fsl_mc_dprc dprc.1: probe with driver fsl_mc_dprc failed with error -28
  ...

I know this is not much to go on initially but I am more than happy to
provide any additional information and test patches as necessary.

Cheers,
Nathan

# bad: [9388ec571cb1adba59d1cded2300eeb11827679c] Add linux-next specific files for 20250321
# good: [b5329d5a35582abbef57562f9fb6cb26a643f252] Merge tag 'vfs-6.14-final.fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs
git bisect start '9388ec571cb1adba59d1cded2300eeb11827679c' 'b5329d5a35582abbef57562f9fb6cb26a643f252'
# good: [81a405abf1b06a6088197fe1d039ec5dbfd17989] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git
git bisect good 81a405abf1b06a6088197fe1d039ec5dbfd17989
# good: [1afe6ad5ffa395d0809210a3b609751109de3f44] Merge branch 'apparmor-next' of git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor
git bisect good 1afe6ad5ffa395d0809210a3b609751109de3f44
# good: [b97539c1de91aa73b589e145e3c804b9ba5178f2] Merge branch 'driver-core-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
git bisect good b97539c1de91aa73b589e145e3c804b9ba5178f2
# good: [db08813487b74820cb40f507fc6ea38994a44776] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git
git bisect good db08813487b74820cb40f507fc6ea38994a44776
# good: [8dc029f217de83114fe7a59803ea0ac398db414c] Merge branch 'hyperv-next' of git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
git bisect good 8dc029f217de83114fe7a59803ea0ac398db414c
# good: [33239be7f682bf0ef9293c0b8ad53e691275e3a2] Merge branch 'rust-next' of https://github.com/Rust-for-Linux/linux.git
git bisect good 33239be7f682bf0ef9293c0b8ad53e691275e3a2
# good: [f57f2954197b265b6d793ac89d774aae1473854d] Merge branch 'for-next/kspp' of git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
git bisect good f57f2954197b265b6d793ac89d774aae1473854d
# good: [acf9f8da5e19fc1cbf26f2ecb749369e13e7cd85] x86/crc: drop the avx10_256 functions and rename avx10_512 to avx512
git bisect good acf9f8da5e19fc1cbf26f2ecb749369e13e7cd85
# good: [da0c56520e880441d0503d0cf0d6853dcfb5f1a4] iommu/arm-smmu-v3: Set MEV bit in nested STE for DoS mitigations
git bisect good da0c56520e880441d0503d0cf0d6853dcfb5f1a4
# bad: [fe0cdd47410f7d5fc26e81c958cde3fe1d57d0a0] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git
git bisect bad fe0cdd47410f7d5fc26e81c958cde3fe1d57d0a0
# good: [447c98c1ca4a4b0d43be99f76c558c09956484f3] tools/power turbostat: Add idle governor statistics reporting
git bisect good 447c98c1ca4a4b0d43be99f76c558c09956484f3
# good: [916a207692ce0a6f82ced6b37ca895d5219efb17] iommufd: Move iommufd_sw_msi and related functions to driver.c
git bisect good 916a207692ce0a6f82ced6b37ca895d5219efb17
# bad: [71c4c1a2a057bd800e08b752bac660f5426bf6b5] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git
git bisect bad 71c4c1a2a057bd800e08b752bac660f5426bf6b5
# bad: [e009e088d88e8402539f9595b10c0014125a70c1] iommu: Drop sw_msi from iommu_domain
git bisect bad e009e088d88e8402539f9595b10c0014125a70c1
# first bad commit: [e009e088d88e8402539f9595b10c0014125a70c1] iommu: Drop sw_msi from iommu_domain

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Jason Gunthorpe]

On Mon, Mar 24, 2025 at 09:25:58AM -0700, Nathan Chancellor wrote:

> I bisected a loss of networking on one of my machines to this change as
> commit e009e088d88e ("iommu: Drop sw_msi from iommu_domain") in -next.

Okay wow, I will drop this series from the tree if I don't see a
resolution in a few days. We can try again next cycle, thank you for
testing and bisect!

> At this change, I see:
> 
>   [  +0.000000] Linux version 6.14.0-rc2-00033-ge009e088d88e (nathan@ax162) (aarch64-linux-gcc (GCC) 14.2.0, GNU ld (GNU Binutils) 2.42) #1 SMP PREEMPT_DYNAMIC Mon Mar 24 08:57:49 MST 2025
>   ...
>   [  +0.002355] fsl_mc_bus NXP0008:00: Adding to iommu group 0
>   [  +0.000533] fsl_mc_bus NXP0008:00: MC firmware version: 10.28.1
>   [  +0.002565] fsl_mc_dprc dprc.1: DMA mask not set
>   [  +0.019255] fsl_mc_dprc dprc.1: Adding to iommu group 1
>   [  +0.046820] fsl_mc_dprc dprc.1: Failed to allocate IRQs

I guess it is tripping up going through iommu_dma_prepare_msi()
somehow?

Maybe fsl bus is special and doesn't manage to set
IOMMU_COOKIE_DMA_IOVA for some reason?

I wonder if this is not right:

+               default:
+                       ret = -EOPNOTSUPP;
+                       break;

And it should be just break instead (return 0) which is what was
happening before?

Jason

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nicolin Chen]

On Mon, Mar 24, 2025 at 01:40:23PM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 24, 2025 at 09:25:58AM -0700, Nathan Chancellor wrote:
> 
> > I bisected a loss of networking on one of my machines to this change as
> > commit e009e088d88e ("iommu: Drop sw_msi from iommu_domain") in -next.
> 
> Okay wow, I will drop this series from the tree if I don't see a
> resolution in a few days. We can try again next cycle, thank you for
> testing and bisect!
> 
> > At this change, I see:
> > 
> >   [  +0.000000] Linux version 6.14.0-rc2-00033-ge009e088d88e (nathan@ax162) (aarch64-linux-gcc (GCC) 14.2.0, GNU ld (GNU Binutils) 2.42) #1 SMP PREEMPT_DYNAMIC Mon Mar 24 08:57:49 MST 2025
> >   ...
> >   [  +0.002355] fsl_mc_bus NXP0008:00: Adding to iommu group 0
> >   [  +0.000533] fsl_mc_bus NXP0008:00: MC firmware version: 10.28.1
> >   [  +0.002565] fsl_mc_dprc dprc.1: DMA mask not set
> >   [  +0.019255] fsl_mc_dprc dprc.1: Adding to iommu group 1
> >   [  +0.046820] fsl_mc_dprc dprc.1: Failed to allocate IRQs
> 
> I guess it is tripping up going through iommu_dma_prepare_msi()
> somehow?
> 
> Maybe fsl bus is special and doesn't manage to set
> IOMMU_COOKIE_DMA_IOVA for some reason?
> 
> I wonder if this is not right:
> 
> +               default:
> +                       ret = -EOPNOTSUPP;
> +                       break;
> 
> And it should be just break instead (return 0) which is what was
> happening before?

Yea, I found the diff here too.

Nathan, would you please give it a try by removing this
	ret = -EOPNOTSUPP;
?

Thanks
Nicolin

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nicolin Chen]

On Mon, Mar 24, 2025 at 09:55:48AM -0700, Nicolin Chen wrote:
> On Mon, Mar 24, 2025 at 01:40:23PM -0300, Jason Gunthorpe wrote:
> > On Mon, Mar 24, 2025 at 09:25:58AM -0700, Nathan Chancellor wrote:
> > 
> > > I bisected a loss of networking on one of my machines to this change as
> > > commit e009e088d88e ("iommu: Drop sw_msi from iommu_domain") in -next.
> > 
> > Okay wow, I will drop this series from the tree if I don't see a
> > resolution in a few days. We can try again next cycle, thank you for
> > testing and bisect!
> > 
> > > At this change, I see:
> > > 
> > >   [  +0.000000] Linux version 6.14.0-rc2-00033-ge009e088d88e (nathan@ax162) (aarch64-linux-gcc (GCC) 14.2.0, GNU ld (GNU Binutils) 2.42) #1 SMP PREEMPT_DYNAMIC Mon Mar 24 08:57:49 MST 2025
> > >   ...
> > >   [  +0.002355] fsl_mc_bus NXP0008:00: Adding to iommu group 0
> > >   [  +0.000533] fsl_mc_bus NXP0008:00: MC firmware version: 10.28.1
> > >   [  +0.002565] fsl_mc_dprc dprc.1: DMA mask not set
> > >   [  +0.019255] fsl_mc_dprc dprc.1: Adding to iommu group 1
> > >   [  +0.046820] fsl_mc_dprc dprc.1: Failed to allocate IRQs
> > 
> > I guess it is tripping up going through iommu_dma_prepare_msi()
> > somehow?
> > 
> > Maybe fsl bus is special and doesn't manage to set
> > IOMMU_COOKIE_DMA_IOVA for some reason?
> > 
> > I wonder if this is not right:
> > 
> > +               default:
> > +                       ret = -EOPNOTSUPP;
> > +                       break;
> > 
> > And it should be just break instead (return 0) which is what was
> > happening before?
> 
> Yea, I found the diff here too.

Or it should return 0 just for "case IOMMU_COOKIE_NONE" doing a
passthrough for !CONFIG_IRQ_MSI_IOMMU configuration.

Thanks
Nicolin

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nathan Chancellor]

On Mon, Mar 24, 2025 at 09:55:46AM -0700, Nicolin Chen wrote:
> On Mon, Mar 24, 2025 at 01:40:23PM -0300, Jason Gunthorpe wrote:
> > On Mon, Mar 24, 2025 at 09:25:58AM -0700, Nathan Chancellor wrote:
> > 
> > > I bisected a loss of networking on one of my machines to this change as
> > > commit e009e088d88e ("iommu: Drop sw_msi from iommu_domain") in -next.
> > 
> > Okay wow, I will drop this series from the tree if I don't see a
> > resolution in a few days. We can try again next cycle, thank you for
> > testing and bisect!
> > 
> > > At this change, I see:
> > > 
> > >   [  +0.000000] Linux version 6.14.0-rc2-00033-ge009e088d88e (nathan@ax162) (aarch64-linux-gcc (GCC) 14.2.0, GNU ld (GNU Binutils) 2.42) #1 SMP PREEMPT_DYNAMIC Mon Mar 24 08:57:49 MST 2025
> > >   ...
> > >   [  +0.002355] fsl_mc_bus NXP0008:00: Adding to iommu group 0
> > >   [  +0.000533] fsl_mc_bus NXP0008:00: MC firmware version: 10.28.1
> > >   [  +0.002565] fsl_mc_dprc dprc.1: DMA mask not set
> > >   [  +0.019255] fsl_mc_dprc dprc.1: Adding to iommu group 1
> > >   [  +0.046820] fsl_mc_dprc dprc.1: Failed to allocate IRQs
> > 
> > I guess it is tripping up going through iommu_dma_prepare_msi()
> > somehow?
> > 
> > Maybe fsl bus is special and doesn't manage to set
> > IOMMU_COOKIE_DMA_IOVA for some reason?
> > 
> > I wonder if this is not right:
> > 
> > +               default:
> > +                       ret = -EOPNOTSUPP;
> > +                       break;
> > 
> > And it should be just break instead (return 0) which is what was
> > happening before?
> 
> Yea, I found the diff here too.
> 
> Nathan, would you please give it a try by removing this
> 	ret = -EOPNOTSUPP;
> ?

Can confirm, I tested the following diff and it resolved my issue. If it
goes as a standalone patch:

Tested-by: Nathan Chancellor <nathan@kernel.org>

Thanks for the quick triage!

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0f4cc15ded1c..2b81166350ae 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3661,7 +3661,6 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 			ret = iommufd_sw_msi(group->domain, desc, msi_addr);
 			break;
 		default:
-			ret = -EOPNOTSUPP;
 			break;
 		}
 	}

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Jason Gunthorpe]

On Mon, Mar 24, 2025 at 10:07:43AM -0700, Nathan Chancellor wrote:

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0f4cc15ded1c..2b81166350ae 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3661,7 +3661,6 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
>  			ret = iommufd_sw_msi(group->domain, desc, msi_addr);
>  			break;
>  		default:
> -			ret = -EOPNOTSUPP;
>  			break;
>  		}
>  	}

Can we explain why this scenario has a 0 cookie_type?

Actually.. Is it just an identity domain? Nicolin did you test this on
your arm system with a device using identity (iommu=pt kernel param)?
I would expect identity to end up with a 0 cookie because we never
setup dma-iommu.c code on it.

Should we be testing for identity to return 0 instead?

Jason

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nathan Chancellor]

On Mon, Mar 24, 2025 at 05:05:01PM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 24, 2025 at 10:07:43AM -0700, Nathan Chancellor wrote:
> 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 0f4cc15ded1c..2b81166350ae 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -3661,7 +3661,6 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
> >  			ret = iommufd_sw_msi(group->domain, desc, msi_addr);
> >  			break;
> >  		default:
> > -			ret = -EOPNOTSUPP;
> >  			break;
> >  		}
> >  	}
> 
> Can we explain why this scenario has a 0 cookie_type?
> 
> Actually.. Is it just an identity domain? Nicolin did you test this on
> your arm system with a device using identity (iommu=pt kernel param)?
> I would expect identity to end up with a 0 cookie because we never
> setup dma-iommu.c code on it.
> 
> Should we be testing for identity to return 0 instead?

For the record, the particular system I noticed the issue on does need
"iommu.passthrough=1" (which is the ARM equivalent to "iommu=pt" IIUC?)
to boot due to a lack of firmware support for IORT RMR, so I think the
answer to your first question is probably yes?

Cheers,
Nathan

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nicolin Chen]

On Mon, Mar 24, 2025 at 01:43:52PM -0700, Nathan Chancellor wrote:
> On Mon, Mar 24, 2025 at 05:05:01PM -0300, Jason Gunthorpe wrote:
> > On Mon, Mar 24, 2025 at 10:07:43AM -0700, Nathan Chancellor wrote:
> > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 0f4cc15ded1c..2b81166350ae 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -3661,7 +3661,6 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
> > >  			ret = iommufd_sw_msi(group->domain, desc, msi_addr);
> > >  			break;
> > >  		default:
> > > -			ret = -EOPNOTSUPP;
> > >  			break;
> > >  		}
> > >  	}
> > 
> > Can we explain why this scenario has a 0 cookie_type?
> > 
> > Actually.. Is it just an identity domain? Nicolin did you test this on
> > your arm system with a device using identity (iommu=pt kernel param)?
> > I would expect identity to end up with a 0 cookie because we never
> > setup dma-iommu.c code on it.
> > 
> > Should we be testing for identity to return 0 instead?

My feeling is that we should just let all other cases return 0
like the previous function did, as this seems to be commonly on
the IRQ allocation path that shouldn't fail like this. E.g. if
we fail a blocked domain, would it retry switching domains?

> For the record, the particular system I noticed the issue on does need
> "iommu.passthrough=1" (which is the ARM equivalent to "iommu=pt" IIUC?)
> to boot due to a lack of firmware support for IORT RMR, so I think the
> answer to your first question is probably yes?

Yea, I confirmed that identity domain on ARM fails too. My bad
that I didn't catch this case.

Thanks
Nicolin

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Jason Gunthorpe]

On Mon, Mar 24, 2025 at 02:38:33PM -0700, Nicolin Chen wrote:

> My feeling is that we should just let all other cases return 0
> like the previous function did, as this seems to be commonly on
> the IRQ allocation path that shouldn't fail like this. E.g. if
> we fail a blocked domain, would it retry switching domains?

I don't know, I think if the interrupt driver is calling this function
it knows the MSI is translated by the iommu and if the iommu is set to
something like blocked then it really should fail rather than silently
not work. Same for a paging domain without any kind of sw_msi route.

So, I'm feeling like we should check for identity and still fail the
other cases?

Can you send another version of the series with this and Arnd's two
fixes integrated?

Thanks,
Jason

Re: [PATCH v4 3/3] iommu: Drop sw_msi from iommu_domain

[Nicolin Chen]

On Mon, Mar 24, 2025 at 07:29:00PM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 24, 2025 at 02:38:33PM -0700, Nicolin Chen wrote:
> 
> > My feeling is that we should just let all other cases return 0
> > like the previous function did, as this seems to be commonly on
> > the IRQ allocation path that shouldn't fail like this. E.g. if
> > we fail a blocked domain, would it retry switching domains?
> 
> I don't know, I think if the interrupt driver is calling this function
> it knows the MSI is translated by the iommu and if the iommu is set to
> something like blocked then it really should fail rather than silently
> not work. Same for a paging domain without any kind of sw_msi route.

OK. That sounds correct to me.

I will give the default blocked domain case a try. If later the
irqchip driver can still allocate IRQ after it switches to some
normal working domain, then we will be safe to do so.

> So, I'm feeling like we should check for identity and still fail the
> other cases?
> 
> Can you send another version of the series with this and Arnd's two
> fixes integrated?

Yes, I will do that. And I assume they will be still rebased on:
da0c56520e88 iommu/arm-smmu-v3: Set MEV bit in nested STE for DoS mitigations

Will take some time run some tests with different combinations
this time. Sorry for the trouble. Should have been more careful.

Nicolin

Re: [PATCH v4 0/3] iommu: Clean up cookie and sw_msi in struct iommu_domain

[Jason Gunthorpe]

On Thu, Mar 06, 2025 at 01:00:46PM -0800, Nicolin Chen wrote:

> Changelog
> v4
>  * Rebase on top of a bug fix for hwpt_iommufd cookie
>    https://lore.kernel.org/all/20250305211800.229465-1-nicolinc@nvidia.com/

I suppose this means it needs to go to the iommufd tree?

Joerg OK?

Thanks,
Jason

Re: [PATCH v4 0/3] iommu: Clean up cookie and sw_msi in struct iommu_domain

[Jason Gunthorpe]

On Thu, Mar 06, 2025 at 01:00:46PM -0800, Nicolin Chen wrote:

> Nicolin Chen (2):
>   iommufd: Move iommufd_sw_msi and related functions to driver.c
>   iommu: Drop sw_msi from iommu_domain
> 
> Robin Murphy (1):
>   iommu: Sort out domain user data
> 
>  drivers/iommu/dma-iommu.h               |  14 ++
>  drivers/iommu/iommu-priv.h              |  13 ++
>  drivers/iommu/iommufd/iommufd_private.h |   7 +-
>  include/linux/iommu.h                   |  35 ++--
>  drivers/iommu/dma-iommu.c               | 208 ++++++++++++------------
>  drivers/iommu/iommu-sva.c               |   1 +
>  drivers/iommu/iommu.c                   |  35 +++-
>  drivers/iommu/iommufd/device.c          | 131 +--------------
>  drivers/iommu/iommufd/driver.c          | 125 ++++++++++++++
>  drivers/iommu/iommufd/hw_pagetable.c    |   6 +-
>  10 files changed, 320 insertions(+), 255 deletions(-)

Applied 

Thanks,
Jason