RE: [PATCH v4 09/10] iommu: Make iommu_queue_iopf() more generic

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Tuesday, September 5, 2023 1:20 PM
> 
> I added below patch to address the iopf_queue_flush_dev() issue. What do
> you think of this？
> 
> iommu: Improve iopf_queue_flush_dev()
> 
> The iopf_queue_flush_dev() is called by the iommu driver before releasing
> a PASID. It ensures that all pending faults for this PASID have been
> handled or cancelled, and won't hit the address space that reuses this
> PASID. The driver must make sure that no new fault is added to the queue.
> 
> The SMMUv3 driver doesn't use it because it only implements the
> Arm-specific stall fault model where DMA transactions are held in the SMMU
> while waiting for the OS to handle iopf's. Since a device driver must
> complete all DMA transactions before detaching domain, there are no
> pending iopf's with the stall model. PRI support requires adding a call to
> iopf_queue_flush_dev() after flushing the hardware page fault queue.
> 
> The current implementation of iopf_queue_flush_dev() is a simplified
> version. It is only suitable for SVA case in which the processing of iopf
> is implemented in the inner loop of the iommu subsystem.
> 
> Improve this interface to make it also work for handling iopf out of the
> iommu core.
> 
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>   include/linux/iommu.h      |  4 ++--
>   drivers/iommu/intel/svm.c  |  2 +-
>   drivers/iommu/io-pgfault.c | 40
> ++++++++++++++++++++++++++++++++++++--
>   3 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 77ad33ffe3ac..465e23e945d0 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1275,7 +1275,7 @@ iommu_sva_domain_alloc(struct device *dev,
> struct
> mm_struct *mm)
>   #ifdef CONFIG_IOMMU_IOPF
>   int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev);
>   int iopf_queue_remove_device(struct iopf_queue *queue, struct device
> *dev);
> -int iopf_queue_flush_dev(struct device *dev);
> +int iopf_queue_flush_dev(struct device *dev, ioasid_t pasid);
>   struct iopf_queue *iopf_queue_alloc(const char *name);
>   void iopf_queue_free(struct iopf_queue *queue);
>   int iopf_queue_discard_partial(struct iopf_queue *queue);
> @@ -1295,7 +1295,7 @@ iopf_queue_remove_device(struct iopf_queue
> *queue,
> struct device *dev)
>   	return -ENODEV;
>   }
> 
> -static inline int iopf_queue_flush_dev(struct device *dev)
> +static inline int iopf_queue_flush_dev(struct device *dev, ioasid_t pasid)
>   {
>   	return -ENODEV;
>   }
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 780c5bd73ec2..4c3f4533e337 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -495,7 +495,7 @@ void intel_drain_pasid_prq(struct device *dev, u32
> pasid)
>   		goto prq_retry;
>   	}
> 
> -	iopf_queue_flush_dev(dev);
> +	iopf_queue_flush_dev(dev, pasid);
> 
>   	/*
>   	 * Perform steps described in VT-d spec CH7.10 to drain page
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 3e6845bc5902..84728fb89ac7 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -309,17 +309,53 @@ EXPORT_SYMBOL_GPL(iommu_page_response);
>    *
>    * Return: 0 on success and <0 on error.
>    */
> -int iopf_queue_flush_dev(struct device *dev)
> +int iopf_queue_flush_dev(struct device *dev, ioasid_t pasid)
>   {
>   	struct iommu_fault_param *iopf_param =
> iopf_get_dev_fault_param(dev);
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	struct iommu_page_response resp;
> +	struct iopf_fault *iopf, *next;
> +	int ret = 0;
> 
>   	if (!iopf_param)
>   		return -ENODEV;
> 
>   	flush_workqueue(iopf_param->queue->wq);
> +
> +	mutex_lock(&iopf_param->lock);
> +	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
> +		if (!(iopf->fault.prm.flags &
> IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ||
> +		    iopf->fault.prm.pasid != pasid)
> +			break;
> +
> +		list_del(&iopf->list);
> +		kfree(iopf);
> +	}
> +
> +	list_for_each_entry_safe(iopf, next, &iopf_param->faults, list) {
> +		if (!(iopf->fault.prm.flags &
> IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ||
> +		    iopf->fault.prm.pasid != pasid)
> +			continue;
> +
> +		memset(&resp, 0, sizeof(struct iommu_page_response));
> +		resp.pasid = iopf->fault.prm.pasid;
> +		resp.grpid = iopf->fault.prm.grpid;
> +		resp.code = IOMMU_PAGE_RESP_INVALID;
> +
> +		if (iopf->fault.prm.flags &
> IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID)
> +			resp.flags = IOMMU_PAGE_RESP_PASID_VALID;

Out of curiosity. Is it a valid configuration which has REQUEST_PASID_VALID
set but RESP_PASID_VALID cleared? I'm unclear why another response
flag is required beyond what the request flag has told...

> +
> +		ret = ops->page_response(dev, iopf, &resp);
> +		if (ret)
> +			break;
> +
> +		list_del(&iopf->list);
> +		kfree(iopf);
> +	}
> +	mutex_unlock(&iopf_param->lock);
>   	iopf_put_dev_fault_param(iopf_param);
> 
> -	return 0;
> +	return ret;
>   }
>   EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
> 

This looks OK. Another nit is that the warning of "no pending PRQ"
in iommu_page_response() should be removed given with above
change it's expected for iommufd responses to be received after this
flush operation in iommu core.