[PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov]

This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.

Currently uio_pci_generic demands INT#x interrupts source be available. However
there are devices that simply don't have INT#x capability, for instance SR-IOV
VF devices that simply don't have INT#x capability. For such devices
uio_pci_generic will simply fail (more specifically its probe() will fail).

When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to
performance overhead and thus VFIO is not an option users that develop
user-space drivers are left without any option but to develop some proprietary
UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to use UIO
infrastructure.

This series provides a generic solution for this problem while preserving the
original behaviour for devices for which the original uio_pci_generic had worked
before (i.e. INT#x will be used by default).

New in v5:
   - Expanded the commitlog on PATCH1.

New in v4:
   - Use portable __u32 and __s32 types from asm/types.h for
     defining uio_pci_generic_irq_set fields.
   - Use proper _IO macros for defining read and write ioctl()
     commands.
   - Moved bars mapping setting into a separate patch.
   - Update uio_pci_generic example in uio-howto.tmpl.

New in v3:
   - Add __iomem qualifier to temp buffer receiving ioremap value.  
 
New in v2:
   - Added #include <linux/uaccess.h> to uio_pci_generic.c


Vlad Zolotarov (4):
  uio: add ioctl support
  uio_pci_generic: properly initialize PCI bars mappings towards UIO
  uio_pci_generic: add MSI/MSI-X support
  Documentation: update uio-howto

 Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
 drivers/uio/uio.c                    |  15 ++
 drivers/uio/uio_pci_generic.c        | 409 +++++++++++++++++++++++++++++++++--
 include/linux/uio_driver.h           |   3 +
 include/uapi/linux/uio_pci_generic.h |  51 +++++
 5 files changed, 574 insertions(+), 43 deletions(-)
 create mode 100644 include/uapi/linux/uio_pci_generic.h

-- 
2.1.0

[PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

Add the ability for underlying device drivers to register the ioctl
commands. This is useful when some interaction with the user space
beyond sysfs capabilities is required, e.g. query the interrupt mode
or bind eventfd to interrupt notifications (similarly to vfio ioctl
VFIO_DEVICE_SET_IRQS).

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/uio/uio.c          | 15 +++++++++++++++
 include/linux/uio_driver.h |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 8196581..714b0e5 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	}
 }
 
+static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+	struct uio_listener *listener = filep->private_data;
+	struct uio_device *idev = listener->dev;
+
+	if (!idev->info)
+		return -EIO;
+
+	if (!idev->info->ioctl)
+		return -ENOTTY;
+
+	return idev->info->ioctl(idev->info, cmd, arg);
+}
+
 static const struct file_operations uio_fops = {
 	.owner		= THIS_MODULE,
 	.open		= uio_open,
@@ -712,6 +726,7 @@ static const struct file_operations uio_fops = {
 	.write		= uio_write,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
+	.unlocked_ioctl	= uio_ioctl,
 	.fasync		= uio_fasync,
 	.llseek		= noop_llseek,
 };
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83..10d7833 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -89,6 +89,7 @@ struct uio_device {
  * @mmap:		mmap operation for this uio device
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
+ * @ioctl:		ioctl handler
  * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
  */
 struct uio_info {
@@ -105,6 +106,8 @@ struct uio_info {
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+	int (*ioctl)(struct uio_info *info, unsigned int cmd,
+		     unsigned long arg);
 };
 
 extern int __must_check
-- 
2.1.0

[PATCH v5 2/4] uio_pci_generic: properly initialize PCI bars mappings towards UIO

[Vlad Zolotarov]

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/uio/uio_pci_generic.c | 89 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index d0b508b..2c6e2b1 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -40,6 +40,75 @@ to_uio_pci_generic_dev(struct uio_info *info)
 	return container_of(info, struct uio_pci_generic_dev, info);
 }
 
+/* Unmap previously ioremap'd resources */
+static void release_iomaps(struct uio_pci_generic_dev *gdev)
+{
+	int i;
+	struct uio_mem *mem = gdev->info.mem;
+
+	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
+		if (mem->internal_addr) {
+			iounmap(mem->internal_addr);
+			mem->internal_addr = NULL;
+		}
+	}
+}
+
+static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
+{
+	int i, m = 0, p = 0, err;
+	static const char * const bar_names[] = {
+		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
+	};
+
+	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
+		unsigned long start = pci_resource_start(pdev, i);
+		unsigned long flags = pci_resource_flags(pdev, i);
+		unsigned long len = pci_resource_len(pdev, i);
+
+		if (start == 0 || len == 0)
+			continue;
+
+		if (flags & IORESOURCE_MEM) {
+			void __iomem *addr;
+
+			if (m >= MAX_UIO_MAPS)
+				continue;
+
+			addr = ioremap(start, len);
+			if (addr == NULL) {
+				err = -EINVAL;
+				goto fail;
+			}
+
+			info->mem[m].name = bar_names[i];
+			info->mem[m].addr = start;
+			info->mem[m].internal_addr = addr;
+			info->mem[m].size = len;
+			info->mem[m].memtype = UIO_MEM_PHYS;
+			++m;
+		} else if (flags & IORESOURCE_IO) {
+			if (p >= MAX_UIO_PORT_REGIONS)
+				continue;
+
+			info->port[p].name = bar_names[i];
+			info->port[p].start = start;
+			info->port[p].size = len;
+			info->port[p].porttype = UIO_PORT_X86;
+			++p;
+		}
+	}
+
+	return 0;
+fail:
+	for (i = 0; i < m; i++) {
+		iounmap(info->mem[i].internal_addr);
+		info->mem[i].internal_addr = NULL;
+	}
+
+	return err;
+}
+
 /* Interrupt handler. Read/modify/write the command register to disable
  * the interrupt. */
 static irqreturn_t irqhandler(int irq, struct uio_info *info)
@@ -86,18 +155,35 @@ static int probe(struct pci_dev *pdev,
 
 	gdev->info.name = "uio_pci_generic";
 	gdev->info.version = DRIVER_VERSION;
+
+	err = pci_request_regions(pdev, "uio_pci_generic");
+	if (err != 0) {
+		dev_err(&pdev->dev, "Cannot request regions\n");
+		goto err_request_regions;
+	}
+
 	gdev->info.irq = pdev->irq;
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
 
+	/* remap resources */
+	err = setup_maps(pdev, &gdev->info);
+	if (err)
+		goto err_maps;
+
 	err = uio_register_device(&pdev->dev, &gdev->info);
 	if (err)
 		goto err_register;
 	pci_set_drvdata(pdev, gdev);
 
 	return 0;
+
 err_register:
+	release_iomaps(gdev);
+err_maps:
+	pci_release_regions(pdev);
+err_request_regions:
 	kfree(gdev);
 err_alloc:
 err_verify:
@@ -110,8 +196,11 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	release_iomaps(gdev);
+	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
+	pci_set_drvdata(pdev, NULL);
 }
 
 static struct pci_driver uio_pci_driver = {
-- 
2.1.0

[PATCH v5 3/4] uio_pci_generic: add MSI/MSI-X support

[Vlad Zolotarov]

Add support for MSI and MSI-X interrupt modes:
   - Interrupt mode selection order is:
        INT#X (for backward compatibility) -> MSI-X -> MSI.
   - Add ioctl() commands:
      - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode.
      - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs.
      - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio).
      - See an example in Documentation/DocBook/uio-howto.

Currently uio_pci_generic demands INT#x interrupts source be available.
However there are devices that simply don't have INT#x capability, for
instance SR-IOV VF devices that simply don't have INT#x capability. For
such devices uio_pci_generic will simply fail (more specifically its
probe() will fail).

