diff mbox series

[v2,05/10] iommufd: Add replace support in iommufd_access_set_ioas()

Message ID 931be169ff4a1f4d4f1ed060d722c2dc17ce6667.1675802050.git.nicolinc@nvidia.com
State New
Headers
Series Add IO page table replacement support |

Commit Message

Nicolin Chen Feb. 7, 2023, 9:17 p.m. UTC 
  Support an access->ioas replacement in iommufd_access_set_ioas(), which
sets the access->ioas to NULL provisionally so that any further incoming
iommufd_access_pin_pages() callback can be blocked.

Then, call access->ops->unmap() to clean up the entire iopt. To allow an
iommufd_access_unpin_pages() callback to happen via this unmap() call,
add an ioas_unpin pointer so the unpin routine won't be affected by the
"access->ioas = NULL" trick above.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/device.c          | 16 ++++++++++++++--
 drivers/iommu/iommufd/iommufd_private.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Tian, Kevin Feb. 9, 2023, 3:13 a.m. UTC | #1 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, February 8, 2023 5:18 AM
> 
> Support an access->ioas replacement in iommufd_access_set_ioas(), which
> sets the access->ioas to NULL provisionally so that any further incoming
> iommufd_access_pin_pages() callback can be blocked.
> 
> Then, call access->ops->unmap() to clean up the entire iopt. To allow an
> iommufd_access_unpin_pages() callback to happen via this unmap() call,
> add an ioas_unpin pointer so the unpin routine won't be affected by the
> "access->ioas = NULL" trick above.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

move patch10 before this one.

