Re: [PATCH v8 12/12] iommu: Use refcount for fault data access

Baolu Lu
2023-12-13T02:24:35Z

On 12/12/23 11:18 PM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 01:07:17PM +0800, Baolu Lu wrote:
>
>> Yes, agreed. The iopf_fault_param should be passed in together with the
>> iopf_group. The reference count should be released in the
>> iopf_free_group(). These two helps could look like below:
>>
>> int iommu_page_response(struct iopf_group *group,
>> struct iommu_page_response *msg)
>> {
>> bool needs_pasid;
>> int ret = -EINVAL;
>> struct iopf_fault *evt;
>> struct iommu_fault_page_request *prm;
>> struct device *dev = group->fault_param->dev;
>> const struct iommu_ops *ops = dev_iommu_ops(dev);
>> bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
>> struct iommu_fault_param *fault_param = group->fault_param;
>>
>> if (!ops->page_response)
>> return -ENODEV;
>
> We should never get here if this is the case, prevent the device from
> being added in the first place

Yeah, could move it to iopf_queue_add_device(). WARN and return failure
there if the driver is not ready for page request handling.

>
>> /* 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");
>> goto done_unlock;
>> }
>> /*
>> * Check if we have a matching page request pending to respond,
>> * otherwise return -EINVAL
>> */
>> list_for_each_entry(evt, &fault_param->faults, list) {
>> prm = &evt->fault.prm;
>> if (prm->grpid != msg->grpid)
>> continue;
>>
>> /*
>> * If the PASID is required, the corresponding request is
>> * matched using the group ID, the PASID valid bit and the PASID
>> * value. Otherwise only the group ID matches request and
>> * response.
>> */
>> needs_pasid = prm->flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
>> if (needs_pasid && (!has_pasid || msg->pasid != prm->pasid))
>> continue;
>>
>> if (!needs_pasid && has_pasid) {
>> /* No big deal, just clear it. */
>> msg->flags &= ~IOMMU_PAGE_RESP_PASID_VALID;
>> msg->pasid = 0;
>> }
>>
>> ret = ops->page_response(dev, evt, msg);
>> list_del(&evt->list);
>> kfree(evt);
>> break;
>> }
>>
>> done_unlock:
>> mutex_unlock(&fault_param->lock);
>
> I would have expected the group to free'd here? But regardless this
> looks like a good direction

Both work for me. We can decide it according to the needs of code later.

>
> Jason

Best regards,
baolu