When IOMMU is either not available (e.g. Amazon EC2) or not acceptable
due to performance overhead and thus VFIO is not an option users that
develop user-space drivers are left without any option but to develop
some proprietary UIO drivers (e.g. igb_uio driver in Intel's DPDK) just
to be able to use UIO infrastructure.

This series provides a generic solution for this problem while
preserving the original behaviour for devices for which the original
uio_pci_generic had worked before (i.e. INT#x will be used by default).

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v4:
   - Use portable __u32 and __s32 types from asm/types.h for
     defining uio_pci_generic_irq_set fields.
   - Use proper _IO macros for defining read and write ioctl()
     commands.
   - Moved bars mapping setting into a separate patch.

New in v3:
   - Add __iomem qualifier to temp buffer receiving ioremap value.

New in v2:
   - Added #include <linux/uaccess.h> to uio_pci_generic.c
---
 drivers/uio/uio_pci_generic.c        | 320 ++++++++++++++++++++++++++++++++---
 include/uapi/linux/uio_pci_generic.h |  51 ++++++
 2 files changed, 348 insertions(+), 23 deletions(-)
 create mode 100644 include/uapi/linux/uio_pci_generic.h

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index 2c6e2b1..be92918 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -22,16 +22,32 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/msi.h>
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
+#include <linux/eventfd.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/uio_pci_generic.h>
 
 #define DRIVER_VERSION	"0.01.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
 #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
 
+struct msix_info {
+	u32 num_irqs;
+	struct msix_entry *table;
+	struct uio_msix_irq_ctx {
+		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
+		char *name;			/* name in /proc/interrupts */
+	} *ctx;
+};
+
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
+	struct mutex msix_state_lock;		/* ioctl mutex */
+	enum uio_int_mode int_mode;
+	struct msix_info msix;
 };
 
 static inline struct uio_pci_generic_dev *
@@ -109,9 +125,107 @@ fail:
 	return err;
 }
 
