On Sat, Apr 06, 2024 at 12:34:14PM +0800, Baolu Lu wrote:
> On 4/3/24 7:58 PM, Jason Gunthorpe wrote:
> > On Wed, Apr 03, 2024 at 09:15:11AM +0800, Lu Baolu wrote:
> > > Currently, when attaching a domain to a device or its PASID, domain is
> > > stored within the iommu group. It could be retrieved for use during the
> > > window between attachment and detachment.
> > >
> > > With new features introduced, there's a need to store more information
> > > than just a domain pointer. This information essentially represents the
> > > association between a domain and a device. For example, the SVA code
> > > already has a custom struct iommu_sva which represents a bond between
> > > sva domain and a PASID of a device. Looking forward, the IOMMUFD needs
> > > a place to store the iommufd_device pointer in the core, so that the
> > > device object ID could be quickly retrieved in the critical fault handling
> > > path.
> > >
> > > Introduce domain attachment handle that explicitly represents the
> > > attachment relationship between a domain and a device or its PASID.
> > > A caller-specific data field can be used by the caller to store additional
> > > information beyond a domain pointer, depending on its specific use case.
> > >
> > > Co-developed-by: Jason Gunthorpe<jgg@xxxxxxxxxx>
> > > Signed-off-by: Jason Gunthorpe<jgg@xxxxxxxxxx>
> > > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/iommu/iommu-priv.h | 9 +++
> > > drivers/iommu/iommu.c | 158 +++++++++++++++++++++++++++++++++----
> > > 2 files changed, 153 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
> > > index 5f731d994803..08c0667cef54 100644
> > > --- a/drivers/iommu/iommu-priv.h
> > > +++ b/drivers/iommu/iommu-priv.h
> > > @@ -28,4 +28,13 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
> > > const struct bus_type *bus,
> > > struct notifier_block *nb);
> > > +struct iommu_attach_handle {
> > > + struct iommu_domain *domain;
> > > + refcount_t users;
> > I don't understand how the refcounting can be generally useful. There
> > is no way to free this:
> >
> > > + void *priv;
> > When the refcount goes to zero.
>
> This field is set by the caller, so the caller ensures that the pointer
> can only be freed after iommu domain detachment. For iopf, the caller
> should automatically respond to all outstanding iopf's in the domain
> detach path.
>
> In the sva case, which uses the workqueue to handle iopf,
> flush_workqueue() is called in the domain detach path to ensure that all
> outstanding iopf's are completed before detach completion.
Which is back to what is the point of the refcount at all?
> +static void iommufd_auto_response_handle(struct iommufd_fault *fault,
> + struct iommu_attach_handle *handle)
> +{
> + struct iommufd_device *idev = handle->priv;
The caller already has the iommufd_device, don't need the handler.
> + struct iopf_group *group;
> + unsigned long index;
> +
> + mutex_lock(&fault->mutex);
> + xa_for_each(&idev->faults, index, group) {
> + xa_erase(&idev->faults, index);
> + iopf_group_response(group, IOMMU_PAGE_RESP_INVALID);
> + }
> + mutex_unlock(&fault->mutex);
This makes sense, yes..
> void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
> struct iommufd_device *idev)
> {
> + struct iommufd_fault *fault = hwpt->fault;
> + struct iommu_attach_handle *handle;
> +
> if (WARN_ON(!hwpt->fault_capable))
> return;
>
> + handle = iommu_attach_handle_get(idev->igroup->group,
> IOMMU_NO_PASID);
> iommu_detach_group(hwpt->domain, idev->igroup->group);
> iommufd_fault_iopf_disable(idev);
But is this right? Couldn't there be PASID's doing PRI?
Jason
