Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Tue, 12 Dec 2023 11:18:09 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Baolu Lu <baolu.lu@linux.intel.com>
Cc:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Kevin Tian <kevin.tian@intel.com>,
        Jean-Philippe Brucker <jean-philippe@linaro.org>,
        Nicolin Chen <nicolinc@nvidia.com>,
        Yi Liu <yi.l.liu@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        Longfang Liu <liulongfang@huawei.com>,
        Yan Zhao <yan.y.zhao@intel.com>, iommu@lists.linux.dev,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 12/12] iommu: Use refcount for fault data access
Message-ID: <20231212151809.GD3013885@ziepe.ca>
References: <20231207064308.313316-1-baolu.lu@linux.intel.com>
 <20231207064308.313316-13-baolu.lu@linux.intel.com>
 <20231211152456.GB1489931@ziepe.ca>
 <0f23e37a-5ace-492c-82e9-cf3d13f4ef6f@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0f23e37a-5ace-492c-82e9-cf3d13f4ef6f@linux.intel.com>
Precedence: bulk

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

> 	/* 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

Jason