Re: [Intel-gfx] [PATCH v3 2/6] iommufd: Create access in vfio_iommufd_emulated_bind()

2023-03-31 Thread Jason Gunthorpe
On Fri, Mar 31, 2023 at 08:16:16AM +, Tian, Kevin wrote:
> > From: Jason Gunthorpe 
> > Sent: Thursday, March 30, 2023 4:00 AM
> > 
> > On Mon, Mar 27, 2023 at 02:33:47AM -0700, Yi Liu wrote:
> > > @@ -494,6 +479,30 @@ void iommufd_access_destroy(struct
> > iommufd_access *access)
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
> > >
> > > +int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> > > +{
> > > + struct iommufd_ioas *new_ioas;
> > > + int rc = 0;
> > > +
> > > + if (access->ioas != NULL && access->ioas->obj.id != ioas_id)
> > > + return -EINVAL;
> > 
> > This should just be
> > 
> >if (access->ioas)
> > return -EINVAL;
> 
> the physical path has the same check:
> 
>   if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) {
>   rc = -EINVAL;
>   goto err_unlock;
>   }
> 
> If we change here then that one should be changed too.

No, that one is checking if the another device attached to the same
group is a compatible hwpt so we succeed to attach the new device

Jason


Re: [Intel-gfx] [PATCH v3 2/6] iommufd: Create access in vfio_iommufd_emulated_bind()

2023-03-31 Thread Tian, Kevin
> From: Jason Gunthorpe 
> Sent: Thursday, March 30, 2023 4:00 AM
> 
> On Mon, Mar 27, 2023 at 02:33:47AM -0700, Yi Liu wrote:
> > @@ -494,6 +479,30 @@ void iommufd_access_destroy(struct
> iommufd_access *access)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
> >
> > +int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> > +{
> > +   struct iommufd_ioas *new_ioas;
> > +   int rc = 0;
> > +
> > +   if (access->ioas != NULL && access->ioas->obj.id != ioas_id)
> > +   return -EINVAL;
> 
> This should just be
> 
>if (access->ioas)
> return -EINVAL;

the physical path has the same check:

if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) {
rc = -EINVAL;
goto err_unlock;
}

If we change here then that one should be changed too.


Re: [Intel-gfx] [PATCH v3 2/6] iommufd: Create access in vfio_iommufd_emulated_bind()

2023-03-29 Thread Jason Gunthorpe
On Mon, Mar 27, 2023 at 02:33:47AM -0700, Yi Liu wrote:
> @@ -494,6 +479,30 @@ void iommufd_access_destroy(struct iommufd_access 
> *access)
>  }
>  EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
>  
> +int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> +{
> + struct iommufd_ioas *new_ioas;
> + int rc = 0;
> +
> + if (access->ioas != NULL && access->ioas->obj.id != ioas_id)
> + return -EINVAL;

This should just be

   if (access->ioas)
return -EINVAL;

> +
> + new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
> + if (IS_ERR(new_ioas))
> + return PTR_ERR(new_ioas);
> +
> + rc = iopt_add_access(_ioas->iopt, access);
> + if (rc) {
> + iommufd_put_object(_ioas->obj);
> + return rc;
> + }
> + iommufd_ref_to_users(_ioas->obj);
> +
> + access->ioas = new_ioas;

Since if ioas is non-null here then we will lose the reference counts
already held.

I'll fix it

Jason


[Intel-gfx] [PATCH v3 2/6] iommufd: Create access in vfio_iommufd_emulated_bind()

2023-03-27 Thread Yi Liu
From: Nicolin Chen 

There are needs to created iommufd_access prior to have an IOAS and set
IOAS later. Like the vfio device cdev needs to have an iommufd object
to represent the bond (iommufd_access) and IOAS replacement.

Moves the iommufd_access_create() call into vfio_iommufd_emulated_bind(),
making it symmetric with the __vfio_iommufd_access_destroy() call in the
vfio_iommufd_emulated_unbind(). This means an access is created/destroyed
by the bind()/unbind(), and the vfio_iommufd_emulated_attach_ioas() only
updates the access->ioas pointer.

Since vfio_iommufd_emulated_bind() does not provide ioas_id, drop it from
the argument list of iommufd_access_create(). Instead, add a new access
API iommufd_access_attach() to set the access->ioas pointer. Also, set
vdev->iommufd_attached accordingly, similar to the physical pathway.

Reviewed-by: Kevin Tian 
Reviewed-by: Jason Gunthorpe 
Tested-by: Terrence Xu 
Signed-off-by: Nicolin Chen 
Signed-off-by: Yi Liu 
---
 drivers/iommu/iommufd/device.c   | 51 +++-
 drivers/iommu/iommufd/selftest.c |  5 +++-
 drivers/vfio/iommufd.c   | 24 ++-
 include/linux/iommufd.h  |  3 +-
 4 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index a0c66f47a65a..6999a10b5496 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -412,9 +412,12 @@ void iommufd_access_destroy_object(struct iommufd_object 
*obj)
struct iommufd_access *access =
container_of(obj, struct iommufd_access, obj);
 
-   iopt_remove_access(>ioas->iopt, access);
+   if (access->ioas) {
+   iopt_remove_access(>ioas->iopt, access);
+   refcount_dec(>ioas->obj.users);
+   access->ioas = NULL;
+   }
iommufd_ctx_put(access->ictx);
-   refcount_dec(>ioas->obj.users);
 }
 
 /**
@@ -431,12 +434,10 @@ void iommufd_access_destroy_object(struct iommufd_object 
*obj)
  * The provided ops are required to use iommufd_access_pin_pages().
  */
 struct iommufd_access *
-iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
+iommufd_access_create(struct iommufd_ctx *ictx,
  const struct iommufd_access_ops *ops, void *data)
 {
struct iommufd_access *access;
-   struct iommufd_object *obj;
-   int rc;
 
/*
 * There is no uAPI for the access object, but to keep things symmetric
@@ -449,21 +450,10 @@ iommufd_access_create(struct iommufd_ctx *ictx, u32 
ioas_id,
access->data = data;
access->ops = ops;
 
-   obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
-   if (IS_ERR(obj)) {
-   rc = PTR_ERR(obj);
-   goto out_abort;
-   }
-   access->ioas = container_of(obj, struct iommufd_ioas, obj);
-   iommufd_ref_to_users(obj);
-
if (ops->needs_pin_pages)
access->iova_alignment = PAGE_SIZE;
else
access->iova_alignment = 1;
-   rc = iopt_add_access(>ioas->iopt, access);
-   if (rc)
-   goto out_put_ioas;
 
/* The calling driver is a user until iommufd_access_destroy() */
refcount_inc(>obj.users);
@@ -471,11 +461,6 @@ iommufd_access_create(struct iommufd_ctx *ictx, u32 
ioas_id,
iommufd_ctx_get(ictx);
iommufd_object_finalize(ictx, >obj);
return access;
-out_put_ioas:
-   refcount_dec(>ioas->obj.users);
-out_abort:
-   iommufd_object_abort(ictx, >obj);
-   return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
 
@@ -494,6 +479,30 @@ void iommufd_access_destroy(struct iommufd_access *access)
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
 
+int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
+{
+   struct iommufd_ioas *new_ioas;
+   int rc = 0;
+
+   if (access->ioas != NULL && access->ioas->obj.id != ioas_id)
+   return -EINVAL;
+
+   new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
+   if (IS_ERR(new_ioas))
+   return PTR_ERR(new_ioas);
+
+   rc = iopt_add_access(_ioas->iopt, access);
+   if (rc) {
+   iommufd_put_object(_ioas->obj);
+   return rc;
+   }
+   iommufd_ref_to_users(_ioas->obj);
+
+   access->ioas = new_ioas;
+   return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);
+
 /**
  * iommufd_access_notify_unmap - Notify users of an iopt to stop using it
  * @iopt: iopt to work on
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 8667eb222cf1..1e89e1a8c5f0 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -571,7 +571,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd 
*ucmd,
}
 
access = iommufd_access_create(
-   ucmd->ictx, ioas_id,
+   ucmd->ictx,