[PATCH v1 0/4] iommu: Isolate iova_cookie to actual owners

[PATCH v1 0/4] iommu: Isolate iova_cookie to actual owners

[Nicolin Chen]

Now, iommufd implements its own sw_msi function that does not touch the
domain->iova_cookie but domain->iommufd_hwpt, as a domain owner pointer.

Isolate the iova_cookie from iommufd by putting it into the union where
the iommufd_hwpt is located.

This requires a set of preparations to move iommu_put_dma_cookie() out
of the common path of iommu_domain_free() that iommufd still calls.

Make thing cleaner that any caller of iommu_get_dma/msi_cookie() should
explicitly call the pairing iommu_put_dma/msi_cookie().

This is a clean-up series for the sw_msi Part-1 core series, prior to
the Part-2/3 series. It's on github:
https://github.com/nicolinc/iommufd/commits/iommufd_msi_cleanup-v1

Thanks
Nicolin

Nicolin Chen (4):
  iommu: Define iommu_get/put_msi_cookie() under CONFIG_IRQ_MSI_IOMMU
  iommu: Add iommu_default_domain_free helper
  iommu: Request iova_cookie owner to put cookie explicitly
  iommu: Turn iova_cookie to dma-iommu private pointer

 include/linux/iommu.h           | 12 ++++++++----
 drivers/iommu/dma-iommu.c       | 12 ++++++++++++
 drivers/iommu/iommu.c           | 13 +++++++++----
 drivers/vfio/vfio_iommu_type1.c |  4 ++++
 4 files changed, 33 insertions(+), 8 deletions(-)


base-commit: 598749522d4254afb33b8a6c1bea614a95896868
-- 
2.43.0

[PATCH v1 1/4] iommu: Define iommu_get/put_msi_cookie() under CONFIG_IRQ_MSI_IOMMU

[Nicolin Chen]

Not all domain owners call iommu_get_dma/msi_cookie(), e.g. iommufd has
its sw_msi implementation without using the domain->iova_cookie but the
domain->iommufd_hwpt.

To isolate the unused iova_cookie from the iommufd, iommu_domain_free()
will no longer call iommu_put_dma_cookie().

Add iommu_put_msi_cookie() to pair with iommu_put_dma_cookie() for VFIO
that is the only caller to explicitly put the MSI cookie.

Move iommufd_get/put_msi_cookie() inside "ifdef CONFIG_IRQ_MSI_IOMMU".

Note that the iommufd_get_msi_cookie now returns a 0 for NOP in case of:
 *) !CONFIG_IOMMU_DMA - the caller in VFIO would have returned prior to
                        reaching to this function.
 *) !CONFIG_IRQ_MSI_IOMMU - in a system without an irqchip driver, vfio
                            or iommufd shouldn't fail if an IOMMU driver
                            still reports SW_MSI reserved regions.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/iommu.h     | 10 +++++++---
 drivers/iommu/dma-iommu.c | 12 ++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e93d2e918599..6f66980e0c86 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1534,12 +1534,16 @@ void iommu_debugfs_setup(void);
 static inline void iommu_debugfs_setup(void) {}
 #endif
 
-#ifdef CONFIG_IOMMU_DMA
+#if defined(CONFIG_IOMMU_DMA) && IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);
-#else /* CONFIG_IOMMU_DMA */
+void iommu_put_msi_cookie(struct iommu_domain *domain);
+#else /* CONFIG_IOMMU_DMA && CONFIG_IRQ_MSI_IOMMU */
 static inline int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 {
-	return -ENODEV;
+	return 0;
+}
+static inline void iommu_put_msi_cookie(struct iommu_domain *domain)
+{
 }
 #endif	/* CONFIG_IOMMU_DMA */
 
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 94263ed2c564..228524c81b72 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -418,6 +418,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
  * number of PAGE_SIZE mappings necessary to cover every MSI doorbell address
  * used by the devices attached to @domain.
  */
+#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 {
 	struct iommu_dma_cookie *cookie;
