Re: [PATCH v6 02/25] iommu: Add IOMMU_DOMAIN_PLATFORM

Baolu Lu
2023-08-12T01:36:49Z

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