-/* Interrupt handler. Read/modify/write the command register to disable
- * the interrupt. */
-static irqreturn_t irqhandler(int irq, struct uio_info *info)
+static irqreturn_t msix_irqhandler(int irq, void *arg);
+
+/* set the mapping between vector # and existing eventfd. */
+static int set_irq_eventfd(struct uio_pci_generic_dev *gdev, u32 vec, int fd)
+{
+	struct uio_msix_irq_ctx *ctx;
+	struct eventfd_ctx *trigger;
+	struct pci_dev *pdev = gdev->pdev;
+	int irq, err;
+
+	if (vec >= gdev->msix.num_irqs) {
+		dev_notice(&gdev->pdev->dev, "vec %u >= num_vec %u\n",
+			   vec, gdev->msix.num_irqs);
+		return -ERANGE;
+	}
+
+	irq = gdev->msix.table[vec].vector;
+
+	/* Cleanup existing irq mapping */
+	ctx = &gdev->msix.ctx[vec];
+	if (ctx->trigger) {
+		free_irq(irq, ctx->trigger);
+		eventfd_ctx_put(ctx->trigger);
+		ctx->trigger = NULL;
+	}
+
+	/* Passing a negative value is used to unbind from the interrupt */
+	if (fd < 0)
+		return 0;
+
+
+	trigger = eventfd_ctx_fdget(fd);
+	if (IS_ERR(trigger)) {
+		err = PTR_ERR(trigger);
+		dev_notice(&gdev->pdev->dev,
+			   "eventfd ctx get failed: %d\n", err);
+		return err;
+	}
+
+	err = request_irq(irq, msix_irqhandler, 0, ctx->name, trigger);
+	if (err) {
+		dev_notice(&pdev->dev, "request irq failed: %d\n", err);
+		eventfd_ctx_put(trigger);
+		return err;
+	}
+
+	dev_dbg(&pdev->dev, "map vector %u to fd %d trigger %p\n",
+		vec, fd, trigger);
+	ctx->trigger = trigger;
+
+	return 0;
+}
+
+static int uio_pci_generic_ioctl(struct uio_info *info, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+	struct uio_pci_generic_irq_set hdr;
+	int err;
+
+	switch (cmd) {
+	case UIO_PCI_GENERIC_IRQ_SET:
+		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
+			return -EFAULT;
+
+		/* Locking is needed to ensure two IRQ_SET ioctl()'s are not
+		 * running in parallel.
+		 */
+		mutex_lock(&gdev->msix_state_lock);
+		if (gdev->int_mode != UIO_INT_MODE_MSIX) {
+			mutex_unlock(&gdev->msix_state_lock);
+			return -EOPNOTSUPP;
+		}
+
+		err = set_irq_eventfd(gdev, hdr.vec, (int)hdr.fd);
+		mutex_unlock(&gdev->msix_state_lock);
+
+		break;
+	case UIO_PCI_GENERIC_IRQ_NUM_GET:
+		if (gdev->int_mode == UIO_INT_MODE_NONE)
+			err = put_user(0, (u32 __user *)arg);
+		else if (gdev->int_mode != UIO_INT_MODE_MSIX)
+			err = put_user(1, (u32 __user *)arg);
+		else
+			err = put_user(gdev->msix.num_irqs,
+				       (u32 __user *)arg);
+
+		break;
+	case UIO_PCI_GENERIC_INT_MODE_GET:
+		err = put_user(gdev->int_mode, (u32 __user *)arg);
+
+		break;
+	default:
+		err = -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
+/* INT#X interrupt handler. */
+static irqreturn_t intx_irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
 
@@ -122,8 +236,162 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 	return IRQ_HANDLED;
 }
 
-static int probe(struct pci_dev *pdev,
-			   const struct pci_device_id *id)
+/* MSI interrupt handler. */
+static irqreturn_t msi_irqhandler(int irq, struct uio_info *info)
+{
+	/* UIO core will signal the user process. */
+	return IRQ_HANDLED;
+}
+
+/* MSI-X interrupt handler. */
+static irqreturn_t msix_irqhandler(int irq, void *arg)
+{
+	struct eventfd_ctx *trigger = arg;
+
+	pr_devel("irq %u trigger %p\n", irq, trigger);
+
+	eventfd_signal(trigger, 1);
+	return IRQ_HANDLED;
+}
+
+static bool enable_intx(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+
+	if (!pdev->irq || !pci_intx_mask_supported(pdev))
+		return false;
+
+	gdev->int_mode = UIO_INT_MODE_INTX;
+	gdev->info.irq = pdev->irq;
+	gdev->info.irq_flags = IRQF_SHARED;
+	gdev->info.handler = intx_irqhandler;
+
+	return true;
+}
+
+static void set_pci_master(struct pci_dev *pdev)
+{
+	pci_set_master(pdev);
+	dev_warn(&pdev->dev, "Enabling PCI bus mastering. Bogus userspace application is able to trash kernel memory using DMA");
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+}
+
+static bool enable_msi(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+
+	set_pci_master(pdev);
+
+	if (pci_enable_msi(pdev))
+		return false;
+
+	gdev->int_mode = UIO_INT_MODE_MSI;
+	gdev->info.irq = pdev->irq;
+	gdev->info.irq_flags = 0;
+	gdev->info.handler = msi_irqhandler;
+
+	return true;
+}
+
+static bool enable_msix(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+	int i, vectors = pci_msix_vec_count(pdev);
+
+	if (vectors <= 0)
+		return false;
+
+	gdev->msix.table = kcalloc(vectors, sizeof(struct msix_entry),
+				   GFP_KERNEL);
+	if (!gdev->msix.table) {
+		dev_err(&pdev->dev, "Failed to allocate memory for MSI-X table");
+		return false;
+	}
+
+	gdev->msix.ctx = kcalloc(vectors, sizeof(struct uio_msix_irq_ctx),
+				 GFP_KERNEL);
+	if (!gdev->msix.ctx) {
+		dev_err(&pdev->dev, "Failed to allocate memory for MSI-X contexts");
+		goto err_ctx_alloc;
+	}
+
+	for (i = 0; i < vectors; i++) {
+		gdev->msix.table[i].entry = i;
+		gdev->msix.ctx[i].name = kasprintf(GFP_KERNEL,
+						   KBUILD_MODNAME "[%d](%s)",
+						   i, pci_name(pdev));
+		if (!gdev->msix.ctx[i].name)
+			goto err_name_alloc;
+	}
+
+	set_pci_master(pdev);
+
+	if (pci_enable_msix(pdev, gdev->msix.table, vectors))
+		goto err_msix_enable;
+
+	gdev->int_mode = UIO_INT_MODE_MSIX;
+	gdev->info.irq = UIO_IRQ_CUSTOM;
+	gdev->msix.num_irqs = (u32)vectors;
+
+	return true;
+
+err_msix_enable:
+	pci_clear_master(pdev);
+err_name_alloc:
+	for (i = 0; i < vectors; i++)
+		kfree(gdev->msix.ctx[i].name);
+
+	kfree(gdev->msix.ctx);
+err_ctx_alloc:
+	kfree(gdev->msix.table);
+
+	return false;
+}
+
+/**
+ * Disable interrupts and free related resources.
+ *
+ * @gdev device handle
+ *
+ * This function should be called after the corresponding UIO device has been
+ * unregistered. This will ensure that there are no currently running ioctl()s
+ * and there won't be any new ones until next probe() call.
+ */
+static void disable_intr(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+	int i;
+
+	switch (gdev->int_mode) {
+	case UIO_INT_MODE_MSI:
+		pci_disable_msi(pdev);
+		pci_clear_master(pdev);
+
+		break;
+	case UIO_INT_MODE_MSIX:
+		/* No need for locking here since there shouldn't be any
+		 * ioctl()s running by now.
+		 */
+		for (i = 0; i < gdev->msix.num_irqs; i++) {
+			if (gdev->msix.ctx[i].trigger)
+				set_irq_eventfd(gdev, i, -1);
+
+			kfree(gdev->msix.ctx[i].name);
+		}
+
+		pci_disable_msix(pdev);
+		pci_clear_master(pdev);
+		kfree(gdev->msix.ctx);
+		kfree(gdev->msix.table);
+
+		break;
+	default:
+		break;
+	}
+}
+
+
+static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
@@ -135,26 +403,17 @@ static int probe(struct pci_dev *pdev,
 		return err;
 	}
 
-	if (!pdev->irq) {
-		dev_warn(&pdev->dev, "No IRQ assigned to device: "
-			 "no support for interrupts?\n");
-		pci_disable_device(pdev);
-		return -ENODEV;
-	}
-
-	if (!pci_intx_mask_supported(pdev)) {
-		err = -ENODEV;
-		goto err_verify;
-	}
-
 	gdev = kzalloc(sizeof(struct uio_pci_generic_dev), GFP_KERNEL);
 	if (!gdev) {
 		err = -ENOMEM;
 		goto err_alloc;
 	}
 
+	gdev->pdev = pdev;
 	gdev->info.name = "uio_pci_generic";
 	gdev->info.version = DRIVER_VERSION;
+	gdev->info.ioctl = uio_pci_generic_ioctl;
+	mutex_init(&gdev->msix_state_lock);
 
 	err = pci_request_regions(pdev, "uio_pci_generic");
 	if (err != 0) {
@@ -162,10 +421,21 @@ static int probe(struct pci_dev *pdev,
 		goto err_request_regions;
 	}
 
-	gdev->info.irq = pdev->irq;
-	gdev->info.irq_flags = IRQF_SHARED;
-	gdev->info.handler = irqhandler;
-	gdev->pdev = pdev;
+	/* Enable the corresponding interrupt mode. Try to enable INT#X first
+	 * for backward compatibility.
+	 */
+	if (enable_intx(gdev))
+		dev_info(&pdev->dev, "Using INT#x mode: IRQ %ld",
+			 gdev->info.irq);
+	else if (enable_msix(gdev))
+		dev_info(&pdev->dev, "Using MSI-X mode: number of IRQs %d",
+			 gdev->msix.num_irqs);
+	else if (enable_msi(gdev))
+		dev_info(&pdev->dev, "Using MSI mode: IRQ %ld", gdev->info.irq);
+	else {
+		err = -ENODEV;
+		goto err_verify;
+	}
 
 	/* remap resources */
 	err = setup_maps(pdev, &gdev->info);
@@ -175,6 +445,7 @@ static int probe(struct pci_dev *pdev,
 	err = uio_register_device(&pdev->dev, &gdev->info);
 	if (err)
 		goto err_register;
+
 	pci_set_drvdata(pdev, gdev);
 
 	return 0;
@@ -182,12 +453,14 @@ static int probe(struct pci_dev *pdev,
 err_register:
 	release_iomaps(gdev);
 err_maps:
+	disable_intr(gdev);
+err_verify:
 	pci_release_regions(pdev);
 err_request_regions:
 	kfree(gdev);
 err_alloc:
-err_verify:
 	pci_disable_device(pdev);
+
 	return err;
 }
 
@@ -196,10 +469,11 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	disable_intr(gdev);
 	release_iomaps(gdev);
 	pci_release_regions(pdev);
-	pci_disable_device(pdev);
 	kfree(gdev);
+	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 }
 
diff --git a/include/uapi/linux/uio_pci_generic.h b/include/uapi/linux/uio_pci_generic.h
new file mode 100644
index 0000000..7c13f5c
--- /dev/null
+++ b/include/uapi/linux/uio_pci_generic.h
@@ -0,0 +1,51 @@
+/*
+ * include/uapi/linux/uio_pci_generic.h
+ *
+ * Header file for userspace generic PCI IO driver and applications with public
+ * API.
+ *
+ * Author: Vlad Zolotarov <vladz@cloudius-systems.com>
+ *
+ * Licensed under the GPLv2 only.
+ */
+
+#ifndef _UIO_PCI_GENERIC_H_
+#define _UIO_PCI_GENERIC_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+enum uio_int_mode {
+	UIO_INT_MODE_NONE = 0,
+	UIO_INT_MODE_INTX = 1,
+	UIO_INT_MODE_MSI  = 2,
+	UIO_INT_MODE_MSIX = 3
+};
+
+/* bind/unbind the requested IRQ to the given eventfd */
+struct uio_pci_generic_irq_set {
+	/* index of the IRQ to connect to starting from 0 */
+	__u32 vec;
+	/* eventfd file descriptor for binding or a negative value for
+	 * unbinding.
+	 */
+	__s32 fd;
+};
+
+#define UIO_PCI_GENERIC_BASE		0x86
+
+/* Bind/unbind the eventfd file descriptor to/from the specific IRQ vector.
+ * Vector is defined by its index starting from 0.
+ */
+#define UIO_PCI_GENERIC_IRQ_SET		_IOW('I', UIO_PCI_GENERIC_BASE + 1, \
+		struct uio_pci_generic_irq_set)
+
+/* Return the number of available IRQs */
+#define UIO_PCI_GENERIC_IRQ_NUM_GET	_IOR('I', UIO_PCI_GENERIC_BASE + 2, \
+		uint32_t)
+
+/* Return the device interrupt mode (uio_int_mode values) */
+#define UIO_PCI_GENERIC_INT_MODE_GET	_IOR('I', UIO_PCI_GENERIC_BASE + 3, \
+		uint32_t)
+
+#endif /* _UIO_PCI_GENERIC_H_ */
-- 
2.1.0

[PATCH v5 4/4] Documentation: update uio-howto

[Vlad Zolotarov]

Change the chapters related to uio_pci_generic that refer interrupt mode.
Add the relevant explanation regarding MSI and MSI-X interrupt modes
support.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v4:
   - Update uio_pci_generic example in uio-howto.tmpl
