Re: [PATCH V2 2/4] iommufd: Lock all objects

[Steven Sistare <steven.sistare@oracle.com>]

Hi Jason,
   Any comment on this patch before I sent V3?
There will be only a few changes in V3  in iommufd_take_all_iova_rwsem:
   * delete lockdep_on/off
   * replace
       down_write(&ioas->iopt.iova_rwsem);
     with
       down_write_nest_lock(&ioas->iopt.iova_rwsem,
                            &ictx->ioas_creation_lock);

- Steve

On 9/26/2024 9:53 AM, Steve Sistare wrote:
> Define helpers to lock and unlock all iommufd_ctx objects.
> This will allow DMA mappings to be updated atomically during live update.
> This code is substantially the same as an initial version provided by
> Jason, plus fixes.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   drivers/iommu/iommufd/ioas.c            | 67 +++++++++++++++++++++++++++++++++
>   drivers/iommu/iommufd/iommufd_private.h |  1 +
>   drivers/iommu/iommufd/main.c            |  1 +
>   3 files changed, 69 insertions(+)
> 
> diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c
> index 2d25ba0..8eb869d 100644
> --- a/drivers/iommu/iommufd/ioas.c
> +++ b/drivers/iommu/iommufd/ioas.c
> @@ -52,7 +52,10 @@ int iommufd_ioas_alloc_ioctl(struct iommufd_ucmd *ucmd)
>   	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>   	if (rc)
>   		goto out_table;
> +
> +	down_read(&ucmd->ictx->ioas_creation_lock);
>   	iommufd_object_finalize(ucmd->ictx, &ioas->obj);
> +	up_read(&ucmd->ictx->ioas_creation_lock);
>   	return 0;
>   
>   out_table:
> @@ -370,6 +373,70 @@ int iommufd_ioas_unmap(struct iommufd_ucmd *ucmd)
>   	return rc;
>   }
>   
> +static void iommufd_release_all_iova_rwsem(struct iommufd_ctx *ictx,
> +				      struct xarray *ioas_list)
> +{
> +	struct iommufd_ioas *ioas;
> +	unsigned long index;
> +
> +	xa_for_each(ioas_list, index, ioas) {
> +		up_write(&ioas->iopt.iova_rwsem);
> +		refcount_dec(&ioas->obj.users);
> +	}
> +	up_write(&ictx->ioas_creation_lock);
> +	xa_destroy(ioas_list);
> +}
> +
> +static int iommufd_take_all_iova_rwsem(struct iommufd_ctx *ictx,
> +				       struct xarray *ioas_list)
> +{
> +	struct iommufd_object *obj;
> +	unsigned long index;
> +	int rc;
> +
> +	/*
> +	 * This is very ugly, it is done instead of adding a lock around
> +	 * pages->source_mm, which is a performance path for mdev, we just
> +	 * obtain the write side of all the iova_rwsems which also protects the
> +	 * pages->source_*. Due to copies we can't know which IOAS could read
> +	 * from the pages, so we just lock everything. This is the only place
> +	 * locks are nested and they are uniformly taken in ID order.
> +	 *
> +	 * ioas_creation_lock prevents new IOAS from being installed in the
> +	 * xarray while we do this, and also prevents more than one thread from
> +	 * holding nested locks.
> +	 */
> +	down_write(&ictx->ioas_creation_lock);
> +	xa_lock(&ictx->objects);
> +	lockdep_off();
> +	xa_for_each(&ictx->objects, index, obj) {
> +		struct iommufd_ioas *ioas;
> +
> +		if (!obj || obj->type != IOMMUFD_OBJ_IOAS)
> +			continue;
> +
> +		if (!refcount_inc_not_zero(&obj->users))
> +			continue;
> +
> +		xa_unlock(&ictx->objects);
> +
> +		ioas = container_of(obj, struct iommufd_ioas, obj);
> +		down_write(&ioas->iopt.iova_rwsem);
> +
> +		rc = xa_err(xa_store(ioas_list, index, ioas, GFP_KERNEL));
> +		if (rc) {
> +			lockdep_on();
> +			iommufd_release_all_iova_rwsem(ictx, ioas_list);
> +			return rc;
> +		}
> +
> +		xa_lock(&ictx->objects);
> +	}
> +	lockdep_on();
> +	xa_unlock(&ictx->objects);
> +	return 0;
> +}
> +
>   int iommufd_option_rlimit_mode(struct iommu_option *cmd,
>   			       struct iommufd_ctx *ictx)
>   {
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 0ebb5bd..06a52f2 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -23,6 +23,7 @@ struct iommufd_ctx {
>   	struct xarray objects;
>   	struct xarray groups;
>   	wait_queue_head_t destroy_wait;
> +	struct rw_semaphore ioas_creation_lock;
>   
>   	u8 account_mode;
>   	/* Compatibility with VFIO no iommu */
> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> index 5f77ce6..13ec031 100644
> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -248,6 +248,7 @@ static int iommufd_fops_open(struct inode *inode, struct file *filp)
>   		pr_info_once("IOMMUFD is providing /dev/vfio/vfio, not VFIO.\n");
>   	}
>   
> +	init_rwsem(&ictx->ioas_creation_lock);
>   	xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
>   	xa_init(&ictx->groups);
>   	ictx->file = filp;