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

[Parav Pandit]

> -----Original Message-----
> From: Cornelia Huck <cohuck@xxxxxxxxxx>
> Sent: Wednesday, May 8, 2019 12:10 PM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kwankhede@xxxxxxxxxx; alex.williamson@xxxxxxxxxx; cjia@xxxxxxxxxx
> Subject: Re: [PATCHv2 08/10] vfio/mdev: Improve the create/remove
> sequence
> 
> On Tue, 30 Apr 2019 17:49:35 -0500
> Parav Pandit <parav@xxxxxxxxxxxx> wrote:
> 
> > This patch addresses below two issues and prepares the code to address
> > 3rd issue listed below.
> >
> > 1. mdev device is placed on the mdev bus before it is created in the
> > vendor driver. Once a device is placed on the mdev bus without
> > creating its supporting underlying vendor device, mdev driver's probe()
> gets triggered.
> > However there isn't a stable mdev available to work on.
> >
> >    create_store()
> >      mdev_create_device()
> >        device_register()
> >           ...
> >          vfio_mdev_probe()
> >         [...]
> >         parent->ops->create()
> >           vfio_ap_mdev_create()
> >             mdev_set_drvdata(mdev, matrix_mdev);
> >             /* Valid pointer set above */
> >
> > Due to this way of initialization, mdev driver who want to use the
> > mdev, doesn't have a valid mdev to work on.
> >
> > 2. Current creation sequence is,
> >    parent->ops_create()
> >    groups_register()
> >
> > Remove sequence is,
> >    parent->ops->remove()
> >    groups_unregister()
> >
> > However, remove sequence should be exact mirror of creation sequence.
> > Once this is achieved, all users of the mdev will be terminated first
> > before removing underlying vendor device.
> > (Follow standard linux driver model).
> > At that point vendor's remove() ops shouldn't failed because device is
> > taken off the bus that should terminate the users.
> >
> > 3. When remove operation fails, mdev sysfs removal attempts to add the
> > file back on already removed device. Following call trace [1] is observed.
> >
> > [1] call trace:
> > kernel: WARNING: CPU: 2 PID: 9348 at fs/sysfs/file.c:327
> > sysfs_create_file_ns+0x7f/0x90
> > kernel: CPU: 2 PID: 9348 Comm: bash Kdump: loaded Not tainted
> > 5.1.0-rc6-vdevbus+ #6
> > kernel: Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b
> > 08/09/2016
> > kernel: RIP: 0010:sysfs_create_file_ns+0x7f/0x90
> > kernel: Call Trace:
> > kernel: remove_store+0xdc/0x100 [mdev]
> > kernel: kernfs_fop_write+0x113/0x1a0
> > kernel: vfs_write+0xad/0x1b0
> > kernel: ksys_write+0x5a/0xe0
> > kernel: do_syscall_64+0x5a/0x210
> > kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > Therefore, mdev core is improved in following ways.
> >
> > 1. Before placing mdev devices on the bus, perform vendor drivers
> > creation which supports the mdev creation.
> > This ensures that mdev specific all necessary fields are initialized
> > before a given mdev can be accessed by bus driver.
> > This follows standard Linux kernel bus and device model similar to
> > other widely used PCI bus.
> >
> > 2. During remove flow, first remove the device from the bus. This
> > ensures that any bus specific devices and data is cleared.
> > Once device is taken of the mdev bus, perform remove() of mdev from
> > the vendor driver.
> >
> > 3. Linux core device model provides way to register and auto
> > unregister the device sysfs attribute groups at dev->groups.
> > Make use of this groups to let core create the groups and simplify
> > code to avoid explicit groups creation and removal.
> >
> > A below stack dump of a mdev device remove process also ensures that
> > vfio driver guards against device removal already in use.
> >
> >  cat /proc/21962/stack
> > [<0>] vfio_del_group_dev+0x216/0x3c0 [vfio] [<0>]
> > mdev_remove+0x21/0x40 [mdev] [<0>]
> > device_release_driver_internal+0xe8/0x1b0
> > [<0>] bus_remove_device+0xf9/0x170
> > [<0>] device_del+0x168/0x350
> > [<0>] mdev_device_remove_common+0x1d/0x50 [mdev] [<0>]
> > mdev_device_remove+0x8c/0xd0 [mdev] [<0>] remove_store+0x71/0x90
> > [mdev] [<0>] kernfs_fop_write+0x113/0x1a0 [<0>] vfs_write+0xad/0x1b0
> > [<0>] ksys_write+0x5a/0xe0 [<0>] do_syscall_64+0x5a/0x210 [<0>]
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [<0>] 0xffffffffffffffff
> >
> > This prepares the code to eliminate calling device_create_file() in
> > subsquent patch.
> 
> I'm afraid I have a bit of a problem following this explanation, so let me try
> to summarize what the patch does to make sure that I understand it
> correctly:
> 
> - Add the sysfs groups to device->groups so that the driver core deals
>   with proper registration/deregistration.
> - Split the device registration/deregistration sequence so that some
>   things can be done between initialization of the device and hooking
>   it up to the infrastructure respectively after deregistering it from
>   the infrastructure but before giving up our final reference. In
>   particular, this means invoking the ->create and ->remove callback in
>   those new windows. This gives the vendor driver an initialized mdev
>   device to work with during creation.
> - Don't allow ->remove to fail, as the device is already removed from
>   the infrastructure at that point in time.
> 
You got all the points pretty accurate.