---
 Documentation/DocBook/uio-howto.tmpl | 133 ++++++++++++++++++++++++++++++-----
 1 file changed, 116 insertions(+), 17 deletions(-)

diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
index cd0e452..507b2ca 100644
--- a/Documentation/DocBook/uio-howto.tmpl
+++ b/Documentation/DocBook/uio-howto.tmpl
@@ -46,6 +46,12 @@ GPL version 2.
 
 <revhistory>
 	<revision>
+	<revnumber>0.10</revnumber>
+	<date>2015-10-04</date>
+	<authorinitials>vz</authorinitials>
+	<revremark>Added MSI and MSI-X support to uio_pci_generic.</revremark>
+	</revision>
+	<revision>
 	<revnumber>0.9</revnumber>
 	<date>2009-07-16</date>
 	<authorinitials>mst</authorinitials>
@@ -935,15 +941,32 @@ and look in the output for failure reasons
 <sect1 id="uio_pci_generic_internals">
 <title>Things to know about uio_pci_generic</title>
 	<para>
-Interrupts are handled using the Interrupt Disable bit in the PCI command
+Interrupts are handled either as MSI-X or MSI interrupts (if the device supports it) or
+as legacy INTx interrupts. By default INTx interrupts are used.
+	</para>
+	<para>
+uio_pci_generic automatically configures a device to use INTx interrupt for backward
+compatibility. If INTx are not available MSI-X interrupts will be used if the device
+supports it and if not MSI interrupts are going to be used. If none of the interrupts
+modes is supported probe() will fail.
+	</para>
+	<para>
+To get the used interrupt mode application has to use UIO_PCI_GENERIC_INT_MODE_GET ioctl
+command.
+UIO_PCI_GENERIC_IRQ_NUM_GET ioctl command may be used to get the total number of IRQs.
+Then UIO_PCI_GENERIC_IRQ_SET ioctl command may be used to bind a specific eventfd to a specific
+IRQ vector.
+	</para>
+	<para>
+Legacy interrupts are handled using the Interrupt Disable bit in the PCI command
 register and Interrupt Status bit in the PCI status register.  All devices
 compliant to PCI 2.3 (circa 2002) and all compliant PCI Express devices should
 support these bits.  uio_pci_generic detects this support, and won't bind to
 devices which do not support the Interrupt Disable Bit in the command register.
 	</para>
 	<para>
-On each interrupt, uio_pci_generic sets the Interrupt Disable bit.
-This prevents the device from generating further interrupts
+If legacy interrupts are used, uio_pci_generic sets the Interrupt Disable bit on
+each interrupt. This prevents the device from generating further interrupts
 until the bit is cleared. The userspace driver should clear this
 bit before blocking and waiting for more interrupts.
 	</para>
@@ -966,17 +989,23 @@ Here is some sample userspace driver code using uio_pci_generic:
 #include &lt;unistd.h&gt;
 #include &lt;sys/types.h&gt;
 #include &lt;sys/stat.h&gt;
+#include &lt;linux/uio_pci_generic.h&gt;
 #include &lt;fcntl.h&gt;
 #include &lt;errno.h&gt;
