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

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

On 5/4/21 11:30 AM, Cornelia Huck wrote:
> On Mon, 3 May 2021 16:14:43 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> 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.
> You probably want to wire up the request callback and notify userspace.

Not sure what you mean by this, but I also don't think it matters.
After coding up the fix for this and testing it, I've learned that
if a user attempts to remove an mdev via the sysfs 'remove'
attribute while the mdev fd is still open (i.e., in use by the guest),
the mdev remove callback is not invoked until the fd is closed
(i.e., the guest is shut down). During that time, the mdev is
physically removed from sysfs so no further actions can be taken
on it; but, since the remove callback has yet to be called, the
guest will have access to the AP resources provided by the
mdev during that time. I also tested detaching the mdev device from the 
guest
(i.e., virsh detach-device) while the mdev was in the process of
being removed and this resulted in allowing the remove to
progress due to the mdev fd getting closed when it is detached.

>
> What happens today if the device in QEMU is removed via device_del?
> Does that already clean up things properly?

Hot plug/unplug is already supported, so yes, things get cleaned up
properly when the mdev fd is closed. This is handled by the mdev
release callback.

>