diff mbox series

[v5,12/12] iommu: Improve iopf_queue_flush_dev()

Message ID 20230914085638.17307-13-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Prepare to deliver page faults to user space |

Commit Message

Baolu Lu Sept. 14, 2023, 8:56 a.m. UTC 
  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. Remove a warning message in iommu_page_response() since the
iopf queue might get flushed before possible pending responses.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      |  4 ++--
 drivers/iommu/intel/svm.c  |  2 +-
 drivers/iommu/io-pgfault.c | 46 +++++++++++++++++++++++++++++++++-----
 3 files changed, 44 insertions(+), 8 deletions(-)

Comments

Tian, Kevin Sept. 25, 2023, 7 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, September 14, 2023 4:57 PM
> @@ -300,6 +299,7 @@ EXPORT_SYMBOL_GPL(iommu_page_response);
>  /**
>   * iopf_queue_flush_dev - Ensure that all queued faults have been
> processed
>   * @dev: the endpoint whose faults need to be flushed.
> + * @pasid: the PASID of the endpoint.
>   *
>   * The IOMMU driver calls this before releasing a PASID, to ensure that all
>   * pending faults for this PASID have been handled, and won't hit the
> address

the comment should be updated too.

> @@ -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)

iopf_queue_flush_dev_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;
> +
> +		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;
>  }

Is it more accurate to call this function as iopf_queue_drop_dev_pasid()?
The added logic essentially implies that the caller doesn't care about
responses and all the in-fly states are either flushed (request) or
abandoned (response).

A normal flush() helper usually means just the flush action. If there is
a need to wait for responses after flush then we could add a
flush_dev_pasid_wait_timeout() later when there is a demand...
Baolu Lu Sept. 26, 2023, 1:49 a.m. UTC | #2 
On 9/25/23 3:00 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, September 14, 2023 4:57 PM
>> @@ -300,6 +299,7 @@ EXPORT_SYMBOL_GPL(iommu_page_response);
>>   /**
>>    * iopf_queue_flush_dev - Ensure that all queued faults have been
>> processed
>>    * @dev: the endpoint whose faults need to be flushed.
>> + * @pasid: the PASID of the endpoint.
>>    *
>>    * The IOMMU driver calls this before releasing a PASID, to ensure that all
>>    * pending faults for this PASID have been handled, and won't hit the
>> address
> 
> the comment should be updated too.

Yes.

     ... pending faults for this PASID have been handled or dropped ...

> 
>> @@ -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)
> 
> iopf_queue_flush_dev_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;
>> +
>> +		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;
>>   }
> 
> Is it more accurate to call this function as iopf_queue_drop_dev_pasid()?
> The added logic essentially implies that the caller doesn't care about
> responses and all the in-fly states are either flushed (request) or
> abandoned (response).
> 
> A normal flush() helper usually means just the flush action. If there is
> a need to wait for responses after flush then we could add a
> flush_dev_pasid_wait_timeout() later when there is a demand...

Fair enough.

As my understanding, "flush" means "handling the pending i/o page faults
immediately and wait until everything is done". Here what the caller
wants is "I have completed using this pasid, discard all the pending
requests by responding an INVALID result so that this PASID could be
reused".

If this holds, how about iopf_queue_discard_dev_pasid()? It matches the
existing iopf_queue_discard_partial().

Best regards,
baolu
Tian, Kevin Sept. 26, 2023, 2 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, September 26, 2023 9:49 AM
> 
> On 9/25/23 3:00 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Thursday, September 14, 2023 4:57 PM
> >> @@ -300,6 +299,7 @@ EXPORT_SYMBOL_GPL(iommu_page_response);
> >>   /**
> >>    * iopf_queue_flush_dev - Ensure that all queued faults have been
> >> processed
> >>    * @dev: the endpoint whose faults need to be flushed.
> >> + * @pasid: the PASID of the endpoint.
> >>    *
> >>    * The IOMMU driver calls this before releasing a PASID, to ensure that all
> >>    * pending faults for this PASID have been handled, and won't hit the
> >> address
> >
> > the comment should be updated too.
> 
> Yes.
> 
>      ... pending faults for this PASID have been handled or dropped ...
> 
> >
> >> @@ -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)
> >
> > iopf_queue_flush_dev_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;
> >> +
> >> +		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;
> >>   }
> >
> > Is it more accurate to call this function as iopf_queue_drop_dev_pasid()?
> > The added logic essentially implies that the caller doesn't care about
> > responses and all the in-fly states are either flushed (request) or
> > abandoned (response).
> >
> > A normal flush() helper usually means just the flush action. If there is
> > a need to wait for responses after flush then we could add a
> > flush_dev_pasid_wait_timeout() later when there is a demand...
> 
> Fair enough.
> 
> As my understanding, "flush" means "handling the pending i/o page faults
> immediately and wait until everything is done". Here what the caller
> wants is "I have completed using this pasid, discard all the pending
> requests by responding an INVALID result so that this PASID could be
> reused".
> 
> If this holds, how about iopf_queue_discard_dev_pasid()? It matches the
> existing iopf_queue_discard_partial().
> 

yes. 'discard' sounds better.
diff mbox series

Patch

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..8d81688f715d 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -254,10 +254,9 @@  int iommu_page_response(struct device *dev,
 
 	/* Only send response if there is a fault report pending */
 	mutex_lock(&fault_param->lock);
-	if (list_empty(&fault_param->faults)) {
-		dev_warn_ratelimited(dev, "no pending PRQ, drop response\n");
+	if (list_empty(&fault_param->faults))
 		goto done_unlock;
-	}
+
 	/*
 	 * Check if we have a matching page request pending to respond,
 	 * otherwise return -EINVAL
@@ -300,6 +299,7 @@  EXPORT_SYMBOL_GPL(iommu_page_response);
 /**
  * iopf_queue_flush_dev - Ensure that all queued faults have been processed
  * @dev: the endpoint whose faults need to be flushed.
+ * @pasid: the PASID of the endpoint.
  *
  * The IOMMU driver calls this before releasing a PASID, to ensure that all
  * pending faults for this PASID have been handled, and won't hit the address
@@ -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;
+
+		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);