+#include &lt;sys/eventfd.h&gt;
+#include &lt;sys/ioctl.h&gt;
 
 int main()
 {
-	int uiofd;
+	int uiofd, event_fd;
 	int configfd;
 	int err;
 	int i;
 	unsigned icount;
+	__u64 read_buf;
 	unsigned char command_high;
+	__u32 int_mode, num_irqs;
+	int bytes_to_read;
 
 	uiofd = open(&quot;/dev/uio0&quot;, O_RDONLY);
 	if (uiofd &lt; 0) {
@@ -989,13 +1018,65 @@ int main()
 		return errno;
 	}
 
-	/* Read and cache command value */
-	err = pread(configfd, &amp;command_high, 1, 5);
-	if (err != 1) {
-		perror(&quot;command config read:&quot;);
+	/* Get the interrupt mode */
+	if (ioctl(uiofd, UIO_PCI_GENERIC_INT_MODE_GET, &amp;int_mode) < 0)
+		{ perror(&quot;getting interrupt mode&quot;); return errno;
+	}
+
+	/* Read the number of available IRQs - for INT#x and MSI 1 (one) will be returned */
+	if (ioctl(uiofd, UIO_PCI_GENERIC_IRQ_NUM_GET, &amp;num_irqs) < 0) {
+		perror(&quot;getting IRQs number&quot;);
 		return errno;
 	}
-	command_high &amp;= ~0x4;
+
+	/* if interrupt mode is MSI-X - allocate eventfd file descriptor and bind it to
+	 * the IRQ you need. We'll bind it to the first available IRQ in this example.
+	 */
+	if (int_mode == UIO_INT_MODE_MSIX) {
+		struct uio_pci_generic_irq_set irq_set;
+
+		if (num_irqs < 1) {
+			printf(&quot;Hmmm... Zero IRQ numbers. Something wrong with MSI-X configuration\n&quot;);
+			return -1;
+		}
+
+		printf(&quot;Interrupt mode is MSI-X, we are going to use IRQ[0]\n&quot;);
+
+		/* set up an eventfd for an interrupt */
+		event_fd = eventfd(0, EFD_CLOEXEC);
+		if (event_fd < 0) {
+			perror(&quot;cannot create irq eventfd&quot;);
+			return errno;
+		}
+
+		/* connect to the first IRQ */
+		irq_set.vec = 0;
+		irq_set.fd = event_fd;
+
+		if (ioctl(uiofd, UIO_PCI_GENERIC_IRQ_SET, &amp;irq_set) < 0) {
+			perror(&quot;binding the eventfd descriptor to IRQ[0]&quot;);
+			return errno;
+		}
+
+		/* eventfd read() requires to read 8 bytes, while UIO - requires 4 bytes */
+		bytes_to_read = 8;
+	} else if (int_mode == UIO_INT_MODE_MSI || int_mode == UIO_INT_MODE_INTX) {
+		event_fd = uiofd;
+		bytes_to_read = 4;
+
+		if (int_mode == UIO_INT_MODE_INTX) {
+			/* Read and cache command value */
+			err = pread(configfd, &amp;command_high, 1, 5);
+			if (err != 1) {
+				perror(&quot;command config read:&quot;);
+				return errno;
+			}
+			command_high &amp;= ~0x4;
+		}
+	} else {
+		printf(&quot;Interrupts are not supported\n&quot;);
+		return -1;
+	}
 
 	for(i = 0;; ++i) {
 		/* Print out a message, for debugging. */
@@ -1006,24 +1087,42 @@ int main()
 
 		/****************************************/
 		/* Here we got an interrupt from the
-		   device. Do something to it. */
+		   device. Do something to it and handle the HW
+		   interrupt state machine if needed. */
 		/****************************************/
 
-		/* Re-enable interrupts. */
-		err = pwrite(configfd, &amp;command_high, 1, 5);
-		if (err != 1) {
-			perror(&quot;config write:&quot;);
-			break;
+		if (int_mode == UIO_INT_MODE_INTX) {
+			/* Re-enable interrupts. */
+			err = pwrite(configfd, &amp;command_high, 1, 5);
+			if (err != 1) {
+				perror(&quot;config write:&quot;);
+				break;
+			}
 		}
 
 		/* Wait for next interrupt. */
-		err = read(uiofd, &amp;icount, 4);
-		if (err != 4) {
+		err = read(event_fd, &amp;read_buf, bytes_to_read);
+		if (err != bytes_to_read) {
 			perror(&quot;uio read:&quot;);
 			break;
 		}
 
+		icount++;
 	}
+
+	/* optional: unbind the eventfd from the IRQ */
+	if (int_mode == UIO_INT_MODE_MSIX) {
+		struct uio_pci_generic_irq_set irq_set = {
+			.vec = 0,
+			.fd = -1
+		};
+
+		if (ioctl(uiofd, UIO_PCI_GENERIC_IRQ_SET, &amp;irq_set) < 0) {
+			perror(&quot;unbinding the eventfd descriptor from IRQ[0]&quot;);
+			return errno;
+		}
+	}
+
 	return errno;
 }
 
-- 
2.1.0

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Michael S. Tsirkin]

On Tue, Oct 06, 2015 at 08:17:35PM +0300, Vlad Zolotarov wrote:
> This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
> 
> Currently uio_pci_generic demands INT#x interrupts source be available. However
> there are devices that simply don't have INT#x capability, for instance SR-IOV
> VF devices that simply don't have INT#x capability. For such devices
> uio_pci_generic will simply fail (more specifically its probe() will fail).
> 
> When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to
> performance overhead and thus VFIO is not an option users that develop
> user-space drivers are left without any option but to develop some proprietary
> UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to use UIO
> infrastructure.
> 
> This series provides a generic solution for this problem while preserving the
> original behaviour for devices for which the original uio_pci_generic had worked
> before (i.e. INT#x will be used by default).
> 
> New in v5:
>    - Expanded the commitlog on PATCH1.

Looks like you didn't attempt to address any of my review comments.
I don't intend to review this until you do.

> New in v4:
>    - Use portable __u32 and __s32 types from asm/types.h for
>      defining uio_pci_generic_irq_set fields.
>    - Use proper _IO macros for defining read and write ioctl()
>      commands.
>    - Moved bars mapping setting into a separate patch.
>    - Update uio_pci_generic example in uio-howto.tmpl.
> 
> New in v3:
>    - Add __iomem qualifier to temp buffer receiving ioremap value.  
>  
> New in v2:
>    - Added #include <linux/uaccess.h> to uio_pci_generic.c
> 
> 
> Vlad Zolotarov (4):
>   uio: add ioctl support
>   uio_pci_generic: properly initialize PCI bars mappings towards UIO
>   uio_pci_generic: add MSI/MSI-X support
>   Documentation: update uio-howto
> 
>  Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
>  drivers/uio/uio.c                    |  15 ++
>  drivers/uio/uio_pci_generic.c        | 409 +++++++++++++++++++++++++++++++++--
>  include/linux/uio_driver.h           |   3 +
>  include/uapi/linux/uio_pci_generic.h |  51 +++++
>  5 files changed, 574 insertions(+), 43 deletions(-)
>  create mode 100644 include/uapi/linux/uio_pci_generic.h
> 
> -- 
> 2.1.0

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov]

On 10/06/15 21:27, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 08:17:35PM +0300, Vlad Zolotarov wrote:
>> This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
>>
>> Currently uio_pci_generic demands INT#x interrupts source be available. However
>> there are devices that simply don't have INT#x capability, for instance SR-IOV
>> VF devices that simply don't have INT#x capability. For such devices
>> uio_pci_generic will simply fail (more specifically its probe() will fail).
>>
>> When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to
>> performance overhead and thus VFIO is not an option users that develop
>> user-space drivers are left without any option but to develop some proprietary
>> UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to use UIO
>> infrastructure.
>>
>> This series provides a generic solution for this problem while preserving the
>> original behaviour for devices for which the original uio_pci_generic had worked
>> before (i.e. INT#x will be used by default).
>>
>> New in v5:
>>     - Expanded the commitlog on PATCH1.
> Looks like you didn't attempt to address any of my review comments.
> I don't intend to review this until you do.

So far there hasn't been any comments related to the code in these 
patches  from your side but rather comments about the general flaws of 
the current uio_pci_generic in particular and UIO in general that have 
nothing to do with this series. Therefore obviously there was nothing to 
address.
If u have any comments related to _THIS_ series I'd be glad to address. 
So far I was under the strong impression that u develop an obviously 
theoretical discussion about "nice to have fixed" stuff in UIO, which 
was obvious to everybody on this thread had nothing to do with this 
patch series.

Could it be that I've got u wrong? If so, could u, pls., clarify what 
u'd like me to fix in these patches exactly and why?

thanks,
vlad

>
>> New in v4:
>>     - Use portable __u32 and __s32 types from asm/types.h for
>>       defining uio_pci_generic_irq_set fields.
>>     - Use proper _IO macros for defining read and write ioctl()
>>       commands.
>>     - Moved bars mapping setting into a separate patch.
>>     - Update uio_pci_generic example in uio-howto.tmpl.
>>
>> New in v3:
>>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>>   
>> New in v2:
>>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>>
>>
>> Vlad Zolotarov (4):
>>    uio: add ioctl support
>>    uio_pci_generic: properly initialize PCI bars mappings towards UIO
>>    uio_pci_generic: add MSI/MSI-X support
>>    Documentation: update uio-howto
>>
>>   Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
>>   drivers/uio/uio.c                    |  15 ++
>>   drivers/uio/uio_pci_generic.c        | 409 +++++++++++++++++++++++++++++++++--
>>   include/linux/uio_driver.h           |   3 +
>>   include/uapi/linux/uio_pci_generic.h |  51 +++++
>>   5 files changed, 574 insertions(+), 43 deletions(-)
>>   create mode 100644 include/uapi/linux/uio_pci_generic.h
>>
>> -- 
>> 2.1.0

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov]

On 10/06/15 23:35, Vlad Zolotarov wrote:
>
>
> On 10/06/15 21:27, Michael S. Tsirkin wrote:
>> On Tue, Oct 06, 2015 at 08:17:35PM +0300, Vlad Zolotarov wrote:
>>> This series add support for MSI and MSI-X interrupts to 
>>> uio_pci_generic driver.
>>>
>>> Currently uio_pci_generic demands INT#x interrupts source be 
>>> available. However
>>> there are devices that simply don't have INT#x capability, for 
>>> instance SR-IOV
>>> VF devices that simply don't have INT#x capability. For such devices
>>> uio_pci_generic will simply fail (more specifically its probe() will 
>>> fail).
>>>
>>> When IOMMU is either not available (e.g. Amazon EC2) or not 
>>> acceptable due to
>>> performance overhead and thus VFIO is not an option users that develop
>>> user-space drivers are left without any option but to develop some 
>>> proprietary
>>> UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to 
>>> use UIO
>>> infrastructure.
>>>
>>> This series provides a generic solution for this problem while 
>>> preserving the
>>> original behaviour for devices for which the original 
>>> uio_pci_generic had worked
>>> before (i.e. INT#x will be used by default).
>>>
>>> New in v5:
>>>     - Expanded the commitlog on PATCH1.
>> Looks like you didn't attempt to address any of my review comments.
>> I don't intend to review this until you do.
>
> So far there hasn't been any comments related to the code in these 
> patches  from your side but rather comments about the general flaws of 
> the current uio_pci_generic in particular and UIO in general that have 
> nothing to do with this series. Therefore obviously there was nothing 
> to address.
> If u have any comments related to _THIS_ series I'd be glad to 
> address. So far I was under the strong impression that u develop an 
> obviously theoretical discussion about "nice to have fixed" stuff in 
> UIO, which was obvious to everybody on this thread had nothing to do 
> with this patch series.
>
> Could it be that I've got u wrong? If so, could u, pls., clarify what 
> u'd like me to fix in these patches exactly and why?

I beg my pardon, there was one relevant comment of yours - to split the 
bars mapping into a separate patch. And this has been addressed. ;)

>
> thanks,
> vlad
>
>>
>>> New in v4:
>>>     - Use portable __u32 and __s32 types from asm/types.h for
>>>       defining uio_pci_generic_irq_set fields.
>>>     - Use proper _IO macros for defining read and write ioctl()
>>>       commands.
>>>     - Moved bars mapping setting into a separate patch.
>>>     - Update uio_pci_generic example in uio-howto.tmpl.
>>>
>>> New in v3:
>>>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>>>   New in v2:
>>>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>>>
>>>
>>> Vlad Zolotarov (4):
>>>    uio: add ioctl support
>>>    uio_pci_generic: properly initialize PCI bars mappings towards UIO
>>>    uio_pci_generic: add MSI/MSI-X support
>>>    Documentation: update uio-howto
>>>
>>>   Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
>>>   drivers/uio/uio.c                    |  15 ++
>>>   drivers/uio/uio_pci_generic.c        | 409 
>>> +++++++++++++++++++++++++++++++++--
>>>   include/linux/uio_driver.h           |   3 +
>>>   include/uapi/linux/uio_pci_generic.h |  51 +++++
>>>   5 files changed, 574 insertions(+), 43 deletions(-)
>>>   create mode 100644 include/uapi/linux/uio_pci_generic.h
>>>
>>> -- 
>>> 2.1.0
>

Re: [PATCH v5 1/4] uio: add ioctl support

[Stephen Hemminger]

On Tue,  6 Oct 2015 20:17:36 +0300
Vlad Zolotarov <vladz@cloudius-systems.com> wrote:

> Add the ability for underlying device drivers to register the ioctl
> commands. This is useful when some interaction with the user space
> beyond sysfs capabilities is required, e.g. query the interrupt mode
> or bind eventfd to interrupt notifications (similarly to vfio ioctl
> VFIO_DEVICE_SET_IRQS).
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>

After discussions on and off list, the idea of an ioctl interface
is just not going to be accepted upstream because it can be abused.
Therefore another API will be necessary.

Ps: since I did most of this first, I am surprised you never gave
any attribution for the earlier work.

Re: [PATCH v5 3/4] uio_pci_generic: add MSI/MSI-X support

[Stephen Hemminger]

On Tue,  6 Oct 2015 20:17:38 +0300
Vlad Zolotarov <vladz@cloudius-systems.com> wrote:

> +	int i, vectors = pci_msix_vec_count(pdev);
> +
> +	if (vectors <= 0)
> +		return false;
> +

Since devices like fm10k can thousands of MSI vectors, and often systems
have limited number of slots, some resource control is needed.
I limited the number in my version. That was one of the reasons for the late
binding of MSI vectors. Intended to allow application to ask for how
many it wanted.

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Michael S. Tsirkin]

On Tue, Oct 06, 2015 at 11:35:27PM +0300, Vlad Zolotarov wrote:
> 
> 
> On 10/06/15 21:27, Michael S. Tsirkin wrote:
> >On Tue, Oct 06, 2015 at 08:17:35PM +0300, Vlad Zolotarov wrote:
> >>This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
> >>
> >>Currently uio_pci_generic demands INT#x interrupts source be available. However
> >>there are devices that simply don't have INT#x capability, for instance SR-IOV
> >>VF devices that simply don't have INT#x capability. For such devices
> >>uio_pci_generic will simply fail (more specifically its probe() will fail).
> >>
> >>When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to
> >>performance overhead and thus VFIO is not an option users that develop
> >>user-space drivers are left without any option but to develop some proprietary
> >>UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to use UIO
> >>infrastructure.
> >>
> >>This series provides a generic solution for this problem while preserving the
> >>original behaviour for devices for which the original uio_pci_generic had worked
> >>before (i.e. INT#x will be used by default).
> >>
> >>New in v5:
> >>    - Expanded the commitlog on PATCH1.
> >Looks like you didn't attempt to address any of my review comments.
> >I don't intend to review this until you do.
> 
> So far there hasn't been any comments related to the code in these patches
> from your side but rather comments about the general flaws of the current
> uio_pci_generic in particular and UIO in general that have nothing to do
> with this series. Therefore obviously there was nothing to address.
> If u have any comments related to _THIS_ series I'd be glad to address. So
> far I was under the strong impression that u develop an obviously
> theoretical discussion about "nice to have fixed" stuff in UIO, which was
> obvious to everybody on this thread had nothing to do with this patch
> series.
> 
> Could it be that I've got u wrong? If so, could u, pls., clarify what u'd
> like me to fix in these patches exactly and why?
> 
> thanks,
> vlad

The issues I pointed out are not "nice to have fixed" at all.
The patchset isn't acceptable if you don't address them.
Sorry, I don't have the time to go over them again.
Please just dig them out of the archive.

> >
> >>New in v4:
> >>    - Use portable __u32 and __s32 types from asm/types.h for
> >>      defining uio_pci_generic_irq_set fields.
> >>    - Use proper _IO macros for defining read and write ioctl()
> >>      commands.
> >>    - Moved bars mapping setting into a separate patch.
> >>    - Update uio_pci_generic example in uio-howto.tmpl.
> >>
> >>New in v3:
> >>    - Add __iomem qualifier to temp buffer receiving ioremap value.
> >>New in v2:
> >>    - Added #include <linux/uaccess.h> to uio_pci_generic.c
> >>
> >>
> >>Vlad Zolotarov (4):
> >>   uio: add ioctl support
> >>   uio_pci_generic: properly initialize PCI bars mappings towards UIO
> >>   uio_pci_generic: add MSI/MSI-X support
> >>   Documentation: update uio-howto
> >>
> >>  Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
> >>  drivers/uio/uio.c                    |  15 ++
> >>  drivers/uio/uio_pci_generic.c        | 409 +++++++++++++++++++++++++++++++++--
> >>  include/linux/uio_driver.h           |   3 +
> >>  include/uapi/linux/uio_pci_generic.h |  51 +++++
> >>  5 files changed, 574 insertions(+), 43 deletions(-)
> >>  create mode 100644 include/uapi/linux/uio_pci_generic.h
> >>
> >>-- 
> >>2.1.0

Re: [PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

On 10/07/15 00:33, Stephen Hemminger wrote:
> On Tue,  6 Oct 2015 20:17:36 +0300
> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>
>> Add the ability for underlying device drivers to register the ioctl
>> commands. This is useful when some interaction with the user space
>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>> VFIO_DEVICE_SET_IRQS).
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> After discussions on and off list, the idea of an ioctl interface
> is just not going to be accepted upstream because it can be abused.
> Therefore another API will be necessary.

I'm open for ideas. So far ioctl seems like the most appropriate 
candidate for the job... ;)

>
> Ps: since I did most of this first,

this particular patch and parts of SET_IRQ code - yes. But that would be 
all. Let's just be specific... ;)
This still doesn't mean that u don't deserve a proper credit for that. 
;) But there was a reason why I didn't do that. See below.
> I am surprised you never gave
> any attribution for the earlier work.