> ---
>  drivers/iommu/iommufd/device.c          | 16 ++++++++++++++--
>  drivers/iommu/iommufd/iommufd_private.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c
> b/drivers/iommu/iommufd/device.c
> index f4bd6f532a90..10ce47484ffa 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct
> iommufd_access *access, u32 ioas_id)
>  		iommufd_ref_to_users(obj);
>  	}
> 
> +	/*
> +	 * 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 (cur_ioas) {
> +		if (new_ioas) {
> +			mutex_unlock(&access->ioas_lock);
> +			access->ops->unmap(access->data, 0, ULONG_MAX);
> +			mutex_lock(&access->ioas_lock);
> +		}

why does above only apply to a valid new_ioas? this is the cleanup on
cur_ioas then required even when new_ioas=NULL.

Instead here the check should be "if (access->ops->unmap)". with
patch10 the cleanup is required only for driver which is allowed to
pin pages.
Nicolin Chen Feb. 9, 2023, 8:28 p.m. UTC | #2 
On Thu, Feb 09, 2023 at 03:13:08AM +0000, Tian, Kevin wrote:
 
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct
> > iommufd_access *access, u32 ioas_id)
> >               iommufd_ref_to_users(obj);
> >       }
> >
> > +     /*
> > +      * 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 (cur_ioas) {
> > +             if (new_ioas) {
> > +                     mutex_unlock(&access->ioas_lock);
> > +                     access->ops->unmap(access->data, 0, ULONG_MAX);
> > +                     mutex_lock(&access->ioas_lock);
> > +             }
> 
> why does above only apply to a valid new_ioas? this is the cleanup on
> cur_ioas then required even when new_ioas=NULL.
  
Though it'd make sense to put it in the common path, our current
detach routine doesn't call this unmap. If we do so, it'd become
something new to the normal detach routine. Or does this mean the
detach routine has been missing an unmap call so far?

Thanks
Nic
Jason Gunthorpe Feb. 9, 2023, 8:49 p.m. UTC | #3 
On Thu, Feb 09, 2023 at 12:28:45PM -0800, Nicolin Chen wrote:
> On Thu, Feb 09, 2023 at 03:13:08AM +0000, Tian, Kevin wrote:
>  
> > > --- a/drivers/iommu/iommufd/device.c
> > > +++ b/drivers/iommu/iommufd/device.c
> > > @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct
> > > iommufd_access *access, u32 ioas_id)
> > >               iommufd_ref_to_users(obj);
> > >       }
> > >
> > > +     /*
> > > +      * 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 (cur_ioas) {
> > > +             if (new_ioas) {
> > > +                     mutex_unlock(&access->ioas_lock);
> > > +                     access->ops->unmap(access->data, 0, ULONG_MAX);
> > > +                     mutex_lock(&access->ioas_lock);
> > > +             }
> > 
> > why does above only apply to a valid new_ioas? this is the cleanup on
> > cur_ioas then required even when new_ioas=NULL.
>   
> Though it'd make sense to put it in the common path, our current
> detach routine doesn't call this unmap. If we do so, it'd become
> something new to the normal detach routine. Or does this mean the
> detach routine has been missing an unmap call so far?

By the time vfio_iommufd_emulated_unbind() is called the driver's
close_device() has already returned

At this point the driver should have removed all active pins.

We should not call back into the driver with unmap after its
close_device() returns.

However, this function is not on the close_device path so it should
always flush all existing mappings before attempting to change the
ioas to anything.

Jason
Nicolin Chen Feb. 9, 2023, 10:18 p.m. UTC | #4 
On Thu, Feb 09, 2023 at 04:49:55PM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 09, 2023 at 12:28:45PM -0800, Nicolin Chen wrote:
> > On Thu, Feb 09, 2023 at 03:13:08AM +0000, Tian, Kevin wrote:
> >  
> > > > --- a/drivers/iommu/iommufd/device.c
> > > > +++ b/drivers/iommu/iommufd/device.c
> > > > @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct
> > > > iommufd_access *access, u32 ioas_id)
> > > >               iommufd_ref_to_users(obj);
> > > >       }
> > > >
> > > > +     /*
> > > > +      * 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 (cur_ioas) {
> > > > +             if (new_ioas) {
> > > > +                     mutex_unlock(&access->ioas_lock);
> > > > +                     access->ops->unmap(access->data, 0, ULONG_MAX);
> > > > +                     mutex_lock(&access->ioas_lock);
> > > > +             }
> > > 
> > > why does above only apply to a valid new_ioas? this is the cleanup on
> > > cur_ioas then required even when new_ioas=NULL.
> >   
> > Though it'd make sense to put it in the common path, our current
> > detach routine doesn't call this unmap. If we do so, it'd become
> > something new to the normal detach routine. Or does this mean the
> > detach routine has been missing an unmap call so far?
> 
> By the time vfio_iommufd_emulated_unbind() is called the driver's
> close_device() has already returned
> 
> At this point the driver should have removed all active pins.
> 
> We should not call back into the driver with unmap after its
> close_device() returns.

I see. Just found that vfio_device_last_close().

> However, this function is not on the close_device path so it should
> always flush all existing mappings before attempting to change the
> ioas to anything.

OK. That means I also need to fix my change here:

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 8a9834fc129a..ba3fd35b7540 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -465,7 +465,10 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
        struct iommufd_access *access =
                container_of(obj, struct iommufd_access, obj);

-       iommufd_access_set_ioas(access, 0);
+       if (access->ioas) {
+               iopt_remove_access(&access->ioas->iopt, access);
+               refcount_dec(&access->ioas->obj.users);
+       }
        iommufd_ctx_put(access->ictx);
        mutex_destroy(&access->ioas_lock);
 }

Otherwise, the close_device path would invoke this function via
the unbind() call.

Thanks
Nic
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index f4bd6f532a90..10ce47484ffa 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -509,11 +509,23 @@  int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
 		iommufd_ref_to_users(obj);
 	}
 
+	/*
+	 * 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 (cur_ioas) {
+		if (new_ioas) {
+			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);
 	}
 
+	access->ioas_unpin = new_ioas;
 	access->ioas = new_ioas;
 	mutex_unlock(&access->ioas_lock);
 
@@ -587,11 +599,11 @@  void iommufd_access_unpin_pages(struct iommufd_access *access,
 		return;
 
 	mutex_lock(&access->ioas_lock);
-	if (!access->ioas) {
+	if (!access->ioas_unpin) {
 		mutex_unlock(&access->ioas_lock);
 		return;
 	}
-	iopt = &access->ioas->iopt;
+	iopt = &access->ioas_unpin->iopt;
 
 	down_read(&iopt->iova_rwsem);
 	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 2f4bb106bac6..593138bb37b8 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -261,6 +261,7 @@  struct iommufd_access {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
 	struct iommufd_ioas *ioas;
+	struct iommufd_ioas *ioas_unpin;
 	struct mutex ioas_lock;
 	const struct iommufd_access_ops *ops;
 	void *data;