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

Cornelia Huck
2021-04-26T17:49:14Z

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;
> }

(...)