| Message ID | 20220827171037.30297-16-kevin.tian@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tidy up vfio_device life cycle | expand |
On Sun, 28 Aug 2022 01:10:37 +0800 Kevin Tian <kevin.tian@intel.com> wrote: > From: Yi Liu <yi.l.liu@intel.com> > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > sysfs path of the parent, indicating the device is bound to a vfio > driver, e.g.: > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > It is also a preparatory step toward adding cdev for supporting future > device-oriented uAPI. Shall we start Documentation/ABI/testing/vfio-dev now? Thanks. Alex
On Tue, Aug 30, 2022 at 04:18:38PM -0600, Alex Williamson wrote: > On Sun, 28 Aug 2022 01:10:37 +0800 > Kevin Tian <kevin.tian@intel.com> wrote: > > > From: Yi Liu <yi.l.liu@intel.com> > > > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > > sysfs path of the parent, indicating the device is bound to a vfio > > driver, e.g.: > > > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > > > It is also a preparatory step toward adding cdev for supporting future > > device-oriented uAPI. > > Shall we start Documentation/ABI/testing/vfio-dev now? Thanks. I always thought that was something to use when adding new custom sysfs attributes? Here we are just creating a standard struct device with its standard sysfs? Jason
On Sun, Aug 28, 2022 at 01:10:37AM +0800, Kevin Tian wrote: > From: Yi Liu <yi.l.liu@intel.com> > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > sysfs path of the parent, indicating the device is bound to a vfio > driver, e.g.: > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > It is also a preparatory step toward adding cdev for supporting future > device-oriented uAPI. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/vfio_main.c | 70 +++++++++++++++++++++++++++++++++------- > include/linux/vfio.h | 6 ++-- > 2 files changed, 61 insertions(+), 15 deletions(-) > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 0c5d120aeced..9ad0cbb83f1c 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -46,6 +46,8 @@ static struct vfio { > struct mutex group_lock; /* locks group_list */ > struct ida group_ida; > dev_t group_devt; > + struct class *device_class; > + struct ida device_ida; > } vfio; > > struct vfio_iommu_driver { > @@ -524,11 +526,19 @@ EXPORT_SYMBOL_GPL(_vfio_alloc_device); > * > * Only vfio-ccw driver should call this interface. > */ > +static void vfio_device_release(struct device *dev); Since you added this new function in patch 1, it would be nice to place it in a way that avoids this forward reference in this patch > ret = alloc_chrdev_region(&vfio.group_devt, 0, MINORMASK + 1, "vfio"); I think we should change this "vfio" string at this point, it is really the group fd, so "vfio_group" ? It only shows in procfs. Jason
> From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Wednesday, August 31, 2022 7:53 AM > > On Tue, Aug 30, 2022 at 04:18:38PM -0600, Alex Williamson wrote: > > On Sun, 28 Aug 2022 01:10:37 +0800 > > Kevin Tian <kevin.tian@intel.com> wrote: > > > > > From: Yi Liu <yi.l.liu@intel.com> > > > > > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > > > sysfs path of the parent, indicating the device is bound to a vfio > > > driver, e.g.: > > > > > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > > > > > It is also a preparatory step toward adding cdev for supporting future > > > device-oriented uAPI. > > > > Shall we start Documentation/ABI/testing/vfio-dev now? Thanks. > > I always thought that was something to use when adding new custom > sysfs attributes? > > Here we are just creating a standard struct device with its standard > sysfs? > There is nothing special for vfio-dev/vfioX. But from pci device p.o.v this does introduce a custom node in the directory, which is probably what Alex referred to? Anyway if required following can be introduced: diff --git a/Documentation/ABI/testing/sysfs-devices-vfio-dev b/Documentation/ABI/testing/sysfs-devices-vfio-dev new file mode 100644 index 000000000000..dfe8baaf1ccb --- /dev/null +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev @@ -0,0 +1,8 @@ +What: /sys/.../<device>/vfio-dev/vfioX/ +Date: September 2022 +Contact: Yi Liu <yi.l.liu@intel.com> +Description: + This directory is created when the device is bound to a + vfio driver. The layout under this directory matches what + exists for a standard 'struct device'. 'X' is a random + number marking this device in vfio. At the start I thought it might make more sense to add it into an existing vfio ABI file. But looks it doesn't exist. Curious why nobody asked for ABI doc for /dev/vfio/vfio, /sys/class/vfio, etc... Thanks Kevin
> From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Wednesday, August 31, 2022 8:32 AM > > On Sun, Aug 28, 2022 at 01:10:37AM +0800, Kevin Tian wrote: > > From: Yi Liu <yi.l.liu@intel.com> > > > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > > sysfs path of the parent, indicating the device is bound to a vfio > > driver, e.g.: > > > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > > > It is also a preparatory step toward adding cdev for supporting future > > device-oriented uAPI. > > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > Signed-off-by: Kevin Tian <kevin.tian@intel.com> > > --- > > drivers/vfio/vfio_main.c | 70 +++++++++++++++++++++++++++++++++------- > > include/linux/vfio.h | 6 ++-- > > 2 files changed, 61 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > > index 0c5d120aeced..9ad0cbb83f1c 100644 > > --- a/drivers/vfio/vfio_main.c > > +++ b/drivers/vfio/vfio_main.c > > @@ -46,6 +46,8 @@ static struct vfio { > > struct mutex group_lock; /* locks group_list */ > > struct ida group_ida; > > dev_t group_devt; > > + struct class *device_class; > > + struct ida device_ida; > > } vfio; > > > > struct vfio_iommu_driver { > > @@ -524,11 +526,19 @@ EXPORT_SYMBOL_GPL(_vfio_alloc_device); > > * > > * Only vfio-ccw driver should call this interface. > > */ > > +static void vfio_device_release(struct device *dev); > > Since you added this new function in patch 1, it would be nice to > place it in a way that avoids this forward reference in this patch > > > ret = alloc_chrdev_region(&vfio.group_devt, 0, MINORMASK + 1, > "vfio"); > > I think we should change this "vfio" string at this point, it is > really the group fd, so "vfio_group" ? > > It only shows in procfs. > All make sense and fixed.
On Wed, 31 Aug 2022 06:10:51 +0000 "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Jason Gunthorpe <jgg@ziepe.ca> > > Sent: Wednesday, August 31, 2022 7:53 AM > > > > On Tue, Aug 30, 2022 at 04:18:38PM -0600, Alex Williamson wrote: > > > On Sun, 28 Aug 2022 01:10:37 +0800 > > > Kevin Tian <kevin.tian@intel.com> wrote: > > > > > > > From: Yi Liu <yi.l.liu@intel.com> > > > > > > > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > > > > sysfs path of the parent, indicating the device is bound to a vfio > > > > driver, e.g.: > > > > > > > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > > > > > > > It is also a preparatory step toward adding cdev for supporting future > > > > device-oriented uAPI. > > > > > > Shall we start Documentation/ABI/testing/vfio-dev now? Thanks. > > > > I always thought that was something to use when adding new custom > > sysfs attributes? > > > > Here we are just creating a standard struct device with its standard > > sysfs? > > > > There is nothing special for vfio-dev/vfioX. But from pci device p.o.v > this does introduce a custom node in the directory, which is probably > what Alex referred to? Yup, but not just for pci, we're adding a node into the device directory for any device bound to vfio. > Anyway if required following can be introduced: > > diff --git a/Documentation/ABI/testing/sysfs-devices-vfio-dev b/Documentation/ABI/testing/sysfs-devices-vfio-dev > new file mode 100644 > index 000000000000..dfe8baaf1ccb > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev > @@ -0,0 +1,8 @@ > +What: /sys/.../<device>/vfio-dev/vfioX/ > +Date: September 2022 > +Contact: Yi Liu <yi.l.liu@intel.com> > +Description: > + This directory is created when the device is bound to a > + vfio driver. The layout under this directory matches what > + exists for a standard 'struct device'. 'X' is a random > + number marking this device in vfio. It's not really random, it's a unique index. Seems like a good starting point. > > At the start I thought it might make more sense to add it into an > existing vfio ABI file. But looks it doesn't exist. > > Curious why nobody asked for ABI doc for /dev/vfio/vfio, /sys/class/vfio, etc... Oversight, there should probably be a sysfs-class-vfio file. Thanks, Alex
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Thursday, September 1, 2022 1:15 AM > > On Wed, 31 Aug 2022 06:10:51 +0000 > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > From: Jason Gunthorpe <jgg@ziepe.ca> > > > Sent: Wednesday, August 31, 2022 7:53 AM > > > > > > On Tue, Aug 30, 2022 at 04:18:38PM -0600, Alex Williamson wrote: > > > > On Sun, 28 Aug 2022 01:10:37 +0800 > > > > Kevin Tian <kevin.tian@intel.com> wrote: > > > > > > > > > From: Yi Liu <yi.l.liu@intel.com> > > > > > > > > > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > > > > > sysfs path of the parent, indicating the device is bound to a vfio > > > > > driver, e.g.: > > > > > > > > > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > > > > > > > > > It is also a preparatory step toward adding cdev for supporting future > > > > > device-oriented uAPI. > > > > > > > > Shall we start Documentation/ABI/testing/vfio-dev now? Thanks. > > > > > > I always thought that was something to use when adding new custom > > > sysfs attributes? > > > > > > Here we are just creating a standard struct device with its standard > > > sysfs? > > > > > > > There is nothing special for vfio-dev/vfioX. But from pci device p.o.v > > this does introduce a custom node in the directory, which is probably > > what Alex referred to? > > Yup, but not just for pci, we're adding a node into the device > directory for any device bound to vfio. > > > Anyway if required following can be introduced: > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-vfio-dev > b/Documentation/ABI/testing/sysfs-devices-vfio-dev > > new file mode 100644 > > index 000000000000..dfe8baaf1ccb > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev > > @@ -0,0 +1,8 @@ > > +What: /sys/.../<device>/vfio-dev/vfioX/ > > +Date: September 2022 > > +Contact: Yi Liu <yi.l.liu@intel.com> > > +Description: > > + This directory is created when the device is bound to a > > + vfio driver. The layout under this directory matches what > > + exists for a standard 'struct device'. 'X' is a random > > + number marking this device in vfio. > > It's not really random, it's a unique index. Seems like a good > starting point. > > > > > At the start I thought it might make more sense to add it into an > > existing vfio ABI file. But looks it doesn't exist. > > > > Curious why nobody asked for ABI doc for /dev/vfio/vfio, /sys/class/vfio, > etc... > > Oversight, there should probably be a sysfs-class-vfio file. Thanks, > I can help add one. btw I plan to respin v2 tomorrow. Regarding to this ABI thing there are three options: 1) Just add sysfs-devices-vfio-dev in this series. Later merge to sysfs-class-vfio once the latter is introduced in a separate patch. 2) Do sysfs-class-vfio in this series, including both existing vfio ABIs and the new vfio-dev. 3) No ABI file in this series. Handle it in a separate patch with sysfs-class-vfio. Which one do you prefer to? Thanks Kevin
On Thu, 1 Sep 2022 00:46:51 +0000 "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Thursday, September 1, 2022 1:15 AM > > > > On Wed, 31 Aug 2022 06:10:51 +0000 > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > From: Jason Gunthorpe <jgg@ziepe.ca> > > > > Sent: Wednesday, August 31, 2022 7:53 AM > > > > > > > > On Tue, Aug 30, 2022 at 04:18:38PM -0600, Alex Williamson wrote: > > > > > On Sun, 28 Aug 2022 01:10:37 +0800 > > > > > Kevin Tian <kevin.tian@intel.com> wrote: > > > > > > > > > > > From: Yi Liu <yi.l.liu@intel.com> > > > > > > > > > > > > and replace kref. With it a 'vfio-dev/vfioX' node is created under the > > > > > > sysfs path of the parent, indicating the device is bound to a vfio > > > > > > driver, e.g.: > > > > > > > > > > > > /sys/devices/pci0000\:6f/0000\:6f\:01.0/vfio-dev/vfio0 > > > > > > > > > > > > It is also a preparatory step toward adding cdev for supporting future > > > > > > device-oriented uAPI. > > > > > > > > > > Shall we start Documentation/ABI/testing/vfio-dev now? Thanks. > > > > > > > > I always thought that was something to use when adding new custom > > > > sysfs attributes? > > > > > > > > Here we are just creating a standard struct device with its standard > > > > sysfs? > > > > > > > > > > There is nothing special for vfio-dev/vfioX. But from pci device p.o.v > > > this does introduce a custom node in the directory, which is probably > > > what Alex referred to? > > > > Yup, but not just for pci, we're adding a node into the device > > directory for any device bound to vfio. > > > > > Anyway if required following can be introduced: > > > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-vfio-dev > > b/Documentation/ABI/testing/sysfs-devices-vfio-dev > > > new file mode 100644 > > > index 000000000000..dfe8baaf1ccb > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev > > > @@ -0,0 +1,8 @@ > > > +What: /sys/.../<device>/vfio-dev/vfioX/ > > > +Date: September 2022 > > > +Contact: Yi Liu <yi.l.liu@intel.com> > > > +Description: > > > + This directory is created when the device is bound to a > > > + vfio driver. The layout under this directory matches what > > > + exists for a standard 'struct device'. 'X' is a random > > > + number marking this device in vfio. > > > > It's not really random, it's a unique index. Seems like a good > > starting point. > > > > > > > > At the start I thought it might make more sense to add it into an > > > existing vfio ABI file. But looks it doesn't exist. > > > > > > Curious why nobody asked for ABI doc for /dev/vfio/vfio, /sys/class/vfio, > > etc... > > > > Oversight, there should probably be a sysfs-class-vfio file. Thanks, > > > > I can help add one. > > btw I plan to respin v2 tomorrow. Regarding to this ABI thing there are > three options: > > 1) Just add sysfs-devices-vfio-dev in this series. Later merge to > sysfs-class-vfio once the latter is introduced in a separate patch. This. Thanks, Alex > > 2) Do sysfs-class-vfio in this series, including both existing vfio ABIs and > the new vfio-dev. > > 3) No ABI file in this series. Handle it in a separate patch with > sysfs-class-vfio. > > Which one do you prefer to? > > Thanks > Kevin >
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 0c5d120aeced..9ad0cbb83f1c 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -46,6 +46,8 @@ static struct vfio { struct mutex group_lock; /* locks group_list */ struct ida group_ida; dev_t group_devt; + struct class *device_class; + struct ida device_ida; } vfio; struct vfio_iommu_driver { @@ -524,11 +526,19 @@ EXPORT_SYMBOL_GPL(_vfio_alloc_device); * * Only vfio-ccw driver should call this interface. */ +static void vfio_device_release(struct device *dev); int vfio_init_device(struct vfio_device *device, struct device *dev, const struct vfio_device_ops *ops) { int ret; + ret = ida_alloc_max(&vfio.device_ida, MINORMASK, GFP_KERNEL); + if (ret < 0) { + dev_dbg(dev, "Error to alloc index\n"); + return ret; + } + + device->index = ret; init_completion(&device->comp); device->dev = dev; device->ops = ops; @@ -536,11 +546,18 @@ int vfio_init_device(struct vfio_device *device, struct device *dev, if (ops->init) { ret = ops->init(device); if (ret) - return ret; + goto out_ida; } - kref_init(&device->kref); + device_initialize(&device->device); + device->device.release = vfio_device_release; + device->device.class = vfio.device_class; + device->device.parent = device->dev; return 0; + +out_ida: + ida_free(&vfio.device_ida, device->index); + return ret; } EXPORT_SYMBOL_GPL(vfio_init_device); @@ -556,12 +573,13 @@ void vfio_free_device(struct vfio_device *device) EXPORT_SYMBOL_GPL(vfio_free_device); /* Release helper called by vfio_put_device() */ -void vfio_device_release(struct kref *kref) +static void vfio_device_release(struct device *dev) { struct vfio_device *device = - container_of(kref, struct vfio_device, kref); + container_of(dev, struct vfio_device, device); vfio_release_device_set(device); + ida_free(&vfio.device_ida, device->index); /* * kvfree() cannot be done here due to a life cycle mess in @@ -571,7 +589,6 @@ void vfio_device_release(struct kref *kref) */ device->ops->release(device); } -EXPORT_SYMBOL_GPL(vfio_device_release); static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev, enum vfio_group_type type) @@ -654,6 +671,7 @@ static int __vfio_register_dev(struct vfio_device *device, struct vfio_group *group) { struct vfio_device *existing_device; + int ret; if (IS_ERR(group)) return PTR_ERR(group); @@ -670,16 +688,21 @@ static int __vfio_register_dev(struct vfio_device *device, dev_WARN(device->dev, "Device already exists on group %d\n", iommu_group_id(group->iommu_group)); vfio_device_put_registration(existing_device); - if (group->type == VFIO_NO_IOMMU || - group->type == VFIO_EMULATED_IOMMU) - iommu_group_remove_device(device->dev); - vfio_group_put(group); - return -EBUSY; + ret = -EBUSY; + goto err_out; } /* Our reference on group is moved to the device */ device->group = group; + ret = dev_set_name(&device->device, "vfio%d", device->index); + if (ret) + goto err_out; + + ret = device_add(&device->device); + if (ret) + goto err_out; + /* Refcounting can't start until the driver calls register */ refcount_set(&device->refcount, 1); @@ -689,6 +712,12 @@ static int __vfio_register_dev(struct vfio_device *device, mutex_unlock(&group->device_lock); return 0; +err_out: + if (group->type == VFIO_NO_IOMMU || + group->type == VFIO_EMULATED_IOMMU) + iommu_group_remove_device(device->dev); + vfio_group_put(group); + return ret; } int vfio_register_group_dev(struct vfio_device *device) @@ -776,6 +805,9 @@ void vfio_unregister_group_dev(struct vfio_device *device) group->dev_counter--; mutex_unlock(&group->device_lock); + /* Balances device_add in register path */ + device_del(&device->device); + if (group->type == VFIO_NO_IOMMU || group->type == VFIO_EMULATED_IOMMU) iommu_group_remove_device(device->dev); @@ -2142,6 +2174,7 @@ static int __init vfio_init(void) int ret; ida_init(&vfio.group_ida); + ida_init(&vfio.device_ida); mutex_init(&vfio.group_lock); mutex_init(&vfio.iommu_drivers_lock); INIT_LIST_HEAD(&vfio.group_list); @@ -2157,11 +2190,18 @@ static int __init vfio_init(void) vfio.class = class_create(THIS_MODULE, "vfio"); if (IS_ERR(vfio.class)) { ret = PTR_ERR(vfio.class); - goto err_class; + goto err_group_class; } vfio.class->devnode = vfio_devnode; + /* /sys/class/vfio-dev/vfioX */ + vfio.device_class = class_create(THIS_MODULE, "vfio-dev"); + if (IS_ERR(vfio.device_class)) { + ret = PTR_ERR(vfio.device_class); + goto err_dev_class; + } + ret = alloc_chrdev_region(&vfio.group_devt, 0, MINORMASK + 1, "vfio"); if (ret) goto err_alloc_chrdev; @@ -2178,9 +2218,12 @@ static int __init vfio_init(void) err_driver_register: unregister_chrdev_region(vfio.group_devt, MINORMASK + 1); err_alloc_chrdev: + class_destroy(vfio.device_class); + vfio.device_class = NULL; +err_dev_class: class_destroy(vfio.class); vfio.class = NULL; -err_class: +err_group_class: misc_deregister(&vfio_dev); return ret; } @@ -2192,8 +2235,11 @@ static void __exit vfio_cleanup(void) #ifdef CONFIG_VFIO_NOIOMMU vfio_unregister_iommu_driver(&vfio_noiommu_ops); #endif + ida_destroy(&vfio.device_ida); ida_destroy(&vfio.group_ida); unregister_chrdev_region(vfio.group_devt, MINORMASK + 1); + class_destroy(vfio.device_class); + vfio.device_class = NULL; class_destroy(vfio.class); vfio.class = NULL; misc_deregister(&vfio_dev); diff --git a/include/linux/vfio.h b/include/linux/vfio.h index a36a65b966b5..ae398c2f18eb 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -45,7 +45,8 @@ struct vfio_device { struct kvm *kvm; /* Members below here are private, not for driver use */ - struct kref kref; /* object life cycle */ + unsigned int index; + struct device device; /* device.kref covers object life circle */ refcount_t refcount; /* user count on registered device*/ unsigned int open_count; struct completion comp; @@ -154,10 +155,9 @@ struct vfio_device *_vfio_alloc_device(size_t size, struct device *dev, int vfio_init_device(struct vfio_device *device, struct device *dev, const struct vfio_device_ops *ops); void vfio_free_device(struct vfio_device *device); -void vfio_device_release(struct kref *kref); static inline void vfio_put_device(struct vfio_device *device) { - kref_put(&device->kref, vfio_device_release); + put_device(&device->device); } int vfio_register_group_dev(struct vfio_device *device);