If memory serves me well I asked u about the "attribution" a few days 
ago of the dpdk-dev list but got no answer.
Therefore I took no steps in this regard since it left me under the 
impression that u just didn't want it. However it seems now
that that's not the case... ;)

Pls., let me know if u want me to mention that this patch and SET_IRQ 
ioctl code is based on your patches on dpdk-dev list and if yes, in which
form: just mentioning it in the patch description or putting your 
signed-off to the patch(es).

thanks,
vlad

Re: [PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

On 10/07/15 11:17, Vlad Zolotarov wrote:
>
>
> On 10/07/15 00:33, Stephen Hemminger wrote:
>> On Tue,  6 Oct 2015 20:17:36 +0300
>> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>>
>>> Add the ability for underlying device drivers to register the ioctl
>>> commands. This is useful when some interaction with the user space
>>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>>> VFIO_DEVICE_SET_IRQS).
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> After discussions on and off list, the idea of an ioctl interface
>> is just not going to be accepted upstream because it can be abused.
>> Therefore another API will be necessary.
>
> I'm open for ideas. So far ioctl seems like the most appropriate 
> candidate for the job... ;)
>
>>
>> Ps: since I did most of this first,
>
> this particular patch and parts of SET_IRQ code - yes. But that would 
> be all. Let's just be specific... ;)

