> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> > Sent: Monday, May 27, 2024 12:05 PM > > Add iopf-capable hw page table attach/detach/replace helpers. The pointer > to iommufd_device is stored in the domain attachment handle, so that it > can be echo'ed back in the iopf_group. this message needs an update. now the device pointer is not in the attach handle. and there worths a explanation about VF in the commit msg. > @@ -376,7 +377,10 @@ int iommufd_hw_pagetable_attach(struct > iommufd_hw_pagetable *hwpt, > * attachment. > */ > if (list_empty(&idev->igroup->device_list)) { > - rc = iommu_attach_group(hwpt->domain, idev->igroup->group); > + if (hwpt->fault) > + rc = iommufd_fault_domain_attach_dev(hwpt, idev); > + else > + rc = iommu_attach_group(hwpt->domain, idev- > >igroup->group); Does it read better to have a iommufd_attach_device() wrapper with above branches handled internally? > > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev) > +{ > + struct device *dev = idev->dev; > + int ret; > + > + /* > + * Once we turn on PCI/PRI support for VF, the response failure code > + * could not be forwarded to the hardware due to PRI being a shared you could but just doing so is incorrect. 😊 s/could/should/ > + * resource between PF and VFs. There is no coordination for this > + * shared capability. This waits for a vPRI reset to recover. > + */ this may go a bit further to talk about supporting it requires an emulation in iommufd (i.e. pause any further fault delivery until vPRI reset). It is a future work so disable it for VF at this point. > +void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable > *hwpt, > + struct iommufd_device *idev) > +{ > + struct iommufd_attach_handle *handle; > + > + handle = iommufd_device_get_attach_handle(idev); > + iommu_detach_group_handle(hwpt->domain, idev->igroup->group); > + iommufd_auto_response_faults(hwpt, handle); > + iommufd_fault_iopf_disable(idev); > + kfree(handle); this assumes that the detach callback of the iommu driver needs to drain in-fly page requests so no further reference to handle or queue new req to the deliver queue when it returns, otherwise there is a use-after-free race or stale requests in the fault queue which auto response doesn't cleanly handle. iirc previous discussion reveals that only intel-iommu driver guarantees that behavior. In any case this should be documented to avoid this being used in a non-conforming iommu driver. If I misunderstood, some comment why no race in this window is also appreciated. 😊 > +} > + > +static int __fault_domain_replace_dev(struct iommufd_device *idev, > + struct iommufd_hw_pagetable *hwpt, > + struct iommufd_hw_pagetable *old) > +{ > + struct iommufd_attach_handle *handle, *curr = NULL; > + int ret; > + > + if (old->fault) > + curr = iommufd_device_get_attach_handle(idev); > + > + if (hwpt->fault) { > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > + if (!handle) > + return -ENOMEM; > + > + handle->handle.domain = hwpt->domain; > + handle->idev = idev; > + ret = iommu_replace_group_handle(idev->igroup->group, > + hwpt->domain, &handle- > >handle); > + } else { > + ret = iommu_replace_group_handle(idev->igroup->group, > + hwpt->domain, NULL); > + } > + > + if (!ret && curr) { > + iommufd_auto_response_faults(old, curr); > + kfree(curr); > + } In last version you said auto response is required only when switching from fault-capable old to fault-incapable new. But above code doesn't reflect that description?