Re: [Intel-gfx] [PATCH v5 15/19] vfio: Add cdev for vfio_device
> From: Jason Gunthorpe
> Sent: Tuesday, February 28, 2023 3:06 AM
>
> On Mon, Feb 27, 2023 at 03:11:31AM -0800, Yi Liu wrote:
> > @@ -309,6 +310,13 @@ void vfio_unregister_group_dev(struct
> vfio_device *device)
> > bool interrupted = false;
> > long rc;
> >
> > + /*
> > +* Balances vfio_device_add in register path. Putting it as the
> > +* first operation in unregister to prevent registration refcount
> > +* from incrementing per cdev open.
> > +*/
> > + vfio_device_del(device);
> > +
> > vfio_device_put_registration(device);
> > rc = try_wait_for_completion(&device->comp);
> > while (rc <= 0) {
> > @@ -334,9 +342,6 @@ void vfio_unregister_group_dev(struct vfio_device
> *device)
> >
> > vfio_device_group_unregister(device);
> >
> > - /* Balances device_add in register path */
> > - device_del(&device->device);
> > -
> > /* Balances vfio_device_set_group in register path */
> > vfio_device_remove_group(device);
>
> The same rational applies to vfio_device_group_unregister too, so it
> should be moved up as well.
You are right. User may get new registration refcount in below path
which can be in parallel with this vfio_unregister_group_dev() path.
Let me move it and refine the comment as well.
ioctl(group_fd, VFIO_GROUP_GET_DEVICE_FD, )
vfio_group_ioctl_get_device_fd()
-> vfio_device_get_from_name()
-> vfio_device_try_get_registration() -- refcount++
Thanks,
Yi Liu
Re: [Intel-gfx] [PATCH v5 15/19] vfio: Add cdev for vfio_device
> From: Jason Gunthorpe > Sent: Tuesday, February 28, 2023 2:56 AM > > On Mon, Feb 27, 2023 at 03:11:31AM -0800, Yi Liu wrote: > > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig > > index a8f544629467..169762316513 100644 > > --- a/drivers/vfio/Kconfig > > +++ b/drivers/vfio/Kconfig > > @@ -12,6 +12,18 @@ 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 && (X86 || S390 || ARM || ARM64) > > We don't need to propogate this arch detection stuff, at worst it > should be in iommufd kconfig if it is really needed. Ok. this makes sense as cdev's real dependency is iommufd. Btw. Also no need for the below stuff. Is it? just select CDEV if !VFIO_GROUP. right? select VFIO_DEVICE_CDEV if !VFIO_GROUP && (X86 || S390 || ARM || ARM64) > Also that other thread shows that vfio doesn't work on ARM because we > can never take ownership of a device due to arm iommu It's interesting. May you share the link of this thread?:-) Regards, Yi Liu
Re: [Intel-gfx] [PATCH v5 15/19] vfio: Add cdev for vfio_device
On Mon, Feb 27, 2023 at 03:11:31AM -0800, Yi Liu wrote:
> @@ -309,6 +310,13 @@ void vfio_unregister_group_dev(struct vfio_device
> *device)
> bool interrupted = false;
> long rc;
>
> + /*
> + * Balances vfio_device_add in register path. Putting it as the
> + * first operation in unregister to prevent registration refcount
> + * from incrementing per cdev open.
> + */
> + vfio_device_del(device);
> +
> vfio_device_put_registration(device);
> rc = try_wait_for_completion(&device->comp);
> while (rc <= 0) {
> @@ -334,9 +342,6 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>
> vfio_device_group_unregister(device);
>
> - /* Balances device_add in register path */
> - device_del(&device->device);
> -
> /* Balances vfio_device_set_group in register path */
> vfio_device_remove_group(device);
The same rational applies to vfio_device_group_unregister too, so it
should be moved up as well.
Jason
Re: [Intel-gfx] [PATCH v5 15/19] vfio: Add cdev for vfio_device
On Mon, Feb 27, 2023 at 03:11:31AM -0800, Yi Liu wrote: > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig > index a8f544629467..169762316513 100644 > --- a/drivers/vfio/Kconfig > +++ b/drivers/vfio/Kconfig > @@ -12,6 +12,18 @@ 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 && (X86 || S390 || ARM || ARM64) We don't need to propogate this arch detection stuff, at worst it should be in iommufd kconfig if it is really needed. Also that other thread shows that vfio doesn't work on ARM because we can never take ownership of a device due to arm iommu Jason
[Intel-gfx] [PATCH v5 15/19] 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 (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;
*) vfio_unregister_group_dev() path does cdev_device_del();
Signed-off-by: Yi Liu
Reviewed-by: Kevin Tian
---
drivers/vfio/Kconfig | 12
drivers/vfio/Makefile | 1 +
drivers/vfio/device_cdev.c | 63 ++
drivers/vfio/vfio.h| 46
drivers/vfio/vfio_main.c | 22 ++---
include/linux/vfio.h | 4 +++
6 files changed, 144 insertions(+), 4 deletions(-)
create mode 100644 drivers/vfio/device_cdev.c
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index a8f544629467..169762316513 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -12,6 +12,18 @@ 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 && (X86 || S390 || ARM || ARM64)
+ 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 context for device access. It is not available for
+ SPAPR_TCE_IOMMU.
+
+ 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 ..9e2c1ecaaf4f
--- /dev/null
+++ b/drivers/vfio/device_cdev.c
@@ -0,0 +1,63 @@
+// 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(&device->cdev, &vfio_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;
+ }
+
+ df->is_cdev_device = true;
+ 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(&device_devt, 0,
+ MINORMASK + 1, "vfio-dev");
+}
+
+void vfio_cdev_cleanup(void)
+{
+ unregister_chrdev_region(device_devt, MINORMASK + 1);
+}
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index
