RE: [PATCH v5 12/12] iommu: Improve iopf_queue_flush_dev()

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Tuesday, September 26, 2023 9:49 AM
> 
> On 9/25/23 3:00 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> 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.