| Message ID | 949e28875e01646feac5c4951b63723579d29b36.1740014950.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: Add MSI mapping support with nested SMMU (Part-1 core) | expand |
On Wed, Feb 19, 2025 at 05:31:42PM -0800, Nicolin Chen wrote: > 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(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Wed, Feb 19, 2025 at 05:31:42PM -0800, Nicolin Chen wrote: > 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 e93d2e918599..99dd72998cb7 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 */ > }; Ugh, there is a problem here: void iommu_domain_free(struct iommu_domain *domain) { if (domain->type == IOMMU_DOMAIN_SVA) mmdrop(domain->mm); iommu_put_dma_cookie(domain); It calls into dma-iommu for all domain types And if !CONFIG_IRQ_MSI_IOMMU then this isn't possible to protect it against iommufd owning the cookie union: #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU) if (domain->sw_msi != iommu_dma_sw_msi) return; #endif I came up with the below, but I will drop this patch for now you can repost it as a cleanup series.. Jason diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 3b58244e6344a5..31d53552dc4790 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,13 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base) } EXPORT_SYMBOL(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 @@ -449,8 +457,10 @@ 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) return; diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 022bf96a18c5e4..f07544b290e5b1 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -456,6 +456,12 @@ 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_put_dma_cookie(domain); + iommu_domain_free(domain); +} + static void iommu_deinit_device(struct device *dev) { struct iommu_group *group = dev->iommu_group; @@ -494,7 +500,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) { @@ -2023,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); } @@ -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; diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 50ebc9593c9d70..b5bb946c9c1b19 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); } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 99dd72998cb7f7..082274e8ba6a3d 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); +void iommu_put_msi_cookie(struct iommu_domain *domain); #else /* CONFIG_IOMMU_DMA */ 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 */
On 2025-02-21 2:39 pm, Jason Gunthorpe wrote: > On Wed, Feb 19, 2025 at 05:31:42PM -0800, Nicolin Chen wrote: >> 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 e93d2e918599..99dd72998cb7 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 */ >> }; > > Ugh, there is a problem here: > > void iommu_domain_free(struct iommu_domain *domain) > { > if (domain->type == IOMMU_DOMAIN_SVA) > mmdrop(domain->mm); > iommu_put_dma_cookie(domain); > > It calls into dma-iommu for all domain types > > And if !CONFIG_IRQ_MSI_IOMMU then this isn't possible to protect it > against iommufd owning the cookie union: > > #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU) > if (domain->sw_msi != iommu_dma_sw_msi) > return; > #endif > > I came up with the below, but I will drop this patch for now you can > repost it as a cleanup series.. Eww... What's the issue with just checking the domain type in iommu_put_dma_cookie()? Is is that IOMMUFD and VFIO type 1 are both doing their own different thing with IOMMU_DOMAIN_UNMANAGED? In general it seems like a bad smell to have a union in a structure with not enough information within that structire itself to know which union member is valid... :/ Thanks, Robin. > > Jason > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 3b58244e6344a5..31d53552dc4790 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,13 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base) > } > EXPORT_SYMBOL(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 > @@ -449,8 +457,10 @@ 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) > return; > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 022bf96a18c5e4..f07544b290e5b1 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -456,6 +456,12 @@ 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_put_dma_cookie(domain); > + iommu_domain_free(domain); > +} > + > static void iommu_deinit_device(struct device *dev) > { > struct iommu_group *group = dev->iommu_group; > @@ -494,7 +500,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) { > @@ -2023,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); > } > @@ -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; > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 50ebc9593c9d70..b5bb946c9c1b19 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); > } > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 99dd72998cb7f7..082274e8ba6a3d 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); > +void iommu_put_msi_cookie(struct iommu_domain *domain); > #else /* CONFIG_IOMMU_DMA */ > 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 */ >
On Fri, Feb 21, 2025 at 03:23:22PM +0000, Robin Murphy wrote: > Eww... What's the issue with just checking the domain type in > iommu_put_dma_cookie()? Is is that IOMMUFD and VFIO type 1 are both doing > their own different thing with IOMMU_DOMAIN_UNMANAGED? Yes > In general it seems like a bad smell to have a union in a structure with not > enough information within that structire itself to know which union member > is valid... :/ The concept is the opaque pointer belongs only to the caller that allocated and owns the domain. The core iommu code should never look at it or touch it. The problem is with the mandatory call to dma-iommu in the free path - dma-iommu code should never be invoked outside of VFIO and the default domain cases. So the little rework I sketched makes it into the caller knowing if dma-iommu is operating that domain and then only does it call the dma-iommu related functions, and the core code never touches the union content. Jason
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index e93d2e918599..99dd72998cb7 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 */
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(-)