@@ -439,6 +440,17 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 }
 EXPORT_SYMBOL(iommu_get_msi_cookie);
 
+/**
+ * iommu_put_msi_cookie - Release a domain's MSI remapping resources
+ * @domain: IOMMU domain previously prepared by iommu_get_msi_cookie()
+ */
+void iommu_put_msi_cookie(struct iommu_domain *domain)
+{
+	iommu_put_dma_cookie(domain);
+}
+EXPORT_SYMBOL_GPL(iommu_put_msi_cookie);
+#endif
+
 /**
  * iommu_put_dma_cookie - Release a domain's DMA mapping resources
  * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() or
-- 
2.43.0

[PATCH v1 2/4] iommu: Add iommu_default_domain_free helper

[Nicolin Chen]

The iommu_put_dma_cookie() will be moved out of iommu_domain_free(). For a
default domain, iommu_put_dma_cookie() can be simply added to this helper.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 022bf96a18c5..28cde7007cd7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -456,6 +456,11 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	return ret;
 }
 
+static void iommu_default_domain_free(struct iommu_domain *domain)
+{
+	iommu_domain_free(domain);
+}
+
 static void iommu_deinit_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
@@ -494,7 +499,7 @@ static void iommu_deinit_device(struct device *dev)
 	 */
 	if (list_empty(&group->devices)) {
 		if (group->default_domain) {
-			iommu_domain_free(group->default_domain);
+			iommu_default_domain_free(group->default_domain);
 			group->default_domain = NULL;
 		}
 		if (group->blocking_domain) {
@@ -3000,7 +3005,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 
 out_free_old:
 	if (old_dom)
-		iommu_domain_free(old_dom);
+		iommu_default_domain_free(old_dom);
 	return ret;
 
 err_restore_domain:
@@ -3009,7 +3014,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
 err_restore_def_domain:
 	if (old_dom) {
-		iommu_domain_free(dom);
+		iommu_default_domain_free(dom);
 		group->default_domain = old_dom;
 	}
 	return ret;
-- 
2.43.0

Re: [PATCH v1 2/4] iommu: Add iommu_default_domain_free helper

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 12:16:05PM -0800, Nicolin Chen wrote:
> The iommu_put_dma_cookie() will be moved out of iommu_domain_free(). For a
> default domain, iommu_put_dma_cookie() can be simply added to this helper.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommu.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Let's try to do what Robin suggested and put a private_data_owner
value in the struct then this patch isn't used, we'd just do

      if (domain->private_data_owner == DMA)
	iommu_put_dma_cookie(domain);

Instead of this change and the similar VFIO change

Thanks,
Jason

Re: [PATCH v1 2/4] iommu: Add iommu_default_domain_free helper

[Nicolin Chen]

On Thu, Feb 27, 2025 at 03:50:36PM -0400, Jason Gunthorpe wrote:
> On Wed, Feb 26, 2025 at 12:16:05PM -0800, Nicolin Chen wrote:
> > The iommu_put_dma_cookie() will be moved out of iommu_domain_free(). For a
> > default domain, iommu_put_dma_cookie() can be simply added to this helper.
> > 
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >  drivers/iommu/iommu.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Let's try to do what Robin suggested and put a private_data_owner
> value in the struct then this patch isn't used, we'd just do
> 
>       if (domain->private_data_owner == DMA)
> 	iommu_put_dma_cookie(domain);
> 
> Instead of this change and the similar VFIO change

Ack. I assume I should go with a smaller series starting with this
"private_data_owner", and then later a bigger series for the other
bits like translation_type that you mentioned in the other thread.

Thanks
Nicolin

Re: [PATCH v1 2/4] iommu: Add iommu_default_domain_free helper

[Jason Gunthorpe]

On Thu, Feb 27, 2025 at 12:59:04PM -0800, Nicolin Chen wrote:
> On Thu, Feb 27, 2025 at 03:50:36PM -0400, Jason Gunthorpe wrote:
> > On Wed, Feb 26, 2025 at 12:16:05PM -0800, Nicolin Chen wrote:
> > > The iommu_put_dma_cookie() will be moved out of iommu_domain_free(). For a
> > > default domain, iommu_put_dma_cookie() can be simply added to this helper.
> > > 
> > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > ---
> > >  drivers/iommu/iommu.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > Let's try to do what Robin suggested and put a private_data_owner
> > value in the struct then this patch isn't used, we'd just do
> > 
> >       if (domain->private_data_owner == DMA)
> > 	iommu_put_dma_cookie(domain);
> > 
> > Instead of this change and the similar VFIO change
> 
> Ack. I assume I should go with a smaller series starting with this
> "private_data_owner", and then later a bigger series for the other
> bits like translation_type that you mentioned in the other thread.

That could work, you could bitfiled type and steal a few bits for
"private_data_owner" ?

Then try the sw_msi removal at the same time too?

Jason

Re: [PATCH v1 2/4] iommu: Add iommu_default_domain_free helper

[Nicolin Chen]

On Thu, Feb 27, 2025 at 05:03:36PM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 27, 2025 at 12:59:04PM -0800, Nicolin Chen wrote:
> > On Thu, Feb 27, 2025 at 03:50:36PM -0400, Jason Gunthorpe wrote:
> > > On Wed, Feb 26, 2025 at 12:16:05PM -0800, Nicolin Chen wrote:
> > > > The iommu_put_dma_cookie() will be moved out of iommu_domain_free(). For a
> > > > default domain, iommu_put_dma_cookie() can be simply added to this helper.
> > > > 
> > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > > ---
> > > >  drivers/iommu/iommu.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > Let's try to do what Robin suggested and put a private_data_owner
> > > value in the struct then this patch isn't used, we'd just do
> > > 
> > >       if (domain->private_data_owner == DMA)
> > > 	iommu_put_dma_cookie(domain);
> > > 
> > > Instead of this change and the similar VFIO change
> > 
> > Ack. I assume I should go with a smaller series starting with this
> > "private_data_owner", and then later a bigger series for the other
> > bits like translation_type that you mentioned in the other thread.
> 
> That could work, you could bitfiled type and steal a few bits for
> "private_data_owner" ?
> 
> Then try the sw_msi removal at the same time too?

Ack. I drafted four patches:
  iommu: Add private_data_owner to iommu_domain_free
  iommu: Turn iova_cookie to dma-iommu private pointer
  iommufd: Move iommufd_sw_msi and related functions to driver.c
  iommu: Drop sw_msi from iommu_domain

Will do some proper build tests and then wrap them up.

Thanks
Nicolin

Re: [PATCH v1 2/4] iommu: Add iommu_default_domain_free helper

[Robin Murphy]

On 27/02/2025 11:32 pm, Nicolin Chen wrote:
> On Thu, Feb 27, 2025 at 05:03:36PM -0400, Jason Gunthorpe wrote:
>> On Thu, Feb 27, 2025 at 12:59:04PM -0800, Nicolin Chen wrote:
>>> On Thu, Feb 27, 2025 at 03:50:36PM -0400, Jason Gunthorpe wrote:
>>>> On Wed, Feb 26, 2025 at 12:16:05PM -0800, Nicolin Chen wrote:
>>>>> The iommu_put_dma_cookie() will be moved out of iommu_domain_free(). For a
>>>>> default domain, iommu_put_dma_cookie() can be simply added to this helper.
>>>>>
>>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>>>>> ---
>>>>>   drivers/iommu/iommu.c | 11 ++++++++---
>>>>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> Let's try to do what Robin suggested and put a private_data_owner
>>>> value in the struct then this patch isn't used, we'd just do
>>>>
>>>>        if (domain->private_data_owner == DMA)
>>>> 	iommu_put_dma_cookie(domain);
>>>>
>>>> Instead of this change and the similar VFIO change
>>>
>>> Ack. I assume I should go with a smaller series starting with this
>>> "private_data_owner", and then later a bigger series for the other
>>> bits like translation_type that you mentioned in the other thread.
>>
>> That could work, you could bitfiled type and steal a few bits for
>> "private_data_owner" ?
>>
>> Then try the sw_msi removal at the same time too?
> 
> Ack. I drafted four patches:
>    iommu: Add private_data_owner to iommu_domain_free
>    iommu: Turn iova_cookie to dma-iommu private pointer
>    iommufd: Move iommufd_sw_msi and related functions to driver.c
>    iommu: Drop sw_msi from iommu_domain
> 
> Will do some proper build tests and then wrap them up.

Ah, I spent yesterday also writing up a patch to sort things out more 
generally - expect to see that shortly, then we can decide what we like.

Cheers,
Robin.

[PATCH v1 3/4] iommu: Request iova_cookie owner to put cookie explicitly

[Nicolin Chen]

Not all domain owners need the iova_cookie. To isolate the iova_cookie to
the domain that actually owns it, request the owner to put the DMA or MSI
cookie explicitly, instead of calling the put() in the common path of the
iommu_domain_free().

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu.c           | 2 +-
 drivers/vfio/vfio_iommu_type1.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 28cde7007cd7..f07544b290e5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -458,6 +458,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 
 static void iommu_default_domain_free(struct iommu_domain *domain)
 {
+	iommu_put_dma_cookie(domain);
 	iommu_domain_free(domain);
 }
 
@@ -2028,7 +2029,6 @@ void iommu_domain_free(struct iommu_domain *domain)
 {
 	if (domain->type == IOMMU_DOMAIN_SVA)
 		mmdrop(domain->mm);
-	iommu_put_dma_cookie(domain);
 	if (domain->ops->free)
 		domain->ops->free(domain);
 }
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 50ebc9593c9d..b5bb946c9c1b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2271,6 +2271,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 			if (!iommu_attach_group(d->domain,
 						group->iommu_group)) {
 				list_add(&group->next, &d->group_list);
+				iommu_put_msi_cookie(domain->domain);
 				iommu_domain_free(domain->domain);
 				kfree(domain);
 				goto done;
@@ -2316,6 +2317,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 out_detach:
 	iommu_detach_group(domain->domain, group->iommu_group);
 out_domain:
+	iommu_put_msi_cookie(domain->domain);
 	iommu_domain_free(domain->domain);
 	vfio_iommu_iova_free(&iova_copy);
 	vfio_iommu_resv_free(&group_resv_regions);
@@ -2496,6 +2498,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 					vfio_iommu_unmap_unpin_reaccount(iommu);
 				}
 			}
+			iommu_put_msi_cookie(domain->domain);
 			iommu_domain_free(domain->domain);
 			list_del(&domain->next);
 			kfree(domain);
@@ -2567,6 +2570,7 @@ static void vfio_release_domain(struct vfio_domain *domain)
 		kfree(group);
 	}
 
+	iommu_put_msi_cookie(domain->domain);
 	iommu_domain_free(domain->domain);
 }
 
-- 
2.43.0

[PATCH v1 4/4] iommu: Turn iova_cookie to dma-iommu private pointer

[Nicolin Chen]

Now that iommufd does not rely on dma-iommu.c for any purpose. We can
combine the dma-iommu.c iova_cookie and the iommufd_hwpt under the same
union. This union is effectively 'owner data' and can be used by the
entity that allocated the domain. Note that legacy vfio type1 flows
continue to use dma-iommu.c for sw_msi and still need iova_cookie.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/iommu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6f66980e0c86..c2f9dc0572da 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -216,7 +216,6 @@ struct iommu_domain {
 	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)
@@ -225,6 +224,7 @@ struct iommu_domain {
 #endif
 
 	union { /* Pointer usable by owner of the domain */
+		struct iommu_dma_cookie *iova_cookie; /* dma-iommu */
 		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
 	};
 	union { /* Fault handler */
-- 
2.43.0