Re: [Intel-gfx] [PATCH v8 20/24] vfio: Add cdev for vfio_device
> From: Alex Williamson
> Sent: Thursday, March 30, 2023 3:57 AM
>
> On Mon, 27 Mar 2023 02:40:43 -0700
> Yi Liu wrote:
>
[...]
> > +/*
> > + * device access via the fd opened by this function is blocked until
> > + * .open_device() is called successfully during BIND_IOMMUFD.
> > + */
> > +int vfio_device_fops_cdev_open(struct inode *inode, struct file *filep)
> > +{
> > + struct vfio_device *device = container_of(inode->i_cdev,
> > + struct vfio_device, cdev);
> > + struct vfio_device_file *df;
> > + int ret;
> > +
> > + if (!vfio_device_try_get_registration(device))
> > + return -ENODEV;
> > +
> > + df = vfio_allocate_device_file(device);
> > + if (IS_ERR(df)) {
> > + ret = PTR_ERR(df);
> > + goto err_put_registration;
> > + }
> > +
> > + filep->private_data = df;
> > +
> > + return 0;
> > +
> > +err_put_registration:
> > + vfio_device_put_registration(device);
> > + return ret;
> > +}
> > +
[...]
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index 8e96aab27029..58fc3bb768f2 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -242,6 +242,7 @@ static int vfio_init_device(struct vfio_device *device,
> > struct
> device *dev,
> > device->device.release = vfio_device_release;
> > device->device.class = vfio.device_class;
> > device->device.parent = device->dev;
> > + vfio_init_device_cdev(device);
> > return 0;
> >
> > out_uninit:
> > @@ -280,7 +281,7 @@ static int __vfio_register_dev(struct vfio_device
> > *device,
> > if (ret)
> > goto err_out;
> >
> > - ret = device_add(>device);
> > + ret = vfio_device_add(device);
> > if (ret)
> > goto err_out;
> >
> > @@ -320,6 +321,12 @@ void vfio_unregister_group_dev(struct vfio_device
> > *device)
> > bool interrupted = false;
> > long rc;
> >
> > + /* Prevent new device opened in the group path */
> > + vfio_device_group_unregister(device);
> > +
> > + /* Prevent new device opened in the cdev path */
> > + vfio_device_del(device);
> > +
> > vfio_device_put_registration(device);
> > rc = try_wait_for_completion(>comp);
> > while (rc <= 0) {
> > @@ -343,11 +350,6 @@ void vfio_unregister_group_dev(struct vfio_device
> > *device)
> > }
> > }
> >
> > - vfio_device_group_unregister(device);
> > -
> > - /* Balances device_add in register path */
> > - device_del(>device);
> > -
>
> Why were these relocated? And additionally why was the comment
> regarding the balance operations dropped? The move seems unrelated to
> the patch here, so if it's actually advisable for some reason, it
> should be a separate patch. Thanks,
The reason for the relocation is to prevent new device which would result
in the device->refcount increasing. If the user keeps open device then the
device->refcount may keep increasing. Then the vfio_unregister_group_dev()
may be stuck here. This is rare, but possible.
By doing vfio_device_group_unregister(), the device is removed from the
group->device_list. Then user cannot open the device by
VFIO_GROUP_GET_DEVICE_FD.
Hence it won't increase the device->refcount. I agree with you, this should
be done in a separate patch.
Same reason for relocating device_del(>device); User may keep
opening the cdev to increase the device->refcount. Then the
vfio_device_group_unregister() path would be stuck as well. But this
relocation needs to be done here since user cannot do it if without cdev.
Last, need to keep the balance comment as well even the sequence
it not strictly mirrored. will keep the comment.
> Alex
>
> > /* Balances vfio_device_set_group in register path */
> > vfio_device_remove_group(device);
> > }
> > @@ -555,7 +557,8 @@ static int vfio_device_fops_release(struct inode *inode,
> struct file *filep)
> > struct vfio_device_file *df = filep->private_data;
> > struct vfio_device *device = df->device;
> >
> > - vfio_device_group_close(df);
> > + if (df->group)
> > + vfio_device_group_close(df);
> >
> > vfio_device_put_registration(device);
> >
Thanks,
Yi Liu
Re: [Intel-gfx] [PATCH v8 20/24] vfio: Add cdev for vfio_device
On Mon, 27 Mar 2023 02:40:43 -0700
Yi Liu wrote:
> This allows user to directly open a vfio device w/o using the legacy
> container/group interface, as a prerequisite for supporting new iommu
> features like nested translation.
>
> The device fd opened in this manner doesn't have the capability to access
> the device as the fops open() doesn't open the device until the successful
> BIND_IOMMUFD which be added in next patch.
>
> With this patch, devices registered to vfio core have both group and device
> interface created.
>
> - group interface : /dev/vfio/$groupID
> - device interface: /dev/vfio/devices/vfioX - normal device
> /dev/vfio/devices/noiommu-vfioX - noiommu device
> ("X" is the minor number and is unique across devices)
>
> Given a vfio device the user can identify the matching vfioX by checking
> the sysfs path of the device. Take PCI device (:6a:01.0) for example,
> /sys/bus/pci/devices/\:6a\:01.0/vfio-dev/vfio0/dev contains the
> major:minor of the matching vfioX.
>
> Userspace then opens the /dev/vfio/devices/vfioX and checks with fstat
> that the major:minor matches.
>
> The vfio_device cdev logic in this patch:
> *) __vfio_register_dev() path ends up doing cdev_device_add() for each
>vfio_device if VFIO_DEVICE_CDEV configured.
> *) vfio_unregister_group_dev() path does cdev_device_del();
>
> Reviewed-by: Kevin Tian
> Tested-by: Terrence Xu
> Tested-by: Nicolin Chen
> Tested-by: Matthew Rosato
> Signed-off-by: Yi Liu
> ---
> drivers/vfio/Kconfig | 11 +++
> drivers/vfio/Makefile | 1 +
> drivers/vfio/device_cdev.c | 62 ++
> drivers/vfio/vfio.h| 46
> drivers/vfio/vfio_main.c | 26 +++-
> include/linux/vfio.h | 4 +++
> 6 files changed, 143 insertions(+), 7 deletions(-)
> create mode 100644 drivers/vfio/device_cdev.c
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 89e06c981e43..e2105b4dac2d 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -12,6 +12,17 @@ menuconfig VFIO
> If you don't know what to do here, say N.
>
> if VFIO
> +config VFIO_DEVICE_CDEV
> + bool "Support for the VFIO cdev /dev/vfio/devices/vfioX"
> + depends on IOMMUFD
> + help
> + The VFIO device cdev is another way for userspace to get device
> + access. Userspace gets device fd by opening device cdev under
> + /dev/vfio/devices/vfioX, and then bind the device fd with an iommufd
> + to set up secure DMA context for device access.
> +
> + If you don't know what to do here, say N.
> +
> config VFIO_CONTAINER
> bool "Support for the VFIO container /dev/vfio/vfio"
> select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64)
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 70e7dcb302ef..245394aeb94b 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_VFIO) += vfio.o
> vfio-y += vfio_main.o \
> group.o \
> iova_bitmap.o
> +vfio-$(CONFIG_VFIO_DEVICE_CDEV) += device_cdev.o
> vfio-$(CONFIG_IOMMUFD) += iommufd.o
> vfio-$(CONFIG_VFIO_CONTAINER) += container.o
> vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> new file mode 100644
> index ..1c640016a824
> --- /dev/null
> +++ b/drivers/vfio/device_cdev.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Intel Corporation.
> + */
> +#include
> +
> +#include "vfio.h"
> +
> +static dev_t device_devt;
> +
> +void vfio_init_device_cdev(struct vfio_device *device)
> +{
> + device->device.devt = MKDEV(MAJOR(device_devt), device->index);
> + cdev_init(>cdev, _device_fops);
> + device->cdev.owner = THIS_MODULE;
> +}
> +
> +/*
> + * device access via the fd opened by this function is blocked until
> + * .open_device() is called successfully during BIND_IOMMUFD.
> + */
> +int vfio_device_fops_cdev_open(struct inode *inode, struct file *filep)
> +{
> + struct vfio_device *device = container_of(inode->i_cdev,
> + struct vfio_device, cdev);
> + struct vfio_device_file *df;
> + int ret;
> +
> + if (!vfio_device_try_get_registration(device))
> + return -ENODEV;
> +
> + df = vfio_allocate_device_file(device);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto err_put_registration;
> + }
> +
> + filep->private_data = df;
> +
> + return 0;
> +
> +err_put_registration:
> + vfio_device_put_registration(device);
> + return ret;
> +}
> +
> +static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
> +{
> + return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> +}
> +
> +int vfio_cdev_init(struct class *device_class)
> +{
> + device_class->devnode =
[Intel-gfx] [PATCH v8 20/24] vfio: Add cdev for vfio_device
This allows user to directly open a vfio device w/o using the legacy
container/group interface, as a prerequisite for supporting new iommu
features like nested translation.
The device fd opened in this manner doesn't have the capability to access
the device as the fops open() doesn't open the device until the successful
BIND_IOMMUFD which be added in next patch.
With this patch, devices registered to vfio core have both group and device
interface created.
- group interface : /dev/vfio/$groupID
- device interface: /dev/vfio/devices/vfioX - normal device
/dev/vfio/devices/noiommu-vfioX - noiommu device
("X" is the minor number and is unique across devices)
Given a vfio device the user can identify the matching vfioX by checking
the sysfs path of the device. Take PCI device (:6a:01.0) for example,
/sys/bus/pci/devices/\:6a\:01.0/vfio-dev/vfio0/dev contains the
major:minor of the matching vfioX.
Userspace then opens the /dev/vfio/devices/vfioX and checks with fstat
that the major:minor matches.
The vfio_device cdev logic in this patch:
*) __vfio_register_dev() path ends up doing cdev_device_add() for each
vfio_device if VFIO_DEVICE_CDEV configured.
*) vfio_unregister_group_dev() path does cdev_device_del();
Reviewed-by: Kevin Tian
Tested-by: Terrence Xu
Tested-by: Nicolin Chen
Tested-by: Matthew Rosato
Signed-off-by: Yi Liu
---
drivers/vfio/Kconfig | 11 +++
drivers/vfio/Makefile | 1 +
drivers/vfio/device_cdev.c | 62 ++
drivers/vfio/vfio.h| 46
drivers/vfio/vfio_main.c | 26 +++-
include/linux/vfio.h | 4 +++
6 files changed, 143 insertions(+), 7 deletions(-)
create mode 100644 drivers/vfio/device_cdev.c
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 89e06c981e43..e2105b4dac2d 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -12,6 +12,17 @@ menuconfig VFIO
If you don't know what to do here, say N.
if VFIO
+config VFIO_DEVICE_CDEV
+ bool "Support for the VFIO cdev /dev/vfio/devices/vfioX"
+ depends on IOMMUFD
+ help
+ The VFIO device cdev is another way for userspace to get device
+ access. Userspace gets device fd by opening device cdev under
+ /dev/vfio/devices/vfioX, and then bind the device fd with an iommufd
+ to set up secure DMA context for device access.
+
+ If you don't know what to do here, say N.
+
config VFIO_CONTAINER
bool "Support for the VFIO container /dev/vfio/vfio"
select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64)
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 70e7dcb302ef..245394aeb94b 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_VFIO) += vfio.o
vfio-y += vfio_main.o \
group.o \
iova_bitmap.o
+vfio-$(CONFIG_VFIO_DEVICE_CDEV) += device_cdev.o
vfio-$(CONFIG_IOMMUFD) += iommufd.o
vfio-$(CONFIG_VFIO_CONTAINER) += container.o
vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
new file mode 100644
index ..1c640016a824
--- /dev/null
+++ b/drivers/vfio/device_cdev.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Intel Corporation.
+ */
+#include
+
+#include "vfio.h"
+
+static dev_t device_devt;
+
+void vfio_init_device_cdev(struct vfio_device *device)
+{
+ device->device.devt = MKDEV(MAJOR(device_devt), device->index);
+ cdev_init(>cdev, _device_fops);
+ device->cdev.owner = THIS_MODULE;
+}
+
+/*
+ * device access via the fd opened by this function is blocked until
+ * .open_device() is called successfully during BIND_IOMMUFD.
+ */
+int vfio_device_fops_cdev_open(struct inode *inode, struct file *filep)
+{
+ struct vfio_device *device = container_of(inode->i_cdev,
+ struct vfio_device, cdev);
+ struct vfio_device_file *df;
+ int ret;
+
+ if (!vfio_device_try_get_registration(device))
+ return -ENODEV;
+
+ df = vfio_allocate_device_file(device);
+ if (IS_ERR(df)) {
+ ret = PTR_ERR(df);
+ goto err_put_registration;
+ }
+
+ filep->private_data = df;
+
+ return 0;
+
+err_put_registration:
+ vfio_device_put_registration(device);
+ return ret;
+}
+
+static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
+{
+ return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
+}
+
+int vfio_cdev_init(struct class *device_class)
+{
+ device_class->devnode = vfio_device_devnode;
+ return alloc_chrdev_region(_devt, 0,
+ MINORMASK + 1, "vfio-dev");
+}
+
+void vfio_cdev_cleanup(void)
+{
+ unregister_chrdev_region(device_devt, MINORMASK + 1);
+}
diff --git
