| Message ID | 2-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023/8/3 8:07, Jason Gunthorpe wrote: > This is used when the iommu driver is taking control of the dma_ops, > currently only on S390 and power spapr. It is designed to preserve the > original ops->detach_dev() semantic that these S390 was built around. > > Provide an opaque domain type and a 'default_domain' ops value that allows > the driver to trivially force any single domain as the default domain. > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 14 +++++++++++++- > include/linux/iommu.h | 6 ++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 5e3cdc9f3a9e78..c64365169b678d 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1705,6 +1705,17 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) > > lockdep_assert_held(&group->mutex); > > + /* > + * Allow legacy drivers to specify the domain that will be the default > + * domain. This should always be either an IDENTITY or PLATFORM domain. > + * Do not use in new drivers. > + */ > + if (bus->iommu_ops->default_domain) { > + if (req_type) > + return ERR_PTR(-EINVAL); > + return bus->iommu_ops->default_domain; > + } > + > if (req_type) > return __iommu_group_alloc_default_domain(bus, group, req_type); > > @@ -1967,7 +1978,8 @@ void iommu_domain_free(struct iommu_domain *domain) > if (domain->type == IOMMU_DOMAIN_SVA) > mmdrop(domain->mm); > iommu_put_dma_cookie(domain); > - domain->ops->free(domain); > + if (domain->ops->free) > + domain->ops->free(domain); > } > EXPORT_SYMBOL_GPL(iommu_domain_free); > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index e05c93b6c37fba..87aebba474e093 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -64,6 +64,7 @@ struct iommu_domain_geometry { > #define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */ > > #define __IOMMU_DOMAIN_SVA (1U << 4) /* Shared process address space */ > +#define __IOMMU_DOMAIN_PLATFORM (1U << 5) > > #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ > /* > @@ -81,6 +82,8 @@ struct iommu_domain_geometry { > * invalidation. > * IOMMU_DOMAIN_SVA - DMA addresses are shared process addresses > * represented by mm_struct's. > + * IOMMU_DOMAIN_PLATFORM - Legacy domain for drivers that do their own > + * dma_api stuff. Do not use in new drivers. > */ > #define IOMMU_DOMAIN_BLOCKED (0U) > #define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT) > @@ -91,6 +94,7 @@ struct iommu_domain_geometry { > __IOMMU_DOMAIN_DMA_API | \ > __IOMMU_DOMAIN_DMA_FQ) > #define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SVA) > +#define IOMMU_DOMAIN_PLATFORM (__IOMMU_DOMAIN_PLATFORM) > > struct iommu_domain { > unsigned type; > @@ -256,6 +260,7 @@ struct iommu_iotlb_gather { > * @owner: Driver module providing these ops > * @identity_domain: An always available, always attachable identity > * translation. > + * @default_domain: If not NULL this will always be set as the default domain. > */ > struct iommu_ops { > bool (*capable)(struct device *dev, enum iommu_cap); > @@ -290,6 +295,7 @@ struct iommu_ops { > unsigned long pgsize_bitmap; > struct module *owner; > struct iommu_domain *identity_domain; > + struct iommu_domain *default_domain; I am imaging whether we can merge above two pointers into a single one. It is either an IDENTITY or PLATFORM domain and the core will choose it as the default domain of a group if iommu_group_alloc_default_domain() fails to allocate one through the iommu dev_ops. Those iommu drivers that could result in a NULL default domain could provide such domain and guarantee that this domain is always usable for devices. Probably we could give it a more meaningful name? For example, supplemental_domain or rescue_domain? I am not sure whether this can address the NULL-default-domain issues of all drivers this series tries to address. So just for discussion purpose. Best regards, baolu
On 2023/8/3 8:07, Jason Gunthorpe wrote: > @@ -1967,7 +1978,8 @@ void iommu_domain_free(struct iommu_domain *domain) > if (domain->type == IOMMU_DOMAIN_SVA) > mmdrop(domain->mm); > iommu_put_dma_cookie(domain); > - domain->ops->free(domain); > + if (domain->ops->free) > + domain->ops->free(domain); > } > EXPORT_SYMBOL_GPL(iommu_domain_free); > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index e05c93b6c37fba..87aebba474e093 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -64,6 +64,7 @@ struct iommu_domain_geometry { > #define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */ > > #define __IOMMU_DOMAIN_SVA (1U << 4) /* Shared process address space */ > +#define __IOMMU_DOMAIN_PLATFORM (1U << 5) > > #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ > /* > @@ -81,6 +82,8 @@ struct iommu_domain_geometry { > * invalidation. > * IOMMU_DOMAIN_SVA - DMA addresses are shared process addresses > * represented by mm_struct's. > + * IOMMU_DOMAIN_PLATFORM - Legacy domain for drivers that do their own > + * dma_api stuff. Do not use in new drivers. > */ > #define IOMMU_DOMAIN_BLOCKED (0U) > #define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT) > @@ -91,6 +94,7 @@ struct iommu_domain_geometry { > __IOMMU_DOMAIN_DMA_API | \ > __IOMMU_DOMAIN_DMA_FQ) > #define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SVA) > +#define IOMMU_DOMAIN_PLATFORM (__IOMMU_DOMAIN_PLATFORM) Nit: As a default domain could be the type of IOMMU_DOMAIN_PLATFORM, static const char *iommu_domain_type_str(unsigned int t) needs to be updated, so that users can get a right string when reading /sys/.../[group_id]/type. Best regards, baolu
On Sat, Aug 12, 2023 at 09:36:33AM +0800, Baolu Lu wrote: > > @@ -290,6 +295,7 @@ struct iommu_ops { > > unsigned long pgsize_bitmap; > > struct module *owner; > > struct iommu_domain *identity_domain; > > + struct iommu_domain *default_domain; > > I am imaging whether we can merge above two pointers into a single one. > It is either an IDENTITY or PLATFORM domain and the core will choose it > as the default domain of a group if iommu_group_alloc_default_domain() > fails to allocate one through the iommu dev_ops. I think that would be the wrong direction.. identity_domain is a pointer that is always, ALWAYS an identity domain. It is the shortcut for drivers (and all drivers should do this) that implement a global static identity domain. default_domain is a shortcut to avoid implementing the entire flow around def_domain_type/domain_alloc for special cases. For this patch the specialc ase is the IOMMU_DOMAIN_PLATFORM. We'll probably also get a blocking_domain pointer here too. All of this is removing the type multiplexor in alloc_domain so we can so alloc_domain_paging() > Probably we could give it a more meaningful name? For example, > supplemental_domain or rescue_domain? But that isn't what it is for, default_domain is the operational domain for attached drivers.. Jason
On 2023/8/12 19:28, Jason Gunthorpe wrote: > On Sat, Aug 12, 2023 at 09:36:33AM +0800, Baolu Lu wrote: >>> @@ -290,6 +295,7 @@ struct iommu_ops { >>> unsigned long pgsize_bitmap; >>> struct module *owner; >>> struct iommu_domain *identity_domain; >>> + struct iommu_domain *default_domain; >> >> I am imaging whether we can merge above two pointers into a single one. >> It is either an IDENTITY or PLATFORM domain and the core will choose it >> as the default domain of a group if iommu_group_alloc_default_domain() >> fails to allocate one through the iommu dev_ops. > > I think that would be the wrong direction.. > > identity_domain is a pointer that is always, ALWAYS an identity > domain. It is the shortcut for drivers (and all drivers should do > this) that implement a global static identity domain. I see. I originally thought this was special for arm32. > > default_domain is a shortcut to avoid implementing the entire flow > around def_domain_type/domain_alloc for special cases. For this patch > the specialc ase is the IOMMU_DOMAIN_PLATFORM. I think this is special for drivers like s390. You don't want it to be used beyond those special drivers, right? If so, the naming of default_domain seems to be a bit generic. I can't think of a better one, hence I am fine if you keep as it-is. After all, the comment for this field has already explained it very clearly. > We'll probably also get a blocking_domain pointer here too. Yes. > > All of this is removing the type multiplexor in alloc_domain so we can > so alloc_domain_paging() Agreed with you. The dummy domains like identity and blocking could be avoided from calling ops->domain_alloc. >> Probably we could give it a more meaningful name? For example, >> supplemental_domain or rescue_domain? > > But that isn't what it is for, default_domain is the operational > domain for attached drivers.. Best regards, baolu
On Sun, Aug 13, 2023 at 08:11:30PM +0800, Baolu Lu wrote: > On 2023/8/12 19:28, Jason Gunthorpe wrote: > > On Sat, Aug 12, 2023 at 09:36:33AM +0800, Baolu Lu wrote: > > > > @@ -290,6 +295,7 @@ struct iommu_ops { > > > > unsigned long pgsize_bitmap; > > > > struct module *owner; > > > > struct iommu_domain *identity_domain; > > > > + struct iommu_domain *default_domain; > > > > > > I am imaging whether we can merge above two pointers into a single one. > > > It is either an IDENTITY or PLATFORM domain and the core will choose it > > > as the default domain of a group if iommu_group_alloc_default_domain() > > > fails to allocate one through the iommu dev_ops. > > > > I think that would be the wrong direction.. > > > > identity_domain is a pointer that is always, ALWAYS an identity > > domain. It is the shortcut for drivers (and all drivers should do > > this) that implement a global static identity domain. > > I see. I originally thought this was special for arm32. No, everything should use it eventually. Eg you would convert the Intel driver too. It makes the function always available in all error unwind paths. > > default_domain is a shortcut to avoid implementing the entire flow > > around def_domain_type/domain_alloc for special cases. For this patch > > the specialc ase is the IOMMU_DOMAIN_PLATFORM. > > I think this is special for drivers like s390. You don't want it to be > used beyond those special drivers, right? It ended up here: arch/powerpc/kernel/iommu.c: .default_domain = &spapr_tce_platform_domain, drivers/iommu/fsl_pamu_domain.c: .default_domain = &fsl_pamu_platform_domain, drivers/iommu/iommufd/selftest.c: .default_domain = &mock_blocking_domain, drivers/iommu/s390-iommu.c: .default_domain = &s390_iommu_platform_domain, drivers/iommu/tegra-smmu.c: .default_domain = &tegra_smmu_identity_domain, And I have to try to remember why tegra-smmu had it.. So yes special places only. > If so, the naming of default_domain seems to be a bit generic. I can't > think of a better one, hence I am fine if you keep as it-is. After all, > the comment for this field has already explained it very clearly. Me too Thanks, Jason
On Sat, Aug 12, 2023 at 09:41:50AM +0800, Baolu Lu wrote: > > index e05c93b6c37fba..87aebba474e093 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -64,6 +64,7 @@ struct iommu_domain_geometry { > > #define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */ > > #define __IOMMU_DOMAIN_SVA (1U << 4) /* Shared process address space */ > > +#define __IOMMU_DOMAIN_PLATFORM (1U << 5) > > #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ > > /* > > @@ -81,6 +82,8 @@ struct iommu_domain_geometry { > > * invalidation. > > * IOMMU_DOMAIN_SVA - DMA addresses are shared process addresses > > * represented by mm_struct's. > > + * IOMMU_DOMAIN_PLATFORM - Legacy domain for drivers that do their own > > + * dma_api stuff. Do not use in new drivers. > > */ > > #define IOMMU_DOMAIN_BLOCKED (0U) > > #define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT) > > @@ -91,6 +94,7 @@ struct iommu_domain_geometry { > > __IOMMU_DOMAIN_DMA_API | \ > > __IOMMU_DOMAIN_DMA_FQ) > > #define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SVA) > > +#define IOMMU_DOMAIN_PLATFORM (__IOMMU_DOMAIN_PLATFORM) > > Nit: As a default domain could be the type of IOMMU_DOMAIN_PLATFORM, > > static const char *iommu_domain_type_str(unsigned int t) > > needs to be updated, so that users can get a right string when reading > /sys/.../[group_id]/type. Yeah, I missed that, fix it Thanks, Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 5e3cdc9f3a9e78..c64365169b678d 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1705,6 +1705,17 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) lockdep_assert_held(&group->mutex); + /* + * Allow legacy drivers to specify the domain that will be the default + * domain. This should always be either an IDENTITY or PLATFORM domain. + * Do not use in new drivers. + */ + if (bus->iommu_ops->default_domain) { + if (req_type) + return ERR_PTR(-EINVAL); + return bus->iommu_ops->default_domain; + } + if (req_type) return __iommu_group_alloc_default_domain(bus, group, req_type); @@ -1967,7 +1978,8 @@ void iommu_domain_free(struct iommu_domain *domain) if (domain->type == IOMMU_DOMAIN_SVA) mmdrop(domain->mm); iommu_put_dma_cookie(domain); - domain->ops->free(domain); + if (domain->ops->free) + domain->ops->free(domain); } EXPORT_SYMBOL_GPL(iommu_domain_free); diff --git a/include/linux/iommu.h b/include/linux/iommu.h index e05c93b6c37fba..87aebba474e093 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -64,6 +64,7 @@ struct iommu_domain_geometry { #define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */ #define __IOMMU_DOMAIN_SVA (1U << 4) /* Shared process address space */ +#define __IOMMU_DOMAIN_PLATFORM (1U << 5) #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ /* @@ -81,6 +82,8 @@ struct iommu_domain_geometry { * invalidation. * IOMMU_DOMAIN_SVA - DMA addresses are shared process addresses * represented by mm_struct's. + * IOMMU_DOMAIN_PLATFORM - Legacy domain for drivers that do their own + * dma_api stuff. Do not use in new drivers. */ #define IOMMU_DOMAIN_BLOCKED (0U) #define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT) @@ -91,6 +94,7 @@ struct iommu_domain_geometry { __IOMMU_DOMAIN_DMA_API | \ __IOMMU_DOMAIN_DMA_FQ) #define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SVA) +#define IOMMU_DOMAIN_PLATFORM (__IOMMU_DOMAIN_PLATFORM) struct iommu_domain { unsigned type; @@ -256,6 +260,7 @@ struct iommu_iotlb_gather { * @owner: Driver module providing these ops * @identity_domain: An always available, always attachable identity * translation. + * @default_domain: If not NULL this will always be set as the default domain. */ struct iommu_ops { bool (*capable)(struct device *dev, enum iommu_cap); @@ -290,6 +295,7 @@ struct iommu_ops { unsigned long pgsize_bitmap; struct module *owner; struct iommu_domain *identity_domain; + struct iommu_domain *default_domain; }; /**