Pardon - mapping bars was also snitched from your patches... ;)

> This still doesn't mean that u don't deserve a proper credit for that. 
> ;) But there was a reason why I didn't do that. See below.
>> I am surprised you never gave
>> any attribution for the earlier work.
>
> If memory serves me well I asked u about the "attribution" a few days 
> ago of the dpdk-dev list but got no answer.
> Therefore I took no steps in this regard since it left me under the 
> impression that u just didn't want it. However it seems now
> that that's not the case... ;)
>
> Pls., let me know if u want me to mention that this patch and SET_IRQ 
> ioctl code is based on your patches on dpdk-dev list and if yes, in which
> form: just mentioning it in the patch description or putting your 
> signed-off to the patch(es).
>
> thanks,
> vlad
>

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov]

On 10/07/15 01:51, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 11:35:27PM +0300, Vlad Zolotarov wrote:
>>
>> On 10/06/15 21:27, Michael S. Tsirkin wrote:
>>> On Tue, Oct 06, 2015 at 08:17:35PM +0300, Vlad Zolotarov wrote:
>>>> This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
>>>>
>>>> Currently uio_pci_generic demands INT#x interrupts source be available. However
>>>> there are devices that simply don't have INT#x capability, for instance SR-IOV
>>>> VF devices that simply don't have INT#x capability. For such devices
>>>> uio_pci_generic will simply fail (more specifically its probe() will fail).
>>>>
>>>> When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to
>>>> performance overhead and thus VFIO is not an option users that develop
>>>> user-space drivers are left without any option but to develop some proprietary
>>>> UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to use UIO
>>>> infrastructure.
>>>>
>>>> This series provides a generic solution for this problem while preserving the
>>>> original behaviour for devices for which the original uio_pci_generic had worked
>>>> before (i.e. INT#x will be used by default).
>>>>
>>>> New in v5:
>>>>     - Expanded the commitlog on PATCH1.
>>> Looks like you didn't attempt to address any of my review comments.
>>> I don't intend to review this until you do.
>> So far there hasn't been any comments related to the code in these patches
>> from your side but rather comments about the general flaws of the current
>> uio_pci_generic in particular and UIO in general that have nothing to do
>> with this series. Therefore obviously there was nothing to address.
>> If u have any comments related to _THIS_ series I'd be glad to address. So
>> far I was under the strong impression that u develop an obviously
>> theoretical discussion about "nice to have fixed" stuff in UIO, which was
>> obvious to everybody on this thread had nothing to do with this patch
>> series.
>>
>> Could it be that I've got u wrong? If so, could u, pls., clarify what u'd
>> like me to fix in these patches exactly and why?
>>
>> thanks,
>> vlad
> The issues I pointed out are not "nice to have fixed" at all.
> The patchset isn't acceptable if you don't address them.
> Sorry, I don't have the time to go over them again.
> Please just dig them out of the archive.

Let's summarize "the issues u've pointed out" and the explanations why 
they are not relevant to this series for those that haven't been addressed.

 1. "This patch enables bus mastering when MSI or MSI-X interrupts mode
    is used and this would allow bogus user space application trash
    kernel memory".
     1. It's been explained that simple bug in the application that uses
        the newly added interface may not do this. Only specifically
        written user space code that explicitly programs MSI table from
        the user space may cause such harm and this may be done even
        without these patches with the current uio_pci_generic
        implementation.
     2. It's been described how bus mastering and MSI may be enabled
        from the user space right now.
     3. It's been explained how root may trash the kernel memory right
        now even without UIO.
 2. "This patch allows insecure DMA operations since MSI actually
    performs them"
     1. It's been explained that this series adds no additional threat
        to the current driver state since (as described above) this may
        be done without these patches right now.
 3. "Users should not be able to program MSI table from the user space
    therefore it should be zero-mapped".
     1. This may be true but this has nothing to do with this patches
        since they have nothing to do with this ability.
 4. "Why to mimic VFIO SET_IRQ ioctl instead of patching VFIO to be able
    to work in iommu-less mode?"
     1. Because this would break the basic VFIO design assumption and
        would require a lot of non-trivial codding with little-to-none
        added value. See the relevant thread for more detail.
 5. "Separate bars mapping code into a separate patch"
     1. Addressed.

Hope I didn't miss anything.
So, as u may see everything that could be addressed have been addressed 
and the rest are as I've already stated - irrelevant for this patch.

thanks,
vlad

>
>>>> New in v4:
>>>>     - Use portable __u32 and __s32 types from asm/types.h for
>>>>       defining uio_pci_generic_irq_set fields.
>>>>     - Use proper _IO macros for defining read and write ioctl()
>>>>       commands.
>>>>     - Moved bars mapping setting into a separate patch.
>>>>     - Update uio_pci_generic example in uio-howto.tmpl.
>>>>
>>>> New in v3:
>>>>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>>>> New in v2:
>>>>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>>>>
>>>>
>>>> Vlad Zolotarov (4):
>>>>    uio: add ioctl support
>>>>    uio_pci_generic: properly initialize PCI bars mappings towards UIO
>>>>    uio_pci_generic: add MSI/MSI-X support
>>>>    Documentation: update uio-howto
>>>>
>>>>   Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
>>>>   drivers/uio/uio.c                    |  15 ++
>>>>   drivers/uio/uio_pci_generic.c        | 409 +++++++++++++++++++++++++++++++++--
>>>>   include/linux/uio_driver.h           |   3 +
>>>>   include/uapi/linux/uio_pci_generic.h |  51 +++++
>>>>   5 files changed, 574 insertions(+), 43 deletions(-)
>>>>   create mode 100644 include/uapi/linux/uio_pci_generic.h
>>>>
>>>> -- 
>>>> 2.1.0

Re: [PATCH v5 1/4] uio: add ioctl support

[Greg KH]

On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
> Add the ability for underlying device drivers to register the ioctl
> commands. This is useful when some interaction with the user space
> beyond sysfs capabilities is required, e.g. query the interrupt mode
> or bind eventfd to interrupt notifications (similarly to vfio ioctl
> VFIO_DEVICE_SET_IRQS).
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  drivers/uio/uio.c          | 15 +++++++++++++++
>  include/linux/uio_driver.h |  3 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 8196581..714b0e5 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>  	}
>  }
>  
> +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> +{
> +	struct uio_listener *listener = filep->private_data;
> +	struct uio_device *idev = listener->dev;
> +
> +	if (!idev->info)
> +		return -EIO;
> +
> +	if (!idev->info->ioctl)
> +		return -ENOTTY;
> +
> +	return idev->info->ioctl(idev->info, cmd, arg);
> +}

As Stephen said, I will not take this, sorry.  It opens up the ability
to add "new system calls" to a huge range of crappy drivers, it's
something that vendors have been trying to push for years and is
something that I will not allow.

greg k-h

Re: [PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

On 10/07/15 11:46, Greg KH wrote:
> On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
>> Add the ability for underlying device drivers to register the ioctl
>> commands. This is useful when some interaction with the user space
>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>> VFIO_DEVICE_SET_IRQS).
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>   include/linux/uio_driver.h |  3 +++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index 8196581..714b0e5 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>>   	}
>>   }
>>   
>> +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct uio_listener *listener = filep->private_data;
>> +	struct uio_device *idev = listener->dev;
>> +
>> +	if (!idev->info)
>> +		return -EIO;
>> +
>> +	if (!idev->info->ioctl)
>> +		return -ENOTTY;
>> +
>> +	return idev->info->ioctl(idev->info, cmd, arg);
>> +}
> As Stephen said, I will not take this, sorry.  It opens up the ability
> to add "new system calls" to a huge range of crappy drivers, it's
> something that vendors have been trying to push for years and is
> something that I will not allow.

