Re: [Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
On Thu, 1 Sep 2022 00:46:51 + "Tian, Kevin" wrote: > > From: Alex Williamson > > Sent: Thursday, September 1, 2022 1:15 AM > > > > On Wed, 31 Aug 2022 06:10:51 + > > "Tian, Kevin" wrote: > > > > > > From: Jason Gunthorpe > > > > 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 wrote: > > > > > > > > > > > From: Yi Liu > > > > > > > > > > > > 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/pci\:6f/\: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 ..dfe8baaf1ccb > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev > > > @@ -0,0 +1,8 @@ > > > +What: /sys/...//vfio-dev/vfioX/ > > > +Date: September 2022 > > > +Contact: Yi Liu > > > +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 >
Re: [Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
> From: Alex Williamson > Sent: Thursday, September 1, 2022 1:15 AM > > On Wed, 31 Aug 2022 06:10:51 + > "Tian, Kevin" wrote: > > > > From: Jason Gunthorpe > > > 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 wrote: > > > > > > > > > From: Yi Liu > > > > > > > > > > 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/pci\:6f/\: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 ..dfe8baaf1ccb > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev > > @@ -0,0 +1,8 @@ > > +What: /sys/...//vfio-dev/vfioX/ > > +Date: September 2022 > > +Contact:Yi Liu > > +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
Re: [Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
On Wed, 31 Aug 2022 06:10:51 + "Tian, Kevin" wrote: > > From: Jason Gunthorpe > > 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 wrote: > > > > > > > From: Yi Liu > > > > > > > > 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/pci\:6f/\: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 ..dfe8baaf1ccb > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev > @@ -0,0 +1,8 @@ > +What: /sys/...//vfio-dev/vfioX/ > +Date: September 2022 > +Contact: Yi Liu > +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
Re: [Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
> From: Jason Gunthorpe
> Sent: Wednesday, August 31, 2022 8:32 AM
>
> On Sun, Aug 28, 2022 at 01:10:37AM +0800, Kevin Tian wrote:
> > From: Yi Liu
> >
> > 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/pci\:6f/\: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
> > Signed-off-by: Yi Liu
> > Signed-off-by: Kevin Tian
> > ---
> > 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 mutexgroup_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.
Re: [Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
> From: Jason Gunthorpe > 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 wrote: > > > > > From: Yi Liu > > > > > > 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/pci\:6f/\: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 ..dfe8baaf1ccb --- /dev/null +++ b/Documentation/ABI/testing/sysfs-devices-vfio-dev @@ -0,0 +1,8 @@ +What: /sys/...//vfio-dev/vfioX/ +Date: September 2022 +Contact:Yi Liu +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
Re: [Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
On Sun, 28 Aug 2022 01:10:37 +0800 Kevin Tian wrote: > From: Yi Liu > > 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/pci\:6f/\: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
[Intel-gfx] [PATCH 15/15] vfio: Add struct device to vfio_device
From: Yi Liu
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/pci\:6f/\: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
Signed-off-by: Yi Liu
Signed-off-by: Kevin Tian
---
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 mutexgroup_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(s
