On Fri, Jun 23, 2023 at 11:15:40AM -0300, Jason Gunthorpe wrote:
> > +static void __iommufd_access_detach(struct iommufd_access *access)
> > +{
> > + struct iommufd_ioas *cur_ioas = access->ioas;
> > +
> > + lockdep_assert_held(&access->ioas_lock);
> > + /*
> > + * Set ioas to NULL to block any further iommufd_access_pin_pages().
> > + * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
> > + */
> > + access->ioas = NULL;
> > +
> > + if (access->ops->unmap) {
> > + mutex_unlock(&access->ioas_lock);
> > + access->ops->unmap(access->data, 0, ULONG_MAX);
> > + mutex_lock(&access->ioas_lock);
> > + }
> > + iopt_remove_access(&cur_ioas->iopt, access);
> > + refcount_dec(&cur_ioas->obj.users);
> > +}
> > +
> > +void iommufd_access_detach(struct iommufd_access *access)
> > +{
> > + mutex_lock(&access->ioas_lock);
> > + if (WARN_ON(!access->ioas))
> > + goto out;
> > + __iommufd_access_detach(access);
> > +out:
> > + access->ioas_unpin = NULL;
> > + mutex_unlock(&access->ioas_lock);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);
>
> There is not really any benefit to make this two functions
The __iommufd_access_detach() will be used by replace() in the
following series. Yet, let's merge them here then. And I'll add
__iommufd_access_detach() back in the replace series.
> > int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> > {
> [..]
> > if (access->ioas) {
>
> if (access->ioas || access->ioas_unpin) {
Ack.
> But I wonder if it should be a WARN_ON? Does VFIO protect against
> the userspace racing detach and attach, or do we expect to do it here?
VFIO has a vdev->iommufd_attached flag to prevent a double call
of this function. And detach and attach there also have a mutex
protection. So it should be a WARN_ON here, I think.
> > @@ -579,8 +620,8 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
> > void iommufd_access_unpin_pages(struct iommufd_access *access,
> > unsigned long iova, unsigned long length)
> > {
> > - struct io_pagetable *iopt = &access->ioas->iopt;
> > struct iopt_area_contig_iter iter;
> > + struct io_pagetable *iopt;
> > unsigned long last_iova;
> > struct iopt_area *area;
> >
> > @@ -588,6 +629,13 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
> > WARN_ON(check_add_overflow(iova, length - 1, &last_iova)))
> > return;
> >
> > + mutex_lock(&access->ioas_lock);
> > + if (!access->ioas_unpin) {
>
> This should be WARN_ON(), the driver has done something wrong if we
> call this after the access has been detached.
Ack. Also adding a line of comments for that:
+ /*
+ * The driver must be doing something wrong if it calls this before an
+ * iommufd_access_attach() or after an iommufd_access_detach().
+ */
+ if (WARN_ON(!access->ioas_unpin)) {
Thanks
Nic