Ok. Another alternative could be to add new sysfs attributes for the 
MSI-X functionality similarly to what is done with "maps".
Would it be acceptable?

>
> greg k-h

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Michael S. Tsirkin]

On Wed, Oct 07, 2015 at 11:38:12AM +0300, Vlad Zolotarov wrote:
> So, as u may see everything that could be addressed have been addressed

Yea. Wrt upstream you've chosen a wrong approach, it creates issues that
can't be addressed.

-- 
MST

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov]

On 10/07/15 13:42, Michael S. Tsirkin wrote:
> On Wed, Oct 07, 2015 at 11:38:12AM +0300, Vlad Zolotarov wrote:
>> So, as u may see everything that could be addressed have been addressed
> Yea. Wrt upstream you've chosen a wrong approach, it creates issues that
> can't be addressed.

It has been explained and proven to u that it creates no issues - see my 
previous email.

>

Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov]

On 10/07/15 16:44, Vlad Zolotarov wrote:
>
>
> On 10/07/15 13:42, Michael S. Tsirkin wrote:
>> On Wed, Oct 07, 2015 at 11:38:12AM +0300, Vlad Zolotarov wrote:
>>> So, as u may see everything that could be addressed have been addressed
>> Yea. Wrt upstream you've chosen a wrong approach, it creates issues that
>> can't be addressed.
>
> It has been explained and proven to u that it creates no issues - see 
> my previous email.

And please be specific. I suggest we define the "issues" this series 
adds in a list so that we can address the issues in this list in order 
not to explain the same thing more than once.

I've started such list two emails ago and have already addressed all the 
issues in it. If there are more "issues" that are not in that list then, 
pls., reply to that email and add them so that I'd be able to address 
them too.

And if there isn't then could u, pls., clarify your point since to me it 
looks like u are blocking this series without any reason at all.

thanks,
vlad

>
>>
>

Re: [PATCH v5 1/4] uio: add ioctl support

[Greg KH]

On Wed, Oct 07, 2015 at 11:55:01AM +0300, Vlad Zolotarov wrote:
> 
> 
> On 10/07/15 11:46, Greg KH wrote:
> >On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
> >>Add the ability for underlying device drivers to register the ioctl
> >>commands. This is useful when some interaction with the user space
> >>beyond sysfs capabilities is required, e.g. query the interrupt mode
> >>or bind eventfd to interrupt notifications (similarly to vfio ioctl
> >>VFIO_DEVICE_SET_IRQS).
> >>
> >>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> >>---
> >>  drivers/uio/uio.c          | 15 +++++++++++++++
> >>  include/linux/uio_driver.h |  3 +++
> >>  2 files changed, 18 insertions(+)
> >>
> >>diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >>index 8196581..714b0e5 100644
> >>--- a/drivers/uio/uio.c
> >>+++ b/drivers/uio/uio.c
> >>@@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
> >>  	}
> >>  }
> >>+static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> >>+{
> >>+	struct uio_listener *listener = filep->private_data;
> >>+	struct uio_device *idev = listener->dev;
> >>+
> >>+	if (!idev->info)
> >>+		return -EIO;
> >>+
> >>+	if (!idev->info->ioctl)
> >>+		return -ENOTTY;
> >>+
> >>+	return idev->info->ioctl(idev->info, cmd, arg);
> >>+}
> >As Stephen said, I will not take this, sorry.  It opens up the ability
> >to add "new system calls" to a huge range of crappy drivers, it's
> >something that vendors have been trying to push for years and is
> >something that I will not allow.
> 
> Ok. Another alternative could be to add new sysfs attributes for the MSI-X
> functionality similarly to what is done with "maps".
> Would it be acceptable?

If you get everyone else here to agree that this is the interface you
all are going to be using, sure.  All I care is that you not add ioctl
to the UIO interface.

greg k-h

Re: [PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

On 10/07/15 20:21, Greg KH wrote:
> On Wed, Oct 07, 2015 at 11:55:01AM +0300, Vlad Zolotarov wrote:
>>
>> On 10/07/15 11:46, Greg KH wrote:
>>> On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
>>>> Add the ability for underlying device drivers to register the ioctl
>>>> commands. This is useful when some interaction with the user space
>>>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>>>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>>>> VFIO_DEVICE_SET_IRQS).
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>>>   include/linux/uio_driver.h |  3 +++
>>>>   2 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>>>> index 8196581..714b0e5 100644
>>>> --- a/drivers/uio/uio.c
>>>> +++ b/drivers/uio/uio.c
>>>> @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>>>>   	}
>>>>   }
>>>> +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>>>> +{
>>>> +	struct uio_listener *listener = filep->private_data;
>>>> +	struct uio_device *idev = listener->dev;
>>>> +
>>>> +	if (!idev->info)
>>>> +		return -EIO;
>>>> +
>>>> +	if (!idev->info->ioctl)
>>>> +		return -ENOTTY;
>>>> +
>>>> +	return idev->info->ioctl(idev->info, cmd, arg);
>>>> +}
>>> As Stephen said, I will not take this, sorry.  It opens up the ability
>>> to add "new system calls" to a huge range of crappy drivers, it's
>>> something that vendors have been trying to push for years and is
>>> something that I will not allow.
>> Ok. Another alternative could be to add new sysfs attributes for the MSI-X
>> functionality similarly to what is done with "maps".
>> Would it be acceptable?
> If you get everyone else here to agree that this is the interface you
> all are going to be using, sure.  All I care is that you not add ioctl
> to the UIO interface.

Ok then. Thanks.

>
> greg k-h

Re: [PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

On 10/07/15 21:05, Stephen Hemminger wrote:
>
> Thinking more of a custom filesystem like configfs
>

Why would configfs be better?
I imagine the final files layout as follows: uioY device that would have 
enabled MSI-X would have a separate uio/uioY/msix/irqZ for each IRQ[Z] 
MSI-X vector.
And this under the same roof with uio/uioY/maps/mapK files.

We then need to implement the write() method for irqZ files so that it 
would accept the eventfd and bind it (or unbind if the value is negative).
We may also make read() return not only the currently bond eventfd 
descriptor but also the corresponding MSI-X IRQ number.

Re: [PATCH v5 1/4] uio: add ioctl support

[Vlad Zolotarov]

On 10/07/15 22:51, Stephen Hemminger wrote:
>
> I was thinking of not naming it uio at all
>

U mean doing something completely orthogonal to UIO? And how did u thing 
to access bars? With UIO or implement this functionality in the new 
configfs too?
If yes - then it will essentially be UIO with MSI-X support, won't it? 
;) It'll also be the third user-space drivers infrastructure and I'm not 
sure there is enough motivation add it.
Note that "some on this thread" are claiming that even two is too many... ;)

Re: [PATCH v5 1/4] uio: add ioctl support

[Greg Kroah-Hartman]

On Thu, Oct 08, 2015 at 03:41:41PM +0300, Vlad Zolotarov wrote:
> 
> 
> On 10/07/15 22:51, Stephen Hemminger wrote:
> >
> >I was thinking of not naming it uio at all
> >
> 
> U mean doing something completely orthogonal to UIO? And how did u thing to
> access bars? With UIO or implement this functionality in the new configfs
> too?
> If yes - then it will essentially be UIO with MSI-X support, won't it? ;)
> It'll also be the third user-space drivers infrastructure and I'm not sure
> there is enough motivation add it.
> Note that "some on this thread" are claiming that even two is too many... ;)

Let's wait to see what happens here, I talked to Stephen and I think
that a non-UIO driver might be the correct solution here.  At the very
least, the proposed patches are not acceptable, so it can't be any worse
:)

greg k-h