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

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 4/26/21 1:48 PM, Cornelia Huck wrote:
> 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.

I agree, I was not aware that returning a non-zero return code
from this callback did not return the -EBUSY to userspace
when the mdev is removed.

>
> 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.

With the latest kernel from our tree, the remove hangs until the
guest is shutdown and the mdev fd is closed. During the hang, the
dmesg log has the following:

"No mdev vendor driver request callback support, blocked until released 
by user"

So, it looks like nothing is done with the mdev until the fd for the
mdev is closed when the guest is shut down, at which time the
mdev is removed.

>
> 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?)

This is correct.

>
> 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?

This case will occur whenever a user removes the mdev
by echoing a '1' into the mdev's sysfs 'remove' attribute
file. I'm not sure it can be considered graceful to take away
all of the crypto devices from a guest while it is running,
but there is a way to process the remove callback without
leaving things in a "weird, half-dead state".

Up to this point, the onus for ensuring the proper procedure
is followed when managing pass-through crypto devices
for a KVM guest is left to the system administrator. In
other words, we don't prevent an admin from shooting
him/herself in the foot when doing things such as removing
an mdev while a KVM guest is using it. With this in mind,
I will handle this case in the follow-on patches implementing
dynamic AP configuration support for KVM guests.

>
>> +
>> +	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;
>>   }
> (...)
>