On Wed, Oct 23, 2024 at 01:59:05PM -0300, Jason Gunthorpe wrote:
[Comparison] | This v1 | Draft
- Adds to master | A lock and vdev ptr | A lock and viommu ptr
- Set/unset ptr | In ->vdevice_alloc/free | In all ->attach_dev
- Do dev_to_vdev | master->vdev->id | attach_handle?
The set/unset ops have the major issue that they can get out of sync with the domain. The only time things should be routed to the viommu is when a viommu related domain is attached.
Ah, I missed that point.
The lock on attach can be reduced:
iommu_group_mutex_assert(dev) if (domain->type == IOMMU_DOMAIN_NESTED) new_vsmmu = to_smmu_domain(domain)->vsmmu; else new_vsmmu = NULL; if (new_vsmmu != master->vsmmu) { down_write(&master->lock); master->vsmmu = new_vsmmu; up_write(&master->lock); }
And you'd stick this in prepare or commit..
Ack.
Both solutions needs a driver-level lock and an extra pointer in the master structure. And both ISR routines require that driver- level lock to avoid race against attach_dev v.s. vdev alloc/free. Overall, taking step.3 into consideration, it seems that letting master lock&hold the vdev pointer (i.e. this v1) is simpler?
I'm not sure the vdev pointer should even be visible to the drivers..
With the iommufd_vdevice_alloc allocator, we already expose the vdevice structure to the drivers, along with the vdevice_alloc vdevice_free ops, which would be easier for the vCMDQ driver to allocate and hold its own pSID/vSID structure.
And vsid_to_psid() requires to look up the viommu->vdevs xarray. If we hid the vDEVICE structure, we'd need something else than the vdev_to_dev(). Maybe iommufd_viommu_find_dev_by_virt_id()?
As for the implementation of iommufd_viommu_dev_to_vdev(), I read the attach_handle part in the PRI code, yet I don't see the lock that protects the handle returned by iommu_attach_handle_get() in iommu_report_device_fault/find_fault_handler().
It is the xa_lock and some rules about flushing irqs and work queues before allowing the dev to disappear:
"Callers are required to synchronize the call of iommu_attach_handle_get() with domain attachment and detachment. The attach handle can only be used during its life cycle."
But the caller iommu_report_device_fault() is an async event that cannot guarantee the lifecycle. Would you please shed some light?
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) | ??
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?
Then perhaps we can have a struct arm_smmu_vstream to hold all the things, such as vsmmu pointer and vdev->id. If vCMDQ needs an extra structure, it can be stored there too.
Thanks! Nicolin