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);