> >
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > ---
> >  drivers/vfio/mdev/mdev_core.c    | 94 +++++++++-----------------------
> >  drivers/vfio/mdev/mdev_private.h |  2 +-
> >  drivers/vfio/mdev/mdev_sysfs.c   |  2 +-
> >  3 files changed, 27 insertions(+), 71 deletions(-)
> 
> (...)
> 
> > @@ -310,41 +265,43 @@ int mdev_device_create(struct kobject *kobj,
> >
> >  	mdev->parent = parent;
> >
> > +	device_initialize(&mdev->dev);
> >  	mdev->dev.parent  = dev;
> >  	mdev->dev.bus     = &mdev_bus_type;
> >  	mdev->dev.release = mdev_device_release;
> >  	dev_set_name(&mdev->dev, "%pUl", uuid);
> > +	mdev->dev.groups = parent->ops->mdev_attr_groups;
> 
> I like that, that makes things much easier.
> 
True.

> > +	mdev->type_kobj = kobj;
> >
> > -	ret = device_register(&mdev->dev);
> > -	if (ret) {
> > -		put_device(&mdev->dev);
> > -		goto mdev_fail;
> > -	}
> > +	ret = parent->ops->create(kobj, mdev);
> > +	if (ret)
> > +		goto ops_create_fail;
> >
> > -	ret = mdev_device_create_ops(kobj, mdev);
> > +	ret = device_add(&mdev->dev);
> >  	if (ret)
> > -		goto create_fail;
> > +		goto add_fail;
> >
> >  	ret = mdev_create_sysfs_files(&mdev->dev, type);
> > -	if (ret) {
> > -		mdev_device_remove_ops(mdev, true);
> > -		goto create_fail;
> > -	}
> > +	if (ret)
> > +		goto sysfs_fail;
> >
> > -	mdev->type_kobj = kobj;
> >  	mdev->active = true;
> >  	dev_dbg(&mdev->dev, "MDEV: created\n");
> >
> >  	return 0;
> >
> > -create_fail:
> > -	device_unregister(&mdev->dev);
> > +sysfs_fail:
> > +	device_del(&mdev->dev);
> > +add_fail:
> > +	parent->ops->remove(mdev);
> > +ops_create_fail:
> > +	put_device(&mdev->dev);
> >  mdev_fail:
> >  	mdev_put_parent(parent);
> >  	return ret;
> >  }
> >
> > -int mdev_device_remove(struct device *dev, bool force_remove)
> > +int mdev_device_remove(struct device *dev)
> >  {
> >  	struct mdev_device *mdev, *tmp;
> >  	struct mdev_parent *parent;
> > @@ -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.
samples/vfio-mdev/mbochs.c mbochs_remove() always returns 0.

> >
> > -	ret = mdev_device_remove_ops(mdev, force_remove);
> > -	if (ret) {
> > -		mdev->active = true;
> > -		return ret;
> > -	}
> > -
> > -	mdev_remove_sysfs_files(dev, type);
> > -	device_unregister(dev);
> > +	/* Balances with device_initialize() */
> > +	put_device(&mdev->dev);
> >  	mdev_put_parent(parent);
> >
> >  	return 0;
> 
> I think that looks sane in general, but the commit message might benefit
> from tweaking.
Part of your description is more crisp than my commit message, I can probably take snippet from it to improve?
Or any specific entries in commit message that I should address?