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
