| Message ID | 5-v1-21cc72fcfb22+a7a-iommu_all_defdom_jgg@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023-05-01 19:02, Jason Gunthorpe wrote: > Even though dma-iommu.c and CONFIG_ARM_DMA_USE_IOMMU do approximately the > same stuff, the way they relate to the IOMMU core is quiet different. > > dma-iommu.c expects the core code to setup an UNMANAGED domain (of type > IOMMU_DOMAIN_DMA) and then configures itself to use that domain. This > becomes the default_domain for the group. > > ARM_DMA_USE_IOMMU does not use the default_domain, instead it directly > allocates an UNMANAGED domain and operates it just like an external > driver. In this case group->default_domain is NULL. > > Allow iommu drivers to specify a global static identity_domain and, if > present, automatically use this domain as the default_domain when in > ARM_DMA_USE_IOMMU mode. > > This allows drivers that implemented default_domain == NULL as an IDENTITY > translation to trivially get a properly labeled non-NULL default_domain on > ARM32 configs. > > With this arrangment when ARM_DMA_USE_IOMMU wants to disconnect from the > device the normal detach_domain flow will restore the IDENTITY domain as > the default domain. Overall this makes attach_dev() of the IDENTITY domain > called in the same places as detach_dev(). > > This effectively migrates these drivers to default_domain mode. For > drivers that support ARM64 they will gain support for the IDENTITY > translation mode for the dma_api and behave in a uniform way. > > Drivers use this by setting ops->identity_domain to a static singleton > iommu_domain that implements the identity attach. If the core detects > ARM_DMA_USE_IOMMU mode then it automatically attaches the IDENTITY domain > during probe. > > If the driver does not want to support dma_api with translation then it > always sets default_domain to the identity domain and even if IOMMU_DMA is > turned on it will not allow it to be used. Could we not retain the use of .def_domain_type for this? I think if we can avoid the IOMMU_DOMAIN_PLATFORM thing altogether than that's the more appealing option. > This allows removing the set_platform_dma_ops() from every remaining > driver. > > Add the core support and convert rockchip to use it. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 13 +++++++++++++ > drivers/iommu/rockchip-iommu.c | 19 +------------------ > include/linux/iommu.h | 3 +++ > 3 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index ba7f38630665b5..8b9af774de68f1 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1654,6 +1654,16 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) > if (req_type) > return __iommu_group_alloc_default_domain(bus, group, req_type); > > + /* > + * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an > + * identity_domain and it becomes their default domain. Later on > + * ARM_DMA_USE_IOMMU will install its UNMANAGED domain. > + */ > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && For the sake of reasoning, I think just use CONFIG_ARM for this. Otherwise we may still end up with potential corner cases of default_domain == NULL that we'd rather not have to accommodate. > + bus->iommu_ops->identity_domain) > + return __iommu_group_alloc_default_domain( > + bus, group, IOMMU_DOMAIN_IDENTITY); Why not simply return the identity_domain that we now know exists? It would seem even more logical, however, to put this ARM workaround in iommu_get_default_domain_type() and keep the actual allocation path clean. Do we strictly need to check for identity_domain up front? (Note that as it stands this narrowly misses out on un-breaking arm-smmu for 32-bit) Thanks, Robin. > + > /* The driver gave no guidance on what type to use, try the default */ > dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type); > if (dom) > @@ -1923,6 +1933,9 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus, > if (bus == NULL || bus->iommu_ops == NULL) > return NULL; > > + if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain) > + return bus->iommu_ops->identity_domain; > + > domain = bus->iommu_ops->domain_alloc(type); > if (!domain) > return NULL; > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index ea5a3088bb7e8a..9e1296a856ac4c 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -1017,13 +1017,8 @@ static int rk_iommu_identity_attach(struct iommu_domain *identity_domain, > return 0; > } > > -static void rk_iommu_identity_free(struct iommu_domain *domain) > -{ > -} > - > static struct iommu_domain_ops rk_identity_ops = { > .attach_dev = rk_iommu_identity_attach, > - .free = rk_iommu_identity_free, > }; > > static struct iommu_domain rk_identity_domain = { > @@ -1031,13 +1026,6 @@ static struct iommu_domain rk_identity_domain = { > .ops = &rk_identity_ops, > }; > > -#ifdef CONFIG_ARM > -static void rk_iommu_set_platform_dma(struct device *dev) > -{ > - WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev)); > -} > -#endif > - > static int rk_iommu_attach_device(struct iommu_domain *domain, > struct device *dev) > { > @@ -1087,9 +1075,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type) > { > struct rk_iommu_domain *rk_domain; > > - if (type == IOMMU_DOMAIN_IDENTITY) > - return &rk_identity_domain; > - > if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA) > return NULL; > > @@ -1214,13 +1199,11 @@ static int rk_iommu_of_xlate(struct device *dev, > } > > static const struct iommu_ops rk_iommu_ops = { > + .identity_domain = &rk_identity_domain, > .domain_alloc = rk_iommu_domain_alloc, > .probe_device = rk_iommu_probe_device, > .release_device = rk_iommu_release_device, > .device_group = rk_iommu_device_group, > -#ifdef CONFIG_ARM > - .set_platform_dma_ops = rk_iommu_set_platform_dma, > -#endif > .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP, > .of_xlate = rk_iommu_of_xlate, > .default_domain_ops = &(const struct iommu_domain_ops) { > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index ddcad3597c177b..427490b5736d40 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -253,6 +253,8 @@ struct iommu_iotlb_gather { > * @pgsize_bitmap: bitmap of all possible supported page sizes > * @owner: Driver module providing these ops > * @default_domain: If not NULL this will always be set as the default domain. > + * @identity_domain: An always available, always attachable identity > + * translation. > */ > struct iommu_ops { > bool (*capable)(struct device *dev, enum iommu_cap); > @@ -287,6 +289,7 @@ struct iommu_ops { > unsigned long pgsize_bitmap; > struct module *owner; > struct iommu_domain *default_domain; > + struct iommu_domain *identity_domain; > }; > > /**
On Wed, May 03, 2023 at 02:50:02PM +0100, Robin Murphy wrote: > > If the driver does not want to support dma_api with translation then it > > always sets default_domain to the identity domain and even if IOMMU_DMA is > > turned on it will not allow it to be used. > > Could we not retain the use of .def_domain_type for this? I think if we can > avoid the IOMMU_DOMAIN_PLATFORM thing altogether than that's the more > appealing option. We can, but given default_domain is already there this is the simpler implementation. If we can avoid platform then I would switch this too.. > > + /* > > + * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an > > + * identity_domain and it becomes their default domain. Later on > > + * ARM_DMA_USE_IOMMU will install its UNMANAGED domain. > > + */ > > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && > > For the sake of reasoning, I think just use CONFIG_ARM for this. Otherwise > we may still end up with potential corner cases of default_domain == NULL > that we'd rather not have to accommodate. The last patches prevent that directly. I picked this option because I want to eventually get to the point of having the special ARM_IOMMU code consolidated and marked using this ifdef so we can clearly find it all. > > + bus->iommu_ops->identity_domain) > > + return __iommu_group_alloc_default_domain( > > + bus, group, IOMMU_DOMAIN_IDENTITY); > > Why not simply return the identity_domain that we now know exists? Yeah at this patch it is true, but the later patches it has to call the function. I organized it like this to avoid some churn > It would seem even more logical, however, to put this ARM workaround in > iommu_get_default_domain_type() and keep the actual allocation path > clean. I thought about that too.. It felt it might end up more convoluted but I can try to darft it and see. It is still logical here because this is the "use the DMA API policy default" code path, and the default for ARM32 right now is this. > Do we strictly need to check for identity_domain up front? You mean when registering the driver? It does make sense > (Note that as it > stands this narrowly misses out on un-breaking arm-smmu for 32-bit) Can you elaborate? I don't know about this.. Thanks, Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index ba7f38630665b5..8b9af774de68f1 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1654,6 +1654,16 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) if (req_type) return __iommu_group_alloc_default_domain(bus, group, req_type); + /* + * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an + * identity_domain and it becomes their default domain. Later on + * ARM_DMA_USE_IOMMU will install its UNMANAGED domain. + */ + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && + bus->iommu_ops->identity_domain) + return __iommu_group_alloc_default_domain( + bus, group, IOMMU_DOMAIN_IDENTITY); + /* The driver gave no guidance on what type to use, try the default */ dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type); if (dom) @@ -1923,6 +1933,9 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus, if (bus == NULL || bus->iommu_ops == NULL) return NULL; + if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain) + return bus->iommu_ops->identity_domain; + domain = bus->iommu_ops->domain_alloc(type); if (!domain) return NULL; diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index ea5a3088bb7e8a..9e1296a856ac4c 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -1017,13 +1017,8 @@ static int rk_iommu_identity_attach(struct iommu_domain *identity_domain, return 0; } -static void rk_iommu_identity_free(struct iommu_domain *domain) -{ -} - static struct iommu_domain_ops rk_identity_ops = { .attach_dev = rk_iommu_identity_attach, - .free = rk_iommu_identity_free, }; static struct iommu_domain rk_identity_domain = { @@ -1031,13 +1026,6 @@ static struct iommu_domain rk_identity_domain = { .ops = &rk_identity_ops, }; -#ifdef CONFIG_ARM -static void rk_iommu_set_platform_dma(struct device *dev) -{ - WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev)); -} -#endif - static int rk_iommu_attach_device(struct iommu_domain *domain, struct device *dev) { @@ -1087,9 +1075,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type) { struct rk_iommu_domain *rk_domain; - if (type == IOMMU_DOMAIN_IDENTITY) - return &rk_identity_domain; - if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA) return NULL; @@ -1214,13 +1199,11 @@ static int rk_iommu_of_xlate(struct device *dev, } static const struct iommu_ops rk_iommu_ops = { + .identity_domain = &rk_identity_domain, .domain_alloc = rk_iommu_domain_alloc, .probe_device = rk_iommu_probe_device, .release_device = rk_iommu_release_device, .device_group = rk_iommu_device_group, -#ifdef CONFIG_ARM - .set_platform_dma_ops = rk_iommu_set_platform_dma, -#endif .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP, .of_xlate = rk_iommu_of_xlate, .default_domain_ops = &(const struct iommu_domain_ops) { diff --git a/include/linux/iommu.h b/include/linux/iommu.h index ddcad3597c177b..427490b5736d40 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -253,6 +253,8 @@ struct iommu_iotlb_gather { * @pgsize_bitmap: bitmap of all possible supported page sizes * @owner: Driver module providing these ops * @default_domain: If not NULL this will always be set as the default domain. + * @identity_domain: An always available, always attachable identity + * translation. */ struct iommu_ops { bool (*capable)(struct device *dev, enum iommu_cap); @@ -287,6 +289,7 @@ struct iommu_ops { unsigned long pgsize_bitmap; struct module *owner; struct iommu_domain *default_domain; + struct iommu_domain *identity_domain; }; /**
Even though dma-iommu.c and CONFIG_ARM_DMA_USE_IOMMU do approximately the same stuff, the way they relate to the IOMMU core is quiet different. dma-iommu.c expects the core code to setup an UNMANAGED domain (of type IOMMU_DOMAIN_DMA) and then configures itself to use that domain. This becomes the default_domain for the group. ARM_DMA_USE_IOMMU does not use the default_domain, instead it directly allocates an UNMANAGED domain and operates it just like an external driver. In this case group->default_domain is NULL. Allow iommu drivers to specify a global static identity_domain and, if present, automatically use this domain as the default_domain when in ARM_DMA_USE_IOMMU mode. This allows drivers that implemented default_domain == NULL as an IDENTITY translation to trivially get a properly labeled non-NULL default_domain on ARM32 configs. With this arrangment when ARM_DMA_USE_IOMMU wants to disconnect from the device the normal detach_domain flow will restore the IDENTITY domain as the default domain. Overall this makes attach_dev() of the IDENTITY domain called in the same places as detach_dev(). This effectively migrates these drivers to default_domain mode. For drivers that support ARM64 they will gain support for the IDENTITY translation mode for the dma_api and behave in a uniform way. Drivers use this by setting ops->identity_domain to a static singleton iommu_domain that implements the identity attach. If the core detects ARM_DMA_USE_IOMMU mode then it automatically attaches the IDENTITY domain during probe. If the driver does not want to support dma_api with translation then it always sets default_domain to the identity domain and even if IOMMU_DMA is turned on it will not allow it to be used. This allows removing the set_platform_dma_ops() from every remaining driver. Add the core support and convert rockchip to use it. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/iommu.c | 13 +++++++++++++ drivers/iommu/rockchip-iommu.c | 19 +------------------ include/linux/iommu.h | 3 +++ 3 files changed, 17 insertions(+), 18 deletions(-)