On 2024/1/17 12:17, Tian, Kevin wrote:
From: Jason Gunthorpe <jgg@xxxxxxxxxx>
Sent: Tuesday, January 16, 2024 8:58 PM
On Tue, Jan 16, 2024 at 01:18:12AM +0000, Tian, Kevin wrote:
From: Jason Gunthorpe <jgg@xxxxxxxxxx>
Sent: Tuesday, January 16, 2024 1:25 AM
On Sun, Nov 26, 2023 at 10:34:23PM -0800, Yi Liu wrote:
+/**
+ * iommufd_device_pasid_detach - Disconnect a {device, pasid} to an
iommu_domain
+ * @idev: device to detach
+ * @pasid: pasid to detach
+ *
+ * Undo iommufd_device_pasid_attach(). This disconnects the
idev/pasid
from
+ * the previously attached pt_id.
+ */
+void iommufd_device_pasid_detach(struct iommufd_device *idev, u32
pasid)
+{
+ struct iommufd_hw_pagetable *hwpt;
+
+ hwpt = xa_load(&idev->pasid_hwpts, pasid);
+ if (!hwpt)
+ return;
+ xa_erase(&idev->pasid_hwpts, pasid);
+ iommu_detach_device_pasid(hwpt->domain, idev->dev, pasid);
+ iommufd_hw_pagetable_put(idev->ictx, hwpt);
+}
None of this xarray stuff looks locked properly
I had an impression from past discussions that the caller should not
race attach/detach/replace on same device or pasid, otherwise it is
already a problem in a higher level.
I thought that was just at the iommu layer? We want VFIO to do the
same? Then why do we need the dual xarrays?
Still, it looks really wrong to have code like this, we don't need to
- it can be locked properly, it isn't a performance path..
OK, let's add a lock for this.
and the original intention of the group lock was to ensure all devices
in the group have a same view. Not exactly to guard concurrent
attach/detach.
We don't have a group lock here, this is in iommufd.
I meant the lock in iommufd_group.
Use the xarray lock..
eg
hwpt = xa_erase(&idev->pasid_hwpts, pasid);
if (WARN_ON(!hwpt))
return
xa_erase is atomic.
yes, that's better.
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?
[1] iommufd_device_pasid_attach() -> iommufd_device_pasid_do_attach() ->
__iommufd_device_pasid_do_attach()
[2] iommufd_device_pasid_replace -> iommufd_device_pasid_do_replace ->
__iommufd_device_pasid_do_attach
Regards,
Yi Liu
