Re: [PATCH 06/12] vfio/ap_ops: Convert to use vfio_register_group_dev()

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 23 Apr 2021 20:03:03 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> This is straightforward conversion, the ap_matrix_mdev is actually serving
> as the vfio_device and we can replace all the mdev_get_drvdata()'s with a
> simple container_of().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c     | 137 ++++++++++++++++----------
>  drivers/s390/crypto/vfio_ap_private.h |   2 +
>  2 files changed, 89 insertions(+), 50 deletions(-)
> 

(...)

> -static int vfio_ap_mdev_remove(struct mdev_device *mdev)
> +static void vfio_ap_mdev_remove(struct mdev_device *mdev)
>  {
> -	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> +	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(&mdev->dev);
>  
> -	if (matrix_mdev->kvm)
> -		return -EBUSY;
> +	/* FIXME: Remove isn't allowed to fail */
> +	if (WARN_ON(matrix_mdev->kvm))
> +		return;

This is a pre-existing problem, but the rework now makes it more
obvious.

Previously, the mdev code would only print a warning and then continue
with device removal, even if a ->remove() callback returned an error.
Now, it's quite clear that we'll end up in a weird half-dead state.

IIRC, the check for matrix_mdev->kvm is intended to protect against
ripping out the device under a running guest (I think it needs to
manipulate some crypto control blocks?)

So my question for the vfio-ap maintainers is: Can we actually end up
in this case? If yes, is there any way to gracefully shut down the
device?

> +
> +	vfio_unregister_group_dev(&matrix_mdev->vdev);
>  
>  	mutex_lock(&matrix_dev->lock);
> -	vfio_ap_mdev_reset_queues(mdev);
> +	vfio_ap_mdev_reset_queues(matrix_mdev);
>  	list_del(&matrix_mdev->node);
>  	mutex_unlock(&matrix_dev->lock);
>  
>  	kfree(matrix_mdev);
> -	mdev_set_drvdata(mdev, NULL);
>  	atomic_inc(&matrix_dev->available_instances);
> -
> -	return 0;
>  }

(...)