> 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.
