Re: [PATCH v2 10/12] iommu: Make iommu_queue_iopf() more generic

[Baolu Lu]

On 2023/8/11 21:29, Jason Gunthorpe wrote:
> On Fri, Aug 11, 2023 at 10:21:20AM +0800, Baolu Lu wrote:
>
> > > This also has lifetime problems on the mm.
> > >
> > > The domain should flow into the iommu_sva_handle_iopf() instead of the
> > > void *data.
> >
> > Okay, but I still want to keep void *data as a private pointer of the
> > iopf consumer. For SVA, it's probably NULL.
>
> I'd rather give the iommu_domain some 'private' void * than pass
> around weird pointers all over the place... That might be broadly
> useful, eg iommufd could store the hwpt in there.

Yes, you are right. With the private pointer stored in domain and domain
is passed to the iopf handler, there will be no need for a @data
parameter for iopf handler.

> > > We need to document/figure out some how to ensure that the faults are
> > > all done processing before a fault enabled domain can be freed.
> >
> > This has been documented in drivers/iommu/io-pgfault.c:
> >
> > [...]
> >   * Any valid page fault will be eventually routed to an iommu domain and the
> >   * page fault handler installed there will get called. The users of this
> >   * handling framework should guarantee that the iommu domain could only be
> >   * freed after the device has stopped generating page faults (or the iommu
> >   * hardware has been set to block the page faults) and the pending page
> > faults
> >   * have been flushed.
> >   *
> >   * Return: 0 on success and <0 on error.
> >   */
> > int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
> > [...]
> >
> > > This patch would be better ordered before the prior patch.
> >
> > Let me try this in the next version.
>
> Okay.. but can we have some debugging to enforce this maybe? Also add
> a comment when we obtain the domain on this path to see the above
> about the lifetime

Yes. Sure.

Probably I will add a dev_warn() when get_domain for device or pasid
returns NULL...

In the paths of removing domain from device or pasid, I will add a check
for pending faults. Will trigger a dev_warn() if there is any pending
faults for the affected domain.

Best regards,
baolu