Re: [PATCH 14/17] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE

[Robin Murphy <robin.murphy@xxxxxxx>]

On 2021-03-01 08:42, Christoph Hellwig wrote:
> Use explicit methods for setting and querying the information instead.

Now that everyone's using iommu-dma, is there any point in bouncing this 
through the drivers at all? Seems like it would make more sense for the 
x86 drivers to reflect their private options back to iommu_dma_strict 
(and allow Intel's caching mode to override it as well), then have 
iommu_dma_init_domain just test !iommu_dma_strict && 
domain->ops->flush_iotlb_all.

Robin.

> Also remove the now unused iommu_domain_get_attr functionality.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>   drivers/iommu/amd/iommu.c                   | 23 ++-------
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 47 ++++++-----------
>   drivers/iommu/arm/arm-smmu/arm-smmu.c       | 56 +++++----------------
>   drivers/iommu/dma-iommu.c                   |  8 ++-
>   drivers/iommu/intel/iommu.c                 | 27 ++--------
>   drivers/iommu/iommu.c                       | 19 +++----
>   include/linux/iommu.h                       | 17 ++-----
>   7 files changed, 51 insertions(+), 146 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index a69a8b573e40d0..37a8e51db17656 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1771,24 +1771,11 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>   	return acpihid_device_group(dev);
>   }
>   
> -static int amd_iommu_domain_get_attr(struct iommu_domain *domain,
> -		enum iommu_attr attr, void *data)
> +static bool amd_iommu_dma_use_flush_queue(struct iommu_domain *domain)
>   {
> -	switch (domain->type) {
> -	case IOMMU_DOMAIN_UNMANAGED:
> -		return -ENODEV;
> -	case IOMMU_DOMAIN_DMA:
> -		switch (attr) {
> -		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> -			*(int *)data = !amd_iommu_unmap_flush;
> -			return 0;
> -		default:
> -			return -ENODEV;
> -		}
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	if (domain->type != IOMMU_DOMAIN_DMA)
> +		return false;
> +	return !amd_iommu_unmap_flush;
>   }
>   
>   /*****************************************************************************
> @@ -2257,7 +2244,7 @@ const struct iommu_ops amd_iommu_ops = {
>   	.release_device = amd_iommu_release_device,
>   	.probe_finalize = amd_iommu_probe_finalize,
>   	.device_group = amd_iommu_device_group,
> -	.domain_get_attr = amd_iommu_domain_get_attr,
> +	.dma_use_flush_queue = amd_iommu_dma_use_flush_queue,
>   	.get_resv_regions = amd_iommu_get_resv_regions,
>   	.put_resv_regions = generic_iommu_put_resv_regions,
>   	.is_attach_deferred = amd_iommu_is_attach_deferred,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 8594b4a8304375..bf96172e8c1f71 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2449,33 +2449,21 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
>   	return group;
>   }
>   
> -static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> -				    enum iommu_attr attr, void *data)
> +static bool arm_smmu_dma_use_flush_queue(struct iommu_domain *domain)
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   
> -	switch (domain->type) {
> -	case IOMMU_DOMAIN_UNMANAGED:
> -		switch (attr) {
> -		case DOMAIN_ATTR_NESTING:
> -			*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> -			return 0;
> -		default:
> -			return -ENODEV;
> -		}
> -		break;
> -	case IOMMU_DOMAIN_DMA:
> -		switch (attr) {
> -		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> -			*(int *)data = smmu_domain->non_strict;
> -			return 0;
> -		default:
> -			return -ENODEV;
> -		}
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	if (domain->type != IOMMU_DOMAIN_DMA)
> +		return false;
> +	return smmu_domain->non_strict;
> +}
> +
> +
> +static void arm_smmu_dma_enable_flush_queue(struct iommu_domain *domain)
> +{
> +	if (domain->type != IOMMU_DOMAIN_DMA)
> +		return;
> +	to_smmu_domain(domain)->non_strict = true;
>   }
>   
>   static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> @@ -2505,13 +2493,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
>   		}
>   		break;
>   	case IOMMU_DOMAIN_DMA:
> -		switch(attr) {
> -		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> -			smmu_domain->non_strict = *(int *)data;
> -			break;
> -		default:
> -			ret = -ENODEV;
> -		}
> +		ret = -ENODEV;
>   		break;
>   	default:
>   		ret = -EINVAL;
> @@ -2619,7 +2601,8 @@ static struct iommu_ops arm_smmu_ops = {
>   	.probe_device		= arm_smmu_probe_device,
>   	.release_device		= arm_smmu_release_device,
>   	.device_group		= arm_smmu_device_group,
> -	.domain_get_attr	= arm_smmu_domain_get_attr,
> +	.dma_use_flush_queue	= arm_smmu_dma_use_flush_queue,
> +	.dma_enable_flush_queue	= arm_smmu_dma_enable_flush_queue,
>   	.domain_set_attr	= arm_smmu_domain_set_attr,
>   	.of_xlate		= arm_smmu_of_xlate,
>   	.get_resv_regions	= arm_smmu_get_resv_regions,
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d8c6bfde6a6158..e7893e96f5177a 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1481,42 +1481,20 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
>   	return group;
>   }
>   
> -static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> -				    enum iommu_attr attr, void *data)
> +static bool arm_smmu_dma_use_flush_queue(struct iommu_domain *domain)
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   
> -	switch(domain->type) {
> -	case IOMMU_DOMAIN_UNMANAGED:
> -		switch (attr) {
> -		case DOMAIN_ATTR_NESTING:
> -			*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> -			return 0;
> -		case DOMAIN_ATTR_IO_PGTABLE_CFG: {
> -			struct io_pgtable_domain_attr *pgtbl_cfg = data;
> -			*pgtbl_cfg = smmu_domain->pgtbl_cfg;
> +	if (domain->type != IOMMU_DOMAIN_DMA)
> +		return false;
> +	return smmu_domain->pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT;
> +}
>   
> -			return 0;
> -		}
> -		default:
> -			return -ENODEV;
> -		}
> -		break;
> -	case IOMMU_DOMAIN_DMA:
> -		switch (attr) {
> -		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: {
> -			bool non_strict = smmu_domain->pgtbl_cfg.quirks &
> -					  IO_PGTABLE_QUIRK_NON_STRICT;
> -			*(int *)data = non_strict;
> -			return 0;
> -		}
> -		default:
> -			return -ENODEV;
> -		}
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +static void arm_smmu_dma_enable_flush_queue(struct iommu_domain *domain)
> +{
> +	if (domain->type != IOMMU_DOMAIN_DMA)
> +		return;
> +	to_smmu_domain(domain)->pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
>   }
>   
>   static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> @@ -1557,16 +1535,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
>   		}
>   		break;
>   	case IOMMU_DOMAIN_DMA:
> -		switch (attr) {
> -		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> -			if (*(int *)data)
> -				smmu_domain->pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> -			else
> -				smmu_domain->pgtbl_cfg.quirks &= ~IO_PGTABLE_QUIRK_NON_STRICT;
> -			break;
> -		default:
> -			ret = -ENODEV;
> -		}
> +		ret = -ENODEV;
>   		break;
>   	default:
>   		ret = -EINVAL;
> @@ -1631,7 +1600,8 @@ static struct iommu_ops arm_smmu_ops = {
>   	.probe_device		= arm_smmu_probe_device,
>   	.release_device		= arm_smmu_release_device,
>   	.device_group		= arm_smmu_device_group,
> -	.domain_get_attr	= arm_smmu_domain_get_attr,
> +	.dma_use_flush_queue	= arm_smmu_dma_use_flush_queue,
> +	.dma_enable_flush_queue	= arm_smmu_dma_enable_flush_queue,
>   	.domain_set_attr	= arm_smmu_domain_set_attr,
>   	.of_xlate		= arm_smmu_of_xlate,
>   	.get_resv_regions	= arm_smmu_get_resv_regions,
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9ab6ee22c11088..d3fe5aad9d6ecf 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -305,8 +305,8 @@ static void iommu_dma_flush_iotlb_all(struct iova_domain *iovad)
>   	cookie = container_of(iovad, struct iommu_dma_cookie, iovad);
>   	domain = cookie->fq_domain;
>   	/*
> -	 * The IOMMU driver supporting DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
> -	 * implies that ops->flush_iotlb_all must be non-NULL.
> +	 * The IOMMU driver supporting a DMA flush queue implies that
> +	 * ops->flush_iotlb_all must be non-NULL.
>   	 */
>   	domain->ops->flush_iotlb_all(domain);
>   }
> @@ -329,7 +329,6 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>   	unsigned long order, base_pfn;
>   	struct iova_domain *iovad;
> -	int attr;
>   
>   	if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE)
>   		return -EINVAL;
> @@ -365,8 +364,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>   
>   	init_iova_domain(iovad, 1UL << order, base_pfn);
>   
> -	if (!cookie->fq_domain && !iommu_domain_get_attr(domain,
> -			DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, &attr) && attr) {
> +	if (!cookie->fq_domain && iommu_dma_use_flush_queue(domain)) {
>   		if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
>   					  iommu_dma_entry_dtor))
>   			pr_warn("iova flush queue initialization failed\n");
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index ee0932307d646b..eaa80c33f4bc91 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5453,13 +5453,13 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
>   	return ret;
>   }
>   
> -static bool domain_use_flush_queue(void)
> +static bool intel_iommu_dma_use_flush_queue(struct iommu_domain *domain)
>   {
>   	struct dmar_drhd_unit *drhd;
>   	struct intel_iommu *iommu;
>   	bool r = true;
>   
> -	if (intel_iommu_strict)
> +	if (domain->type != IOMMU_DOMAIN_DMA || intel_iommu_strict)
>   		return false;
>   
>   	/*
> @@ -5483,27 +5483,6 @@ static bool domain_use_flush_queue(void)
>   	return r;
>   }
>   
> -static int
> -intel_iommu_domain_get_attr(struct iommu_domain *domain,
> -			    enum iommu_attr attr, void *data)
> -{
> -	switch (domain->type) {
> -	case IOMMU_DOMAIN_UNMANAGED:
> -		return -ENODEV;
> -	case IOMMU_DOMAIN_DMA:
> -		switch (attr) {
> -		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> -			*(int *)data = domain_use_flush_queue();
> -			return 0;
> -		default:
> -			return -ENODEV;
> -		}
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -
>   /*
>    * Check that the device does not live on an external facing PCI port that is
>    * marked as untrusted. Such devices should not be able to apply quirks and
> @@ -5576,7 +5555,7 @@ const struct iommu_ops intel_iommu_ops = {
>   	.capable		= intel_iommu_capable,
>   	.domain_alloc		= intel_iommu_domain_alloc,
>   	.domain_free		= intel_iommu_domain_free,
> -	.domain_get_attr        = intel_iommu_domain_get_attr,
> +	.dma_use_flush_queue	= intel_iommu_dma_use_flush_queue,
>   	.domain_set_attr	= intel_iommu_domain_set_attr,
>   	.attach_dev		= intel_iommu_attach_device,
>   	.detach_dev		= intel_iommu_detach_device,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 23daaea7883b75..0f12c4d58cdc42 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1512,12 +1512,8 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus,
>   	if (!group->domain)
>   		group->domain = dom;
>   
> -	if (!iommu_dma_strict) {
> -		int attr = 1;
> -		iommu_domain_set_attr(dom,
> -				      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
> -				      &attr);
> -	}
> +	if (!iommu_dma_strict && dom->ops->dma_enable_flush_queue)
> +		dom->ops->dma_enable_flush_queue(dom);
>   
>   	return 0;
>   }
> @@ -2664,14 +2660,13 @@ static int __init iommu_init(void)
>   }
>   core_initcall(iommu_init);
>   
> -int iommu_domain_get_attr(struct iommu_domain *domain,
> -			  enum iommu_attr attr, void *data)
> +bool iommu_dma_use_flush_queue(struct iommu_domain *domain)
>   {
> -	if (!domain->ops->domain_get_attr)
> -		return -EINVAL;
> -	return domain->ops->domain_get_attr(domain, attr, data);
> +	if (!domain->ops->dma_use_flush_queue)
> +		return false;
> +	return domain->ops->dma_use_flush_queue(domain);
>   }
> -EXPORT_SYMBOL_GPL(iommu_domain_get_attr);
> +EXPORT_SYMBOL_GPL(iommu_dma_use_flush_queue);
>   
>   int iommu_domain_set_attr(struct iommu_domain *domain,
>   			  enum iommu_attr attr, void *data)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c15a8658daad64..f30de33c6ff56e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -108,7 +108,6 @@ enum iommu_cap {
>   
>   enum iommu_attr {
>   	DOMAIN_ATTR_NESTING,	/* two stages of translation */
> -	DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
>   	DOMAIN_ATTR_IO_PGTABLE_CFG,
>   	DOMAIN_ATTR_MAX,
>   };
> @@ -194,7 +193,8 @@ struct iommu_iotlb_gather {
>    * @probe_finalize: Do final setup work after the device is added to an IOMMU
>    *                  group and attached to the groups domain
>    * @device_group: find iommu group for a particular device
> - * @domain_get_attr: Query domain attributes
> + * @dma_use_flush_queue: Returns %true if a DMA flush queue is used
> + * @dma_enable_flush_queue: Try to enable the DMA flush queue
>    * @domain_set_attr: Change domain attributes
>    * @get_resv_regions: Request list of reserved regions for a device
>    * @put_resv_regions: Free list of reserved regions for a device
> @@ -244,8 +244,8 @@ struct iommu_ops {
>   	void (*release_device)(struct device *dev);
>   	void (*probe_finalize)(struct device *dev);
>   	struct iommu_group *(*device_group)(struct device *dev);
> -	int (*domain_get_attr)(struct iommu_domain *domain,
> -			       enum iommu_attr attr, void *data);
> +	bool (*dma_use_flush_queue)(struct iommu_domain *domain);
> +	void (*dma_enable_flush_queue)(struct iommu_domain *domain);
>   	int (*domain_set_attr)(struct iommu_domain *domain,
>   			       enum iommu_attr attr, void *data);
>   
> @@ -491,8 +491,7 @@ extern int iommu_page_response(struct device *dev,
>   extern int iommu_group_id(struct iommu_group *group);
>   extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
>   
> -extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
> -				 void *data);
> +bool iommu_dma_use_flush_queue(struct iommu_domain *domain);
>   extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
>   				 void *data);
>   
> @@ -861,12 +860,6 @@ static inline int iommu_group_id(struct iommu_group *group)
>   	return -ENODEV;
>   }
>   
> -static inline int iommu_domain_get_attr(struct iommu_domain *domain,
> -					enum iommu_attr attr, void *data)
> -{
> -	return -EINVAL;
> -}
> -
>   static inline int iommu_domain_set_attr(struct iommu_domain *domain,
>   					enum iommu_attr attr, void *data)
>   {