Re: [PATCH 3/8] iommufd: Support attach/replace hwpt per pasid

[Yi Liu]

On 2024/1/17 20:56, Jason Gunthorpe wrote:
> On Wed, Jan 17, 2024 at 04:24:24PM +0800, Yi Liu wrote:
> > Above indeed makes more sense if there can be concurrent attach/replace/detach
> > on a single pasid. Just have one doubt should we add lock to protect the
> > whole attach/replace/detach paths. In the attach/replace path[1] [2], the
> > xarray entry is verified firstly, and then updated after returning from
> > iommu attach/replace API. It is uneasy to protect the xarray operations only
> > with xa_lock as a detach path can acquire xa_lock right after attach/replace
> > path checks the xarray. To avoid it, may need a mutex to protect the whole
> > attach/replace/detach path to avoid race. Or maybe the attach/replace path
> > should mark the corresponding entry as a special state that can block the
> > other path like detach until the attach/replace path update the final hwpt to
> > the xarray. Is there such state in xarray?
>
> If the caller is not allowed to make concurrent attaches/detaches to
> the same pasid then you can document that in a comment,

yes. I can document it. Otherwise, we may need a mutex for pasid to allow
concurrent attaches/detaches.

> but it is
> still better to use xarray in a self-consistent way.

sure. I'll try. At least in the detach path, xarray should be what you've
suggested in prior email. Currently in the attach path, the logic is as
below. Perhaps I can skip the check on old_hwpt since
iommu_attach_device_pasid() should fail if there is an existing domain
attached on the pasid. Then the xarray should be more consistent. what
about your opinion?

	old_hwpt = xa_load(&idev->pasid_hwpts, pasid);
	if (old_hwpt) {
		/* Attach does not allow overwrite */
		if (old_hwpt == hwpt)
			return NULL;
		else
			return ERR_PTR(-EINVAL);
	}

	rc = iommu_attach_device_pasid(hwpt->domain, idev->dev, pasid);
	if (rc)
		return ERR_PTR(rc);

	refcount_inc(&hwpt->obj.users);
	xa_store(&idev->pasid_hwpts, pasid, hwpt, GFP_KERNEL);

--
Regards,
Yi Liu