Message ID | 0-v7-de04a3217c48+15055-iommu_all_defdom_jgg@nvidia.com |
---|---|
Headers | show |
Series | iommu: Make default_domain's mandatory | expand |
On Wed, Aug 23, 2023 at 01:47:16PM -0300, 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. > > Update iommufd selftest to use this instead of set_platform_dma_ops > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 13 +++++++++++++ > drivers/iommu/iommufd/selftest.c | 14 +++++--------- > include/linux/iommu.h | 6 ++++++ > 3 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 33bd1107090720..7cedb0640290c8 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -184,6 +184,8 @@ static const char *iommu_domain_type_str(unsigned int t) > case IOMMU_DOMAIN_DMA: > case IOMMU_DOMAIN_DMA_FQ: > return "Translated"; > + case IOMMU_DOMAIN_PLATFORM: > + return "Platform"; > default: > return "Unknown"; > } > @@ -1752,6 +1754,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. > + */ Would it be worthwhile to mention this in iommu.h for the iommu_ops default_domain? > + 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); > > diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c > index d48a202a7c3b81..fb981ba97c4e87 100644 > --- a/drivers/iommu/iommufd/selftest.c > +++ b/drivers/iommu/iommufd/selftest.c > @@ -281,14 +281,6 @@ static bool mock_domain_capable(struct device *dev, enum iommu_cap cap) > return cap == IOMMU_CAP_CACHE_COHERENCY; > } > > -static void mock_domain_set_plaform_dma_ops(struct device *dev) > -{ > - /* > - * mock doesn't setup default domains because we can't hook into the > - * normal probe path > - */ > -} > - > static struct iommu_device mock_iommu_device = { > }; > > @@ -298,12 +290,16 @@ static struct iommu_device *mock_probe_device(struct device *dev) > } > > static const struct iommu_ops mock_ops = { > + /* > + * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type() > + * because it is zero. > + */ > + .default_domain = &mock_blocking_domain, > .owner = THIS_MODULE, > .pgsize_bitmap = MOCK_IO_PAGE_SIZE, > .hw_info = mock_domain_hw_info, > .domain_alloc = mock_domain_alloc, > .capable = mock_domain_capable, > - .set_platform_dma_ops = mock_domain_set_plaform_dma_ops, > .device_group = generic_device_group, > .probe_device = mock_probe_device, > .default_domain_ops = > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index d0920b2a9f1c0e..48a18b6e07abff 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; > @@ -262,6 +266,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); > @@ -297,6 +302,7 @@ struct iommu_ops { > unsigned long pgsize_bitmap; > struct module *owner; > struct iommu_domain *identity_domain; > + struct iommu_domain *default_domain; > }; > > /** > -- > 2.41.0 >
On Thu, Aug 24, 2023 at 06:51:48PM -0700, Jerry Snitselaar wrote: > > + /* > > + * 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. > > + */ > > Would it be worthwhile to mention this in iommu.h for the iommu_ops default_domain? I did this: diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 11d47f9ac9b345..7fa53d28feca87 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1757,8 +1757,8 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) /* * 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. + * domain. This should always be either an IDENTITY/BLOCKED/PLATFORM + * domain. Do not use in new drivers. */ if (ops->default_domain) { if (req_type) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 7e9d94a56f473e..6f9e0aacc4431a 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -267,6 +267,8 @@ struct iommu_iotlb_gather { * @blocked_domain: An always available, always attachable blocking * translation. * @default_domain: If not NULL this will always be set as the default domain. + * This should be an IDENTITY/BLOCKED/PLATFORM domain. + * Do not use in new drivers. */ struct iommu_ops { bool (*capable)(struct device *dev, enum iommu_cap); Thanks, Jason
On Fri, Aug 25, 2023 at 02:40:10PM -0300, Jason Gunthorpe wrote: > On Thu, Aug 24, 2023 at 06:51:48PM -0700, Jerry Snitselaar wrote: > > > > + /* > > > + * 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. > > > + */ > > > > Would it be worthwhile to mention this in iommu.h for the iommu_ops default_domain? > > I did this: > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 11d47f9ac9b345..7fa53d28feca87 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1757,8 +1757,8 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) > > /* > * 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. > + * domain. This should always be either an IDENTITY/BLOCKED/PLATFORM > + * domain. Do not use in new drivers. > */ > if (ops->default_domain) { > if (req_type) > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 7e9d94a56f473e..6f9e0aacc4431a 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -267,6 +267,8 @@ struct iommu_iotlb_gather { > * @blocked_domain: An always available, always attachable blocking > * translation. > * @default_domain: If not NULL this will always be set as the default domain. > + * This should be an IDENTITY/BLOCKED/PLATFORM domain. > + * Do not use in new drivers. > */ > struct iommu_ops { > bool (*capable)(struct device *dev, enum iommu_cap); > > Thanks, > Jason > For all of 02/24 Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Wed, Aug 23, 2023 at 01:47:17PM -0300, Jason Gunthorpe wrote: > POWER is using the set_platform_dma_ops() callback to hook up its private > dma_ops, but this is buired under some indirection and is weirdly > happening for a BLOCKED domain as well. > > For better documentation create a PLATFORM domain to manage the dma_ops, > since that is what it is for, and make the BLOCKED domain an alias for > it. BLOCKED is required for VFIO. > > Also removes the leaky allocation of the BLOCKED domain by using a global > static. > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > arch/powerpc/kernel/iommu.c | 38 +++++++++++++++++-------------------- > 1 file changed, 17 insertions(+), 21 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:20PM -0300, Jason Gunthorpe wrote: > Thierry says this is not used anymore, and doesn't think it makes sense as > an iommu driver. The HW it supports is about 10 years old now and newer HW > uses different IOMMU drivers. > > As this is the only driver with a GART approach, and it doesn't really > meet the driver expectations from the IOMMU core, let's just remove it > so we don't have to think about how to make it fit in. > > It has a number of identified problems: > - The assignment of iommu_groups doesn't match the HW behavior > > - It claims to have an UNMANAGED domain but it is really an IDENTITY > domain with a translation aperture. This is inconsistent with the core > expectation for security sensitive operations > > - It doesn't implement a SW page table under struct iommu_domain so > * It can't accept a map until the domain is attached > * It forgets about all maps after the domain is detached > * It doesn't clear the HW of maps once the domain is detached > (made worse by having the wrong groups) > > Cc: Thierry Reding <treding@nvidia.com> > Cc: Dmitry Osipenko <digetx@gmail.com> > Acked-by: Thierry Reding <treding@nvidia.com> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:21PM -0300, Jason Gunthorpe wrote: > What mtk does during mtk_iommu_v1_set_platform_dma() is actually putting > the iommu into identity mode. Make this available as a proper IDENTITY > domain. > > The mtk_iommu_v1_def_domain_type() from > commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type") explains > this was needed to allow probe_finalize() to be called, but now the > IDENTITY domain will do the same job so change the returned > def_domain_type. > > mkt_v1 is the only driver that returns IOMMU_DOMAIN_UNMANAGED from > def_domain_type(). This allows the next patch to enforce an IDENTITY > domain policy for this driver. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/mtk_iommu_v1.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:22PM -0300, Jason Gunthorpe wrote: > Except for dart (which forces IOMMU_DOMAIN_DMA) every driver returns 0 or > IDENTITY from ops->def_domain_type(). > > The drivers that return IDENTITY have some kind of good reason, typically > that quirky hardware really can't support anything other than IDENTITY. > > Arrange things so that if the driver says it needs IDENTITY then > iommu_get_default_domain_type() either fails or returns IDENTITY. It will > not ignore the driver's override to IDENTITY. > > Split the function into two steps, reducing the group device list to the > driver's def_domain_type() and the untrusted flag. > > Then compute the result based on those two reduced variables. Fully reject > combining untrusted with IDENTITY. > > Remove the debugging print on the iommu_group_store_type() failure path, > userspace should not be able to trigger kernel prints. > > This makes the next patch cleaner that wants to force IDENTITY always for > ARM_IOMMU because there is no support for DMA. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 117 ++++++++++++++++++++++++++++-------------- > 1 file changed, 79 insertions(+), 38 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:24PM -0300, Jason Gunthorpe wrote: > What exynos calls exynos_iommu_detach_device is actually putting the iommu > into identity mode. > > Move to the new core support for ARM_DMA_USE_IOMMU by defining > ops->identity_domain. > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/exynos-iommu.c | 66 +++++++++++++++++------------------- > 1 file changed, 32 insertions(+), 34 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:25PM -0300, Jason Gunthorpe wrote: > What tegra-smmu does during tegra_smmu_set_platform_dma() is actually > putting the iommu into identity mode. > > Move to the new core support for ARM_DMA_USE_IOMMU by defining > ops->identity_domain. > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/tegra-smmu.c | 37 ++++++++++++++++++++++++++++++++----- > 1 file changed, 32 insertions(+), 5 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:26PM -0300, Jason Gunthorpe wrote: > All ARM64 iommu drivers should support IOMMU_DOMAIN_DMA to enable > dma-iommu.c. > > tegra is blocking dma-iommu usage, and also default_domain's, because it > wants an identity translation. This is needed for some device quirk. The > correct way to do this is to support IDENTITY domains and use > ops->def_domain_type() to return IOMMU_DOMAIN_IDENTITY for only the quirky > devices. > > Add support for IOMMU_DOMAIN_DMA and force IOMMU_DOMAIN_IDENTITY mode for > everything so no behavior changes. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/tegra-smmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:28PM -0300, Jason Gunthorpe wrote: > What msm does during msm_iommu_set_platform_dma() is actually putting the > iommu into identity mode. > > Move to the new core support for ARM_DMA_USE_IOMMU by defining > ops->identity_domain. > > This driver does not support IOMMU_DOMAIN_DMA, however it cannot be > compiled on ARM64 either. Most likely it is fine to support dma-iommu.c > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/msm_iommu.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:31PM -0300, Jason Gunthorpe wrote: > This brings back the ops->detach_dev() code that commit > 1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it > into an IDENTITY domain. > > Also reverts commit 584d334b1393 ("iommu/ipmmu-vmsa: Remove > ipmmu_utlb_disable()") > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/ipmmu-vmsa.c | 43 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:32PM -0300, Jason Gunthorpe wrote: > This brings back the ops->detach_dev() code that commit > 1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it > into an IDENTITY domain. > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/mtk_iommu.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:33PM -0300, Jason Gunthorpe wrote: > Prior to commit 1b932ceddd19 ("iommu: Remove detach_dev callbacks") the > sun50i_iommu_detach_device() function was being called by > ops->detach_dev(). > > This is an IDENTITY domain so convert sun50i_iommu_detach_device() into > sun50i_iommu_identity_attach() and a full IDENTITY domain and thus hook it > back up the same was as the old ops->detach_dev(). > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/sun50i-iommu.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed, Aug 23, 2023 at 01:47:37PM -0300, Jason Gunthorpe wrote: > These drivers are all trivially converted since the function is only > called if the domain type is going to be > IOMMU_DOMAIN_UNMANAGED/DMA. > > Tested-by: Heiko Stuebner <heiko@sntech.de> > Tested-by: Steven Price <steven.price@arm.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu/qcom_iommu.c | 6 ++---- > drivers/iommu/exynos-iommu.c | 7 ++----- > drivers/iommu/ipmmu-vmsa.c | 7 ++----- > drivers/iommu/mtk_iommu.c | 7 ++----- > drivers/iommu/rockchip-iommu.c | 7 ++----- > drivers/iommu/sprd-iommu.c | 7 ++----- > drivers/iommu/sun50i-iommu.c | 9 +++------ > drivers/iommu/tegra-smmu.c | 7 ++----- > 8 files changed, 17 insertions(+), 40 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>