Re: [PATCHv2 08/10] vfio/mdev: Improve the create/remove sequence

[Pierre Morel <pmorel@linux.ibm.com>]

On 09/05/2019 11:06, Cornelia Huck wrote:
> [vfio-ap folks: find a question regarding removal further down]
> 
> On Wed, 8 May 2019 22:06:48 +0000
> Parav Pandit <parav@mellanox.com> wrote:
> 
>>> -----Original Message-----
>>> From: Cornelia Huck <cohuck@redhat.com>
>>> Sent: Wednesday, May 8, 2019 12:10 PM
>>> To: Parav Pandit <parav@mellanox.com>
>>> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
>>> kwankhede@nvidia.com; alex.williamson@redhat.com; cjia@nvidia.com
>>> Subject: Re: [PATCHv2 08/10] vfio/mdev: Improve the create/remove
>>> sequence
>>>
>>> On Tue, 30 Apr 2019 17:49:35 -0500
>>> Parav Pandit <parav@mellanox.com> wrote:
>>>    

...snip...

>>>> @@ -373,16 +330,15 @@ int mdev_device_remove(struct device *dev,
>>> bool force_remove)
>>>>   	mutex_unlock(&mdev_list_lock);
>>>>
>>>>   	type = to_mdev_type(mdev->type_kobj);
>>>> +	mdev_remove_sysfs_files(dev, type);
>>>> +	device_del(&mdev->dev);
>>>>   	parent = mdev->parent;
>>>> +	ret = parent->ops->remove(mdev);
>>>> +	if (ret)
>>>> +		dev_err(&mdev->dev, "Remove failed: err=%d\n", ret);
>>>
>>> I think carrying on with removal regardless of the return code of the
>>> ->remove callback makes sense, as it simply matches usual practice.
>>> However, are we sure that every vendor driver works well with that? I think
>>> it should, as removal from bus unregistration (vs. from the sysfs
>>> file) was always something it could not veto, but have you looked at the
>>> individual drivers?
>>>    
>> I looked at following drivers a little while back.
>> Looked again now.
>>
>> drivers/gpu/drm/i915/gvt/kvmgt.c which clears the handle valid in intel_vgpu_release(), which should finish first before remove() is invoked.
>>
>> s390 vfio_ccw_mdev_remove() driver drivers/s390/cio/vfio_ccw_ops.c remove() always returns 0.
>> s39 crypo fails the remove() once vfio_ap_mdev_release marks kvm null, which should finish before remove() is invoked.
> 
> That one is giving me a bit of a headache (the ->kvm reference is
> supposed to keep us from detaching while a vm is running), so let's cc:
> the vfio-ap maintainers to see whether they have any concerns.
> 

We are aware of this race and we did correct this in the IRQ patches for 
which it would have become a real issue.
We now increment/decrement the KVM reference counter inside open and 
release.
Should be right after this.

Thanks for the cc,
Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany