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
