On Fri, 23 Apr 2021 20:03:03 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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;
> }
(...)
