On Wed, Oct 23, 2024 at 11:54:54AM -0700, Nicolin Chen wrote:
The iopf detatch function will act as a barrirer to ensure that all the async work has completed, sort of like how RCU works.
The xa_lock(&group->pasid_array) is released once the handle is returned to the iommu_attach_handle_get callers, so it protects only for a very short window (T0 below). What if: | detach() | isr=>iommu_report_device_fault() T0 | Get attach_handle [xa_lock] | Get attach_handle [xa_lock] T1 | Clean deliver Q [fault->mutex] | Waiting for fault->mutex T2 | iommufd_eventq_iopf_disable() | Add new fault to the deliver Q T3 | kfree(handle) | ??
Prior to iommufd_eventq_iopf_disable() the driver has to ensure the threads calling isr->iommu_report_device_fault() are fenced.
New threads that start running cannot see the attach_handle() because it is not in the xarray anymore. Old threads are completed because of the fence.
But here, I think it is pretty simple, isn't it?
When you update the master->vsmmu you can query the vsmmu to get the vdev id of that master, then store it in the master struct and forward it to the iommufd_viommu_report_irq(). That could even search the xarray since attach is not a performance path.
Then it is locked under the master->lock
Yes! I didn't see that coming. vdev->id must be set before the attach to a nested domain, and can be cleaned after the device detaches. Maybe an attach to vIOMMU-based nested domain should just fail if idev->vdev isn't ready?
That would make sense
Jason