Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1236829 > unrolled thread

[PATCH 0/2] uio_msi: device driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] uio_msi: device driver Stephen Hemminger <stephen@networkplumber.org> - 2015-10-01 00:30 +0200
    [PATCH 2/2] uio: new driver to support PCI MSI-X Stephen Hemminger <stephen@networkplumber.org> - 2015-10-01 00:30 +0200
      Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 10:40 +0200
        Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 12:40 +0200
          Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 18:10 +0200
        Re: [PATCH 2/2] uio: new driver to support PCI MSI-X Stephen Hemminger <stephen@networkplumber.org> - 2015-10-01 17:00 +0200
          Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 17:30 +0200
        Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 18:40 +0200
          Re: [PATCH 2/2] uio: new driver to support PCI MSI-X Stephen Hemminger <stephen@networkplumber.org> - 2015-10-01 19:30 +0200
            Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 20:30 +0200
        Re: [PATCH 2/2] uio: new driver to support PCI MSI-X "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 00:00 +0200
      Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-02 01:50 +0200
        Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X Stephen Hemminger <stephen@networkplumber.org> - 2015-10-02 02:10 +0200
          Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-02 04:40 +0200
        Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X Stephen Hemminger <stephen@networkplumber.org> - 2015-10-02 02:10 +0200
          Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-02 03:30 +0200
    [PATCH 1/2] uio: add support for ioctls Stephen Hemminger <stephen@networkplumber.org> - 2015-10-01 00:30 +0200
    Re: [PATCH 0/2] uio_msi: device driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-01 10:40 +0200
    Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Avi Kivity <avi@scylladb.com> - 2015-10-01 13:00 +0200
      Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Stephen Hemminger <stephen@networkplumber.org> - 2015-10-01 17:00 +0200
        Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-01 21:50 +0200
          Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Stephen Hemminger <stephen@networkplumber.org> - 2015-10-02 00:10 +0200
            Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-02 01:10 +0200
              Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Stephen Hemminger <stephen@networkplumber.org> - 2015-10-02 01:40 +0200
                Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-02 01:50 +0200
                  Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Stephen Hemminger <stephen@networkplumber.org> - 2015-10-02 02:10 +0200
                    Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Alexander Duyck <alexander.duyck@gmail.com> - 2015-10-02 03:40 +0200
                      Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 19:00 +0200
                        Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Greg KH <gregkh@linux-foundation.org> - 2015-10-04 21:10 +0200
                          Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200

Page 1 of 2  [1] 2  Next page →

#1236829 — [PATCH 0/2] uio_msi: device driver

This is a new UIO device driver to allow supporting MSI-X and MSI devices
in userspace.  It has been used in environments like VMware and older versions
of QEMU/KVM where no IOMMU support is available.

Stephen Hemminger (2):

*** BLURB HERE ***

Stephen Hemminger (2):
  uio: add support for ioctls
  uio: new driver to support PCI MSI-X

 drivers/uio/Kconfig          |   9 ++
 drivers/uio/Makefile         |   1 +
 drivers/uio/uio.c            |  15 ++
 drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/uio_driver.h   |   3 +
 include/uapi/linux/Kbuild    |   1 +
 include/uapi/linux/uio_msi.h |  22 +++
 7 files changed, 429 insertions(+)
 create mode 100644 drivers/uio/uio_msi.c
 create mode 100644 include/uapi/linux/uio_msi.h

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1236831 — [PATCH 2/2] uio: new driver to support PCI MSI-X

This driver allows using PCI device with Message Signalled Interrupt
from userspace. The API is similar to the igb_uio driver used by the DPDK.
Via ioctl it provides a mechanism to map MSI-X interrupts into event
file descriptors similar to VFIO.

VFIO is a better choice if IOMMU is available, but often userspace drivers
have to work in environments where IOMMU support (real or emulated) is
not available.  All UIO drivers that support DMA are not secure against
rogue userspace applications programming DMA hardware to access
private memory; this driver is no less secure than existing code.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/uio/Kconfig          |   9 ++
 drivers/uio/Makefile         |   1 +
 drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/Kbuild    |   1 +
 include/uapi/linux/uio_msi.h |  22 +++
 5 files changed, 411 insertions(+)
 create mode 100644 drivers/uio/uio_msi.c
 create mode 100644 include/uapi/linux/uio_msi.h

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 52c98ce..04adfa0 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
 	  primarily, for virtualization scenarios.
 	  If you compile this as a module, it will be called uio_pci_generic.
 
+config UIO_PCI_MSI
+       tristate "Generic driver supporting MSI-x on PCI Express cards"
+	depends on PCI
+	help
+	  Generic driver that provides Message Signalled IRQ events
+	  similar to VFIO. If IOMMMU is available please use VFIO
+	  instead since it provides more security.
+	  If you compile this as a module, it will be called uio_msi.
+
 config UIO_NETX
 	tristate "Hilscher NetX Card driver"
 	depends on PCI
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 8560dad..62fc44b 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
 obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
 obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
 obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
+obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
new file mode 100644
index 0000000..802b5c4
--- /dev/null
+++ b/drivers/uio/uio_msi.c
@@ -0,0 +1,378 @@
+/*-
+ *
+ * Copyright (c) 2015 by Brocade Communications Systems, Inc.
+ * Author: Stephen Hemminger <stephen@networkplumber.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 only.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/eventfd.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/uio_driver.h>
+#include <linux/msi.h>
+#include <linux/uio_msi.h>
+
+#define DRIVER_VERSION		"0.1.1"
+#define MAX_MSIX_VECTORS	64
+
+/* MSI-X vector information */
+struct uio_msi_pci_dev {
+	struct uio_info info;		/* UIO driver info */
+	struct pci_dev *pdev;		/* PCI device */
+	struct mutex	mutex;		/* open/release/ioctl mutex */
+	int		ref_cnt;	/* references to device */
+	unsigned int	max_vectors;	/* MSI-X slots available */
+	struct msix_entry *msix;	/* MSI-X vector table */
+	struct uio_msi_irq_ctx {
+		struct eventfd_ctx *trigger; /* vector to eventfd */
+		char *name;		/* name in /proc/interrupts */
+	} *ctx;
+};
+
+static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
+{
+	struct uio_msi_pci_dev *udev = arg;
+
+	if (pci_check_and_mask_intx(udev->pdev)) {
+		eventfd_signal(udev->ctx->trigger, 1);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
+{
+	struct eventfd_ctx *trigger = arg;
+
+	eventfd_signal(trigger, 1);
+	return IRQ_HANDLED;
+}
+
+/* set the mapping between vector # and existing eventfd. */
+static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
+{
+	struct eventfd_ctx *trigger;
+	int irq, err;
+
+	if (vec >= udev->max_vectors) {
+		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
+			   vec, udev->max_vectors);
+		return -ERANGE;
+	}
+
+	irq = udev->msix[vec].vector;
+	trigger = udev->ctx[vec].trigger;
+	if (trigger) {
+		/* Clearup existing irq mapping */
+		free_irq(irq, trigger);
+		eventfd_ctx_put(trigger);
+		udev->ctx[vec].trigger = NULL;
+	}
+
+	/* Passing -1 is used to disable interrupt */
+	if (fd < 0)
+		return 0;
+
+	trigger = eventfd_ctx_fdget(fd);
+	if (IS_ERR(trigger)) {
+		err = PTR_ERR(trigger);
+		dev_notice(&udev->pdev->dev,
+			   "eventfd ctx get failed: %d\n", err);
+		return err;
+	}
+
+	if (udev->msix)
+		err = request_irq(irq, uio_msi_irqhandler, 0,
+				  udev->ctx[vec].name, trigger);
+	else
+		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
+				  udev->ctx[vec].name, udev);
+
+	if (err) {
+		dev_notice(&udev->pdev->dev,
+			   "request irq failed: %d\n", err);
+		eventfd_ctx_put(trigger);
+		return err;
+	}
+
+	udev->ctx[vec].trigger = trigger;
+	return 0;
+}
+
+static int
+uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
+{
+	struct uio_msi_pci_dev *udev
+		= container_of(info, struct uio_msi_pci_dev, info);
+	struct uio_msi_irq_set hdr;
+	int err;
+
+	switch (cmd) {
+	case UIO_MSI_IRQ_SET:
+		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
+			return -EFAULT;
+
+		mutex_lock(&udev->mutex);
+		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
+		mutex_unlock(&udev->mutex);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+	}
+	return err;
+}
+
+/* Opening the UIO device for first time enables MSI-X */
+static int
+uio_msi_open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_msi_pci_dev *udev
+		= container_of(info, struct uio_msi_pci_dev, info);
+	int err = 0;
+
+	mutex_lock(&udev->mutex);
+	if (udev->ref_cnt++ == 0) {
+		if (udev->msix)
+			err = pci_enable_msix(udev->pdev, udev->msix,
+					      udev->max_vectors);
+	}
+	mutex_unlock(&udev->mutex);
+
+	return err;
+}
+
+/* Last close of the UIO device releases/disables all IRQ's */
+static int
+uio_msi_release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_msi_pci_dev *udev
+		= container_of(info, struct uio_msi_pci_dev, info);
+	int i;
+
+	mutex_lock(&udev->mutex);
+	if (--udev->ref_cnt == 0) {
+		for (i = 0; i < udev->max_vectors; i++) {
+			int irq = udev->msix[i].vector;
+			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
+
+			if (!trigger)
+				continue;
+
+			free_irq(irq, trigger);
+			eventfd_ctx_put(trigger);
+			udev->ctx[i].trigger = NULL;
+		}
+
+		if (udev->msix)
+			pci_disable_msix(udev->pdev);
+	}
+	mutex_unlock(&udev->mutex);
+
+	return 0;
+}
+
+/* Unmap previously ioremap'd resources */
+static void
+release_iomaps(struct uio_mem *mem)
+{
+	int i;
+
+	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
+		if (mem->internal_addr)
+			iounmap(mem->internal_addr);
+	}
+}
+
+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);
+	return err;
+}
+
+static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct uio_msi_pci_dev *udev;
+	int i, err, vectors;
+
+	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
+	if (!udev)
+		return -ENOMEM;
+
+	err = pci_enable_device(pdev);
+	if (err != 0) {
+		dev_err(&pdev->dev, "cannot enable PCI device\n");
+		goto fail_free;
+	}
+
+	err = pci_request_regions(pdev, "uio_msi");
+	if (err != 0) {
+		dev_err(&pdev->dev, "Cannot request regions\n");
+		goto fail_disable;
+	}
+
+	pci_set_master(pdev);
+
+	/* remap resources */
+	err = setup_maps(pdev, &udev->info);
+	if (err)
+		goto fail_release_iomem;
+
+	/* fill uio infos */
+	udev->info.name = "uio_msi";
+	udev->info.version = DRIVER_VERSION;
+	udev->info.priv = udev;
+	udev->pdev = pdev;
+	udev->info.ioctl = uio_msi_ioctl;
+	udev->info.open = uio_msi_open;
+	udev->info.release = uio_msi_release;
+	udev->info.irq = UIO_IRQ_CUSTOM;
+	mutex_init(&udev->mutex);
+
+	vectors = pci_msix_vec_count(pdev);
+	if (vectors > 0) {
+		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
+		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
+			udev->max_vectors);
+
+		err = -ENOMEM;
+		udev->msix = kcalloc(udev->max_vectors,
+				     sizeof(struct msix_entry), GFP_KERNEL);
+		if (!udev->msix)
+			goto fail_release_iomem;
+	} else if (!pci_intx_mask_supported(pdev)) {
+		dev_err(&pdev->dev,
+			"device does not support MSI-X or INTX\n");
+		err = -EINVAL;
+		goto fail_release_iomem;
+	} else {
+		dev_notice(&pdev->dev, "using INTX\n");
+		udev->info.irq_flags = IRQF_SHARED;
+		udev->max_vectors = 1;
+	}
+
+	udev->ctx = kcalloc(udev->max_vectors,
+			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
+	if (!udev->ctx)
+		goto fail_free_msix;
+
+	for (i = 0; i < udev->max_vectors; i++) {
+		udev->msix[i].entry = i;
+
+		udev->ctx[i].name = kasprintf(GFP_KERNEL,
+					      KBUILD_MODNAME "[%d](%s)",
+					      i, pci_name(pdev));
+		if (!udev->ctx[i].name)
+			goto fail_free_ctx;
+	}
+
+	/* register uio driver */
+	err = uio_register_device(&pdev->dev, &udev->info);
+	if (err != 0)
+		goto fail_free_ctx;
+
+	pci_set_drvdata(pdev, udev);
+	return 0;
+
+fail_free_ctx:
+	for (i = 0; i < udev->max_vectors; i++)
+		kfree(udev->ctx[i].name);
+	kfree(udev->ctx);
+fail_free_msix:
+	kfree(udev->msix);
+fail_release_iomem:
+	release_iomaps(udev->info.mem);
+	pci_release_regions(pdev);
+fail_disable:
+	pci_disable_device(pdev);
+fail_free:
+	kfree(udev);
+
+	pr_notice("%s ret %d\n", __func__, err);
+	return err;
+}
+
+static void uio_msi_remove(struct pci_dev *pdev)
+{
+	struct uio_info *info = pci_get_drvdata(pdev);
+	struct uio_msi_pci_dev *udev
+		= container_of(info, struct uio_msi_pci_dev, info);
+	int i;
+
+	uio_unregister_device(info);
+	release_iomaps(info->mem);
+
+	pci_release_regions(pdev);
+	for (i = 0; i < udev->max_vectors; i++)
+		kfree(udev->ctx[i].name);
+	kfree(udev->ctx);
+	kfree(udev->msix);
+	pci_disable_device(pdev);
+
+	pci_set_drvdata(pdev, NULL);
+	kfree(udev);
+}
+
+static struct pci_driver uio_msi_pci_driver = {
+	.name = "uio_msi",
+	.probe = uio_msi_probe,
+	.remove = uio_msi_remove,
+};
+
+module_pci_driver(uio_msi_pci_driver);
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Stephen Hemminger <stephen@networkplumber.org>");
+MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index f7b2db4..d9497691 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -411,6 +411,7 @@ header-y += udp.h
 header-y += uhid.h
 header-y += uinput.h
 header-y += uio.h
+header-y += uio_msi.h
 header-y += ultrasound.h
 header-y += un.h
 header-y += unistd.h
diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
new file mode 100644
index 0000000..297de00
--- /dev/null
+++ b/include/uapi/linux/uio_msi.h
@@ -0,0 +1,22 @@
+/*
+ * UIO_MSI API definition
+ *
+ * Copyright (c) 2015 by Brocade Communications Systems, Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _UIO_PCI_MSI_H
+#define _UIO_PCI_MSI_H
+
+struct uio_msi_irq_set {
+	u32 vec;
+	int fd;
+};
+
+#define UIO_MSI_BASE	0x86
+#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
+
+#endif
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237143 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> This driver allows using PCI device with Message Signalled Interrupt
> from userspace. The API is similar to the igb_uio driver used by the DPDK.
> Via ioctl it provides a mechanism to map MSI-X interrupts into event
> file descriptors similar to VFIO.
>
> VFIO is a better choice if IOMMU is available, but often userspace drivers
> have to work in environments where IOMMU support (real or emulated) is
> not available.  All UIO drivers that support DMA are not secure against
> rogue userspace applications programming DMA hardware to access
> private memory; this driver is no less secure than existing code.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

I don't think copying the igb_uio interface is a good idea.
What DPDK is doing with igb_uio (and indeed uio_pci_generic)
is abusing the sysfs BAR access to provide unlimited
access to hardware.

MSI messages are memory writes so any generic device capable
of MSI is capable of corrupting kernel memory.
This means that a bug in userspace will lead to kernel memory corruption
and crashes.  This is something distributions can't support.

uio_pci_generic is already abused like that, mostly
because when I wrote it, I didn't add enough protections
against using it with DMA capable devices,
and we can't go back and break working userspace.
But at least it does not bind to VFs which all of
them are capable of DMA.

The result of merging this driver will be userspace abusing the
sysfs BAR access with VFs as well, and we do not want that.


Just forwarding events is not enough to make a valid driver.
What is missing is a way to access the device in a safe way.

On a more positive note:

What would be a reasonable interface? One that does the following
in kernel:

1. initializes device rings (can be in pinned userspace memory,
   but can not be writeable by userspace), brings up interface link
2. pins userspace memory (unless using e.g. hugetlbfs)
3. gets request, make sure it's valid and belongs to
   the correct task, put it in the ring
4. in the reverse direction, notify userspace when buffers
   are available in the ring
5. notify userspace about MSI (what this driver does)

What userspace can be allowed to do:

	format requests (e.g. transmit, receive) in userspace
	read ring contents

What userspace can't be allowed to do:

	access BAR
	write rings


This means that the driver can not be a generic one,
and there will be a system call overhead when you
write the ring, but that's the price you have to
pay for ability to run on systems without an IOMMU.




> ---
>  drivers/uio/Kconfig          |   9 ++
>  drivers/uio/Makefile         |   1 +
>  drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/Kbuild    |   1 +
>  include/uapi/linux/uio_msi.h |  22 +++
>  5 files changed, 411 insertions(+)
>  create mode 100644 drivers/uio/uio_msi.c
>  create mode 100644 include/uapi/linux/uio_msi.h
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 52c98ce..04adfa0 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
>  	  primarily, for virtualization scenarios.
>  	  If you compile this as a module, it will be called uio_pci_generic.
>  
> +config UIO_PCI_MSI
> +       tristate "Generic driver supporting MSI-x on PCI Express cards"
> +	depends on PCI
> +	help
> +	  Generic driver that provides Message Signalled IRQ events
> +	  similar to VFIO. If IOMMMU is available please use VFIO
> +	  instead since it provides more security.
> +	  If you compile this as a module, it will be called uio_msi.
> +
>  config UIO_NETX
>  	tristate "Hilscher NetX Card driver"
>  	depends on PCI
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 8560dad..62fc44b 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
>  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
>  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
>  obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
> +obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
> diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
> new file mode 100644
> index 0000000..802b5c4
> --- /dev/null
> +++ b/drivers/uio/uio_msi.c
> @@ -0,0 +1,378 @@
> +/*-
> + *
> + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> + * Author: Stephen Hemminger <stephen@networkplumber.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 only.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/eventfd.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/uio_driver.h>
> +#include <linux/msi.h>
> +#include <linux/uio_msi.h>
> +
> +#define DRIVER_VERSION		"0.1.1"
> +#define MAX_MSIX_VECTORS	64
> +
> +/* MSI-X vector information */
> +struct uio_msi_pci_dev {
> +	struct uio_info info;		/* UIO driver info */
> +	struct pci_dev *pdev;		/* PCI device */
> +	struct mutex	mutex;		/* open/release/ioctl mutex */
> +	int		ref_cnt;	/* references to device */
> +	unsigned int	max_vectors;	/* MSI-X slots available */
> +	struct msix_entry *msix;	/* MSI-X vector table */
> +	struct uio_msi_irq_ctx {
> +		struct eventfd_ctx *trigger; /* vector to eventfd */
> +		char *name;		/* name in /proc/interrupts */
> +	} *ctx;
> +};
> +
> +static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
> +{
> +	struct uio_msi_pci_dev *udev = arg;
> +
> +	if (pci_check_and_mask_intx(udev->pdev)) {
> +		eventfd_signal(udev->ctx->trigger, 1);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
> +{
> +	struct eventfd_ctx *trigger = arg;
> +
> +	eventfd_signal(trigger, 1);
> +	return IRQ_HANDLED;
> +}
> +
> +/* set the mapping between vector # and existing eventfd. */
> +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> +{
> +	struct eventfd_ctx *trigger;
> +	int irq, err;
> +
> +	if (vec >= udev->max_vectors) {
> +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> +			   vec, udev->max_vectors);
> +		return -ERANGE;
> +	}
> +
> +	irq = udev->msix[vec].vector;
> +	trigger = udev->ctx[vec].trigger;
> +	if (trigger) {
> +		/* Clearup existing irq mapping */
> +		free_irq(irq, trigger);
> +		eventfd_ctx_put(trigger);
> +		udev->ctx[vec].trigger = NULL;
> +	}
> +
> +	/* Passing -1 is used to disable interrupt */
> +	if (fd < 0)
> +		return 0;
> +
> +	trigger = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(trigger)) {
> +		err = PTR_ERR(trigger);
> +		dev_notice(&udev->pdev->dev,
> +			   "eventfd ctx get failed: %d\n", err);
> +		return err;
> +	}
> +
> +	if (udev->msix)
> +		err = request_irq(irq, uio_msi_irqhandler, 0,
> +				  udev->ctx[vec].name, trigger);
> +	else
> +		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
> +				  udev->ctx[vec].name, udev);
> +
> +	if (err) {
> +		dev_notice(&udev->pdev->dev,
> +			   "request irq failed: %d\n", err);
> +		eventfd_ctx_put(trigger);
> +		return err;
> +	}
> +
> +	udev->ctx[vec].trigger = trigger;
> +	return 0;
> +}
> +
> +static int
> +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	struct uio_msi_irq_set hdr;
> +	int err;
> +
> +	switch (cmd) {
> +	case UIO_MSI_IRQ_SET:
> +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> +			return -EFAULT;
> +
> +		mutex_lock(&udev->mutex);
> +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> +		mutex_unlock(&udev->mutex);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +	return err;
> +}
> +
> +/* Opening the UIO device for first time enables MSI-X */
> +static int
> +uio_msi_open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	int err = 0;
> +
> +	mutex_lock(&udev->mutex);
> +	if (udev->ref_cnt++ == 0) {
> +		if (udev->msix)
> +			err = pci_enable_msix(udev->pdev, udev->msix,
> +					      udev->max_vectors);
> +	}
> +	mutex_unlock(&udev->mutex);
> +
> +	return err;
> +}
> +
> +/* Last close of the UIO device releases/disables all IRQ's */
> +static int
> +uio_msi_release(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	int i;
> +
> +	mutex_lock(&udev->mutex);
> +	if (--udev->ref_cnt == 0) {
> +		for (i = 0; i < udev->max_vectors; i++) {
> +			int irq = udev->msix[i].vector;
> +			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
> +
> +			if (!trigger)
> +				continue;
> +
> +			free_irq(irq, trigger);
> +			eventfd_ctx_put(trigger);
> +			udev->ctx[i].trigger = NULL;
> +		}
> +
> +		if (udev->msix)
> +			pci_disable_msix(udev->pdev);
> +	}
> +	mutex_unlock(&udev->mutex);
> +
> +	return 0;
> +}
> +
> +/* Unmap previously ioremap'd resources */
> +static void
> +release_iomaps(struct uio_mem *mem)
> +{
> +	int i;
> +
> +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> +		if (mem->internal_addr)
> +			iounmap(mem->internal_addr);
> +	}
> +}
> +
> +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);
> +	return err;
> +}
> +
> +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct uio_msi_pci_dev *udev;
> +	int i, err, vectors;
> +
> +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> +	if (!udev)
> +		return -ENOMEM;
> +
> +	err = pci_enable_device(pdev);
> +	if (err != 0) {
> +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> +		goto fail_free;
> +	}
> +
> +	err = pci_request_regions(pdev, "uio_msi");
> +	if (err != 0) {
> +		dev_err(&pdev->dev, "Cannot request regions\n");
> +		goto fail_disable;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	/* remap resources */
> +	err = setup_maps(pdev, &udev->info);
> +	if (err)
> +		goto fail_release_iomem;
> +
> +	/* fill uio infos */
> +	udev->info.name = "uio_msi";
> +	udev->info.version = DRIVER_VERSION;
> +	udev->info.priv = udev;
> +	udev->pdev = pdev;
> +	udev->info.ioctl = uio_msi_ioctl;
> +	udev->info.open = uio_msi_open;
> +	udev->info.release = uio_msi_release;
> +	udev->info.irq = UIO_IRQ_CUSTOM;
> +	mutex_init(&udev->mutex);
> +
> +	vectors = pci_msix_vec_count(pdev);
> +	if (vectors > 0) {
> +		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
> +		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
> +			udev->max_vectors);
> +
> +		err = -ENOMEM;
> +		udev->msix = kcalloc(udev->max_vectors,
> +				     sizeof(struct msix_entry), GFP_KERNEL);
> +		if (!udev->msix)
> +			goto fail_release_iomem;
> +	} else if (!pci_intx_mask_supported(pdev)) {
> +		dev_err(&pdev->dev,
> +			"device does not support MSI-X or INTX\n");
> +		err = -EINVAL;
> +		goto fail_release_iomem;
> +	} else {
> +		dev_notice(&pdev->dev, "using INTX\n");
> +		udev->info.irq_flags = IRQF_SHARED;
> +		udev->max_vectors = 1;
> +	}
> +
> +	udev->ctx = kcalloc(udev->max_vectors,
> +			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
> +	if (!udev->ctx)
> +		goto fail_free_msix;
> +
> +	for (i = 0; i < udev->max_vectors; i++) {
> +		udev->msix[i].entry = i;
> +
> +		udev->ctx[i].name = kasprintf(GFP_KERNEL,
> +					      KBUILD_MODNAME "[%d](%s)",
> +					      i, pci_name(pdev));
> +		if (!udev->ctx[i].name)
> +			goto fail_free_ctx;
> +	}
> +
> +	/* register uio driver */
> +	err = uio_register_device(&pdev->dev, &udev->info);
> +	if (err != 0)
> +		goto fail_free_ctx;
> +
> +	pci_set_drvdata(pdev, udev);
> +	return 0;
> +
> +fail_free_ctx:
> +	for (i = 0; i < udev->max_vectors; i++)
> +		kfree(udev->ctx[i].name);
> +	kfree(udev->ctx);
> +fail_free_msix:
> +	kfree(udev->msix);
> +fail_release_iomem:
> +	release_iomaps(udev->info.mem);
> +	pci_release_regions(pdev);
> +fail_disable:
> +	pci_disable_device(pdev);
> +fail_free:
> +	kfree(udev);
> +
> +	pr_notice("%s ret %d\n", __func__, err);
> +	return err;
> +}
> +
> +static void uio_msi_remove(struct pci_dev *pdev)
> +{
> +	struct uio_info *info = pci_get_drvdata(pdev);
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	int i;
> +
> +	uio_unregister_device(info);
> +	release_iomaps(info->mem);
> +
> +	pci_release_regions(pdev);
> +	for (i = 0; i < udev->max_vectors; i++)
> +		kfree(udev->ctx[i].name);
> +	kfree(udev->ctx);
> +	kfree(udev->msix);
> +	pci_disable_device(pdev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +	kfree(udev);
> +}
> +
> +static struct pci_driver uio_msi_pci_driver = {
> +	.name = "uio_msi",
> +	.probe = uio_msi_probe,
> +	.remove = uio_msi_remove,
> +};
> +
> +module_pci_driver(uio_msi_pci_driver);
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Stephen Hemminger <stephen@networkplumber.org>");
> +MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index f7b2db4..d9497691 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -411,6 +411,7 @@ header-y += udp.h
>  header-y += uhid.h
>  header-y += uinput.h
>  header-y += uio.h
> +header-y += uio_msi.h
>  header-y += ultrasound.h
>  header-y += un.h
>  header-y += unistd.h
> diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
> new file mode 100644
> index 0000000..297de00
> --- /dev/null
> +++ b/include/uapi/linux/uio_msi.h
> @@ -0,0 +1,22 @@
> +/*
> + * UIO_MSI API definition
> + *
> + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _UIO_PCI_MSI_H
> +#define _UIO_PCI_MSI_H
> +
> +struct uio_msi_irq_set {
> +	u32 vec;
> +	int fd;
> +};
> +
> +#define UIO_MSI_BASE	0x86
> +#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
> +
> +#endif
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237257 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > This driver allows using PCI device with Message Signalled Interrupt
> > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > file descriptors similar to VFIO.
> >
> > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > have to work in environments where IOMMU support (real or emulated) is
> > not available.  All UIO drivers that support DMA are not secure against
> > rogue userspace applications programming DMA hardware to access
> > private memory; this driver is no less secure than existing code.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> I don't think copying the igb_uio interface is a good idea.
> What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> is abusing the sysfs BAR access to provide unlimited
> access to hardware.
> 
> MSI messages are memory writes so any generic device capable
> of MSI is capable of corrupting kernel memory.
> This means that a bug in userspace will lead to kernel memory corruption
> and crashes.  This is something distributions can't support.
> 
> uio_pci_generic is already abused like that, mostly
> because when I wrote it, I didn't add enough protections
> against using it with DMA capable devices,
> and we can't go back and break working userspace.
> But at least it does not bind to VFs which all of
> them are capable of DMA.
> 
> The result of merging this driver will be userspace abusing the
> sysfs BAR access with VFs as well, and we do not want that.
> 
> 
> Just forwarding events is not enough to make a valid driver.
> What is missing is a way to access the device in a safe way.
> 
> On a more positive note:
> 
> What would be a reasonable interface? One that does the following
> in kernel:
> 
> 1. initializes device rings (can be in pinned userspace memory,
>    but can not be writeable by userspace), brings up interface link
> 2. pins userspace memory (unless using e.g. hugetlbfs)
> 3. gets request, make sure it's valid and belongs to
>    the correct task, put it in the ring
> 4. in the reverse direction, notify userspace when buffers
>    are available in the ring
> 5. notify userspace about MSI (what this driver does)
> 
> What userspace can be allowed to do:
> 
> 	format requests (e.g. transmit, receive) in userspace
> 	read ring contents
> 
> What userspace can't be allowed to do:
> 
> 	access BAR
> 	write rings

Thinking about it some more, many devices


Have separate rings for DMA: TX (device reads memory)
and RX (device writes memory).
With such devices, a mode where userspace can write TX ring
but not RX ring might make sense.

This will mean userspace might read kernel memory
through the device, but can not corrupt it.

That's already a big win!

And RX buffers do not have to be added one at a time.
If we assume 0.2usec per system call, batching some 100 buffers per
system call gives you 2 nano seconds overhead.  That seems quite
reasonable.





> 
> This means that the driver can not be a generic one,
> and there will be a system call overhead when you
> write the ring, but that's the price you have to
> pay for ability to run on systems without an IOMMU.
> 
> 
> 
> 
> > ---
> >  drivers/uio/Kconfig          |   9 ++
> >  drivers/uio/Makefile         |   1 +
> >  drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/Kbuild    |   1 +
> >  include/uapi/linux/uio_msi.h |  22 +++
> >  5 files changed, 411 insertions(+)
> >  create mode 100644 drivers/uio/uio_msi.c
> >  create mode 100644 include/uapi/linux/uio_msi.h
> > 
> > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > index 52c98ce..04adfa0 100644
> > --- a/drivers/uio/Kconfig
> > +++ b/drivers/uio/Kconfig
> > @@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
> >  	  primarily, for virtualization scenarios.
> >  	  If you compile this as a module, it will be called uio_pci_generic.
> >  
> > +config UIO_PCI_MSI
> > +       tristate "Generic driver supporting MSI-x on PCI Express cards"
> > +	depends on PCI
> > +	help
> > +	  Generic driver that provides Message Signalled IRQ events
> > +	  similar to VFIO. If IOMMMU is available please use VFIO
> > +	  instead since it provides more security.
> > +	  If you compile this as a module, it will be called uio_msi.
> > +
> >  config UIO_NETX
> >  	tristate "Hilscher NetX Card driver"
> >  	depends on PCI
> > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> > index 8560dad..62fc44b 100644
> > --- a/drivers/uio/Makefile
> > +++ b/drivers/uio/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
> >  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
> >  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
> >  obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
> > +obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
> > diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
> > new file mode 100644
> > index 0000000..802b5c4
> > --- /dev/null
> > +++ b/drivers/uio/uio_msi.c
> > @@ -0,0 +1,378 @@
> > +/*-
> > + *
> > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > + * Author: Stephen Hemminger <stephen@networkplumber.org>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 only.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/msi.h>
> > +#include <linux/uio_msi.h>
> > +
> > +#define DRIVER_VERSION		"0.1.1"
> > +#define MAX_MSIX_VECTORS	64
> > +
> > +/* MSI-X vector information */
> > +struct uio_msi_pci_dev {
> > +	struct uio_info info;		/* UIO driver info */
> > +	struct pci_dev *pdev;		/* PCI device */
> > +	struct mutex	mutex;		/* open/release/ioctl mutex */
> > +	int		ref_cnt;	/* references to device */
> > +	unsigned int	max_vectors;	/* MSI-X slots available */
> > +	struct msix_entry *msix;	/* MSI-X vector table */
> > +	struct uio_msi_irq_ctx {
> > +		struct eventfd_ctx *trigger; /* vector to eventfd */
> > +		char *name;		/* name in /proc/interrupts */
> > +	} *ctx;
> > +};
> > +
> > +static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
> > +{
> > +	struct uio_msi_pci_dev *udev = arg;
> > +
> > +	if (pci_check_and_mask_intx(udev->pdev)) {
> > +		eventfd_signal(udev->ctx->trigger, 1);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
> > +{
> > +	struct eventfd_ctx *trigger = arg;
> > +
> > +	eventfd_signal(trigger, 1);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/* set the mapping between vector # and existing eventfd. */
> > +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> > +{
> > +	struct eventfd_ctx *trigger;
> > +	int irq, err;
> > +
> > +	if (vec >= udev->max_vectors) {
> > +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> > +			   vec, udev->max_vectors);
> > +		return -ERANGE;
> > +	}
> > +
> > +	irq = udev->msix[vec].vector;
> > +	trigger = udev->ctx[vec].trigger;
> > +	if (trigger) {
> > +		/* Clearup existing irq mapping */
> > +		free_irq(irq, trigger);
> > +		eventfd_ctx_put(trigger);
> > +		udev->ctx[vec].trigger = NULL;
> > +	}
> > +
> > +	/* Passing -1 is used to disable interrupt */
> > +	if (fd < 0)
> > +		return 0;
> > +
> > +	trigger = eventfd_ctx_fdget(fd);
> > +	if (IS_ERR(trigger)) {
> > +		err = PTR_ERR(trigger);
> > +		dev_notice(&udev->pdev->dev,
> > +			   "eventfd ctx get failed: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	if (udev->msix)
> > +		err = request_irq(irq, uio_msi_irqhandler, 0,
> > +				  udev->ctx[vec].name, trigger);
> > +	else
> > +		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
> > +				  udev->ctx[vec].name, udev);
> > +
> > +	if (err) {
> > +		dev_notice(&udev->pdev->dev,
> > +			   "request irq failed: %d\n", err);
> > +		eventfd_ctx_put(trigger);
> > +		return err;
> > +	}
> > +
> > +	udev->ctx[vec].trigger = trigger;
> > +	return 0;
> > +}
> > +
> > +static int
> > +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	struct uio_msi_irq_set hdr;
> > +	int err;
> > +
> > +	switch (cmd) {
> > +	case UIO_MSI_IRQ_SET:
> > +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> > +			return -EFAULT;
> > +
> > +		mutex_lock(&udev->mutex);
> > +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> > +		mutex_unlock(&udev->mutex);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +	}
> > +	return err;
> > +}
> > +
> > +/* Opening the UIO device for first time enables MSI-X */
> > +static int
> > +uio_msi_open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int err = 0;
> > +
> > +	mutex_lock(&udev->mutex);
> > +	if (udev->ref_cnt++ == 0) {
> > +		if (udev->msix)
> > +			err = pci_enable_msix(udev->pdev, udev->msix,
> > +					      udev->max_vectors);
> > +	}
> > +	mutex_unlock(&udev->mutex);
> > +
> > +	return err;
> > +}
> > +
> > +/* Last close of the UIO device releases/disables all IRQ's */
> > +static int
> > +uio_msi_release(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int i;
> > +
> > +	mutex_lock(&udev->mutex);
> > +	if (--udev->ref_cnt == 0) {
> > +		for (i = 0; i < udev->max_vectors; i++) {
> > +			int irq = udev->msix[i].vector;
> > +			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
> > +
> > +			if (!trigger)
> > +				continue;
> > +
> > +			free_irq(irq, trigger);
> > +			eventfd_ctx_put(trigger);
> > +			udev->ctx[i].trigger = NULL;
> > +		}
> > +
> > +		if (udev->msix)
> > +			pci_disable_msix(udev->pdev);
> > +	}
> > +	mutex_unlock(&udev->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Unmap previously ioremap'd resources */
> > +static void
> > +release_iomaps(struct uio_mem *mem)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> > +		if (mem->internal_addr)
> > +			iounmap(mem->internal_addr);
> > +	}
> > +}
> > +
> > +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);
> > +	return err;
> > +}
> > +
> > +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct uio_msi_pci_dev *udev;
> > +	int i, err, vectors;
> > +
> > +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> > +	if (!udev)
> > +		return -ENOMEM;
> > +
> > +	err = pci_enable_device(pdev);
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> > +		goto fail_free;
> > +	}
> > +
> > +	err = pci_request_regions(pdev, "uio_msi");
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "Cannot request regions\n");
> > +		goto fail_disable;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	/* remap resources */
> > +	err = setup_maps(pdev, &udev->info);
> > +	if (err)
> > +		goto fail_release_iomem;
> > +
> > +	/* fill uio infos */
> > +	udev->info.name = "uio_msi";
> > +	udev->info.version = DRIVER_VERSION;
> > +	udev->info.priv = udev;
> > +	udev->pdev = pdev;
> > +	udev->info.ioctl = uio_msi_ioctl;
> > +	udev->info.open = uio_msi_open;
> > +	udev->info.release = uio_msi_release;
> > +	udev->info.irq = UIO_IRQ_CUSTOM;
> > +	mutex_init(&udev->mutex);
> > +
> > +	vectors = pci_msix_vec_count(pdev);
> > +	if (vectors > 0) {
> > +		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
> > +		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
> > +			udev->max_vectors);
> > +
> > +		err = -ENOMEM;
> > +		udev->msix = kcalloc(udev->max_vectors,
> > +				     sizeof(struct msix_entry), GFP_KERNEL);
> > +		if (!udev->msix)
> > +			goto fail_release_iomem;
> > +	} else if (!pci_intx_mask_supported(pdev)) {
> > +		dev_err(&pdev->dev,
> > +			"device does not support MSI-X or INTX\n");
> > +		err = -EINVAL;
> > +		goto fail_release_iomem;
> > +	} else {
> > +		dev_notice(&pdev->dev, "using INTX\n");
> > +		udev->info.irq_flags = IRQF_SHARED;
> > +		udev->max_vectors = 1;
> > +	}
> > +
> > +	udev->ctx = kcalloc(udev->max_vectors,
> > +			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
> > +	if (!udev->ctx)
> > +		goto fail_free_msix;
> > +
> > +	for (i = 0; i < udev->max_vectors; i++) {
> > +		udev->msix[i].entry = i;
> > +
> > +		udev->ctx[i].name = kasprintf(GFP_KERNEL,
> > +					      KBUILD_MODNAME "[%d](%s)",
> > +					      i, pci_name(pdev));
> > +		if (!udev->ctx[i].name)
> > +			goto fail_free_ctx;
> > +	}
> > +
> > +	/* register uio driver */
> > +	err = uio_register_device(&pdev->dev, &udev->info);
> > +	if (err != 0)
> > +		goto fail_free_ctx;
> > +
> > +	pci_set_drvdata(pdev, udev);
> > +	return 0;
> > +
> > +fail_free_ctx:
> > +	for (i = 0; i < udev->max_vectors; i++)
> > +		kfree(udev->ctx[i].name);
> > +	kfree(udev->ctx);
> > +fail_free_msix:
> > +	kfree(udev->msix);
> > +fail_release_iomem:
> > +	release_iomaps(udev->info.mem);
> > +	pci_release_regions(pdev);
> > +fail_disable:
> > +	pci_disable_device(pdev);
> > +fail_free:
> > +	kfree(udev);
> > +
> > +	pr_notice("%s ret %d\n", __func__, err);
> > +	return err;
> > +}
> > +
> > +static void uio_msi_remove(struct pci_dev *pdev)
> > +{
> > +	struct uio_info *info = pci_get_drvdata(pdev);
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int i;
> > +
> > +	uio_unregister_device(info);
> > +	release_iomaps(info->mem);
> > +
> > +	pci_release_regions(pdev);
> > +	for (i = 0; i < udev->max_vectors; i++)
> > +		kfree(udev->ctx[i].name);
> > +	kfree(udev->ctx);
> > +	kfree(udev->msix);
> > +	pci_disable_device(pdev);
> > +
> > +	pci_set_drvdata(pdev, NULL);
> > +	kfree(udev);
> > +}
> > +
> > +static struct pci_driver uio_msi_pci_driver = {
> > +	.name = "uio_msi",
> > +	.probe = uio_msi_probe,
> > +	.remove = uio_msi_remove,
> > +};
> > +
> > +module_pci_driver(uio_msi_pci_driver);
> > +MODULE_VERSION(DRIVER_VERSION);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Stephen Hemminger <stephen@networkplumber.org>");
> > +MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
> > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > index f7b2db4..d9497691 100644
> > --- a/include/uapi/linux/Kbuild
> > +++ b/include/uapi/linux/Kbuild
> > @@ -411,6 +411,7 @@ header-y += udp.h
> >  header-y += uhid.h
> >  header-y += uinput.h
> >  header-y += uio.h
> > +header-y += uio_msi.h
> >  header-y += ultrasound.h
> >  header-y += un.h
> >  header-y += unistd.h
> > diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
> > new file mode 100644
> > index 0000000..297de00
> > --- /dev/null
> > +++ b/include/uapi/linux/uio_msi.h
> > @@ -0,0 +1,22 @@
> > +/*
> > + * UIO_MSI API definition
> > + *
> > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > + * All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef _UIO_PCI_MSI_H
> > +#define _UIO_PCI_MSI_H
> > +
> > +struct uio_msi_irq_set {
> > +	u32 vec;
> > +	int fd;
> > +};
> > +
> > +#define UIO_MSI_BASE	0x86
> > +#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
> > +
> > +#endif
> > -- 
> > 2.1.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237542 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 01:37:12PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> > On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > > This driver allows using PCI device with Message Signalled Interrupt
> > > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > > file descriptors similar to VFIO.
> > >
> > > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > > have to work in environments where IOMMU support (real or emulated) is
> > > not available.  All UIO drivers that support DMA are not secure against
> > > rogue userspace applications programming DMA hardware to access
> > > private memory; this driver is no less secure than existing code.
> > > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > I don't think copying the igb_uio interface is a good idea.
> > What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> > is abusing the sysfs BAR access to provide unlimited
> > access to hardware.
> > 
> > MSI messages are memory writes so any generic device capable
> > of MSI is capable of corrupting kernel memory.
> > This means that a bug in userspace will lead to kernel memory corruption
> > and crashes.  This is something distributions can't support.
> > 
> > uio_pci_generic is already abused like that, mostly
> > because when I wrote it, I didn't add enough protections
> > against using it with DMA capable devices,
> > and we can't go back and break working userspace.
> > But at least it does not bind to VFs which all of
> > them are capable of DMA.
> > 
> > The result of merging this driver will be userspace abusing the
> > sysfs BAR access with VFs as well, and we do not want that.
> > 
> > 
> > Just forwarding events is not enough to make a valid driver.
> > What is missing is a way to access the device in a safe way.
> > 
> > On a more positive note:
> > 
> > What would be a reasonable interface? One that does the following
> > in kernel:
> > 
> > 1. initializes device rings (can be in pinned userspace memory,
> >    but can not be writeable by userspace), brings up interface link
> > 2. pins userspace memory (unless using e.g. hugetlbfs)
> > 3. gets request, make sure it's valid and belongs to
> >    the correct task, put it in the ring
> > 4. in the reverse direction, notify userspace when buffers
> >    are available in the ring
> > 5. notify userspace about MSI (what this driver does)
> > 
> > What userspace can be allowed to do:
> > 
> > 	format requests (e.g. transmit, receive) in userspace
> > 	read ring contents
> > 
> > What userspace can't be allowed to do:
> > 
> > 	access BAR
> > 	write rings
> 
> Thinking about it some more, many devices
> 
> 
> Have separate rings for DMA: TX (device reads memory)
> and RX (device writes memory).
> With such devices, a mode where userspace can write TX ring
> but not RX ring might make sense.
> 
> This will mean userspace might read kernel memory
> through the device, but can not corrupt it.
> 
> That's already a big win!
> 
> And RX buffers do not have to be added one at a time.
> If we assume 0.2usec per system call, batching some 100 buffers per
> system call gives you 2 nano seconds overhead.  That seems quite
> reasonable.
> 

To add to that, there's no reason to do this on the same core.
Re-arming descriptors can happen in parallel with packet
processing, so this overhead won't affect PPS or latency at all:
only the CPU utilization.


> 
> 
> 
> > 
> > This means that the driver can not be a generic one,
> > and there will be a system call overhead when you
> > write the ring, but that's the price you have to
> > pay for ability to run on systems without an IOMMU.
> > 
> > 
> > 
> > 
> > > ---
> > >  drivers/uio/Kconfig          |   9 ++
> > >  drivers/uio/Makefile         |   1 +
> > >  drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/Kbuild    |   1 +
> > >  include/uapi/linux/uio_msi.h |  22 +++
> > >  5 files changed, 411 insertions(+)
> > >  create mode 100644 drivers/uio/uio_msi.c
> > >  create mode 100644 include/uapi/linux/uio_msi.h
> > > 
> > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > > index 52c98ce..04adfa0 100644
> > > --- a/drivers/uio/Kconfig
> > > +++ b/drivers/uio/Kconfig
> > > @@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
> > >  	  primarily, for virtualization scenarios.
> > >  	  If you compile this as a module, it will be called uio_pci_generic.
> > >  
> > > +config UIO_PCI_MSI
> > > +       tristate "Generic driver supporting MSI-x on PCI Express cards"
> > > +	depends on PCI
> > > +	help
> > > +	  Generic driver that provides Message Signalled IRQ events
> > > +	  similar to VFIO. If IOMMMU is available please use VFIO
> > > +	  instead since it provides more security.
> > > +	  If you compile this as a module, it will be called uio_msi.
> > > +
> > >  config UIO_NETX
> > >  	tristate "Hilscher NetX Card driver"
> > >  	depends on PCI
> > > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> > > index 8560dad..62fc44b 100644
> > > --- a/drivers/uio/Makefile
> > > +++ b/drivers/uio/Makefile
> > > @@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
> > >  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
> > >  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
> > >  obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
> > > +obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
> > > diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
> > > new file mode 100644
> > > index 0000000..802b5c4
> > > --- /dev/null
> > > +++ b/drivers/uio/uio_msi.c
> > > @@ -0,0 +1,378 @@
> > > +/*-
> > > + *
> > > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > > + * Author: Stephen Hemminger <stephen@networkplumber.org>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 only.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/eventfd.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/uio_driver.h>
> > > +#include <linux/msi.h>
> > > +#include <linux/uio_msi.h>
> > > +
> > > +#define DRIVER_VERSION		"0.1.1"
> > > +#define MAX_MSIX_VECTORS	64
> > > +
> > > +/* MSI-X vector information */
> > > +struct uio_msi_pci_dev {
> > > +	struct uio_info info;		/* UIO driver info */
> > > +	struct pci_dev *pdev;		/* PCI device */
> > > +	struct mutex	mutex;		/* open/release/ioctl mutex */
> > > +	int		ref_cnt;	/* references to device */
> > > +	unsigned int	max_vectors;	/* MSI-X slots available */
> > > +	struct msix_entry *msix;	/* MSI-X vector table */
> > > +	struct uio_msi_irq_ctx {
> > > +		struct eventfd_ctx *trigger; /* vector to eventfd */
> > > +		char *name;		/* name in /proc/interrupts */
> > > +	} *ctx;
> > > +};
> > > +
> > > +static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
> > > +{
> > > +	struct uio_msi_pci_dev *udev = arg;
> > > +
> > > +	if (pci_check_and_mask_intx(udev->pdev)) {
> > > +		eventfd_signal(udev->ctx->trigger, 1);
> > > +		return IRQ_HANDLED;
> > > +	}
> > > +
> > > +	return IRQ_NONE;
> > > +}
> > > +
> > > +static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
> > > +{
> > > +	struct eventfd_ctx *trigger = arg;
> > > +
> > > +	eventfd_signal(trigger, 1);
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +/* set the mapping between vector # and existing eventfd. */
> > > +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> > > +{
> > > +	struct eventfd_ctx *trigger;
> > > +	int irq, err;
> > > +
> > > +	if (vec >= udev->max_vectors) {
> > > +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> > > +			   vec, udev->max_vectors);
> > > +		return -ERANGE;
> > > +	}
> > > +
> > > +	irq = udev->msix[vec].vector;
> > > +	trigger = udev->ctx[vec].trigger;
> > > +	if (trigger) {
> > > +		/* Clearup existing irq mapping */
> > > +		free_irq(irq, trigger);
> > > +		eventfd_ctx_put(trigger);
> > > +		udev->ctx[vec].trigger = NULL;
> > > +	}
> > > +
> > > +	/* Passing -1 is used to disable interrupt */
> > > +	if (fd < 0)
> > > +		return 0;
> > > +
> > > +	trigger = eventfd_ctx_fdget(fd);
> > > +	if (IS_ERR(trigger)) {
> > > +		err = PTR_ERR(trigger);
> > > +		dev_notice(&udev->pdev->dev,
> > > +			   "eventfd ctx get failed: %d\n", err);
> > > +		return err;
> > > +	}
> > > +
> > > +	if (udev->msix)
> > > +		err = request_irq(irq, uio_msi_irqhandler, 0,
> > > +				  udev->ctx[vec].name, trigger);
> > > +	else
> > > +		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
> > > +				  udev->ctx[vec].name, udev);
> > > +
> > > +	if (err) {
> > > +		dev_notice(&udev->pdev->dev,
> > > +			   "request irq failed: %d\n", err);
> > > +		eventfd_ctx_put(trigger);
> > > +		return err;
> > > +	}
> > > +
> > > +	udev->ctx[vec].trigger = trigger;
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> > > +{
> > > +	struct uio_msi_pci_dev *udev
> > > +		= container_of(info, struct uio_msi_pci_dev, info);
> > > +	struct uio_msi_irq_set hdr;
> > > +	int err;
> > > +
> > > +	switch (cmd) {
> > > +	case UIO_MSI_IRQ_SET:
> > > +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> > > +			return -EFAULT;
> > > +
> > > +		mutex_lock(&udev->mutex);
> > > +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> > > +		mutex_unlock(&udev->mutex);
> > > +		break;
> > > +	default:
> > > +		err = -EOPNOTSUPP;
> > > +	}
> > > +	return err;
> > > +}
> > > +
> > > +/* Opening the UIO device for first time enables MSI-X */
> > > +static int
> > > +uio_msi_open(struct uio_info *info, struct inode *inode)
> > > +{
> > > +	struct uio_msi_pci_dev *udev
> > > +		= container_of(info, struct uio_msi_pci_dev, info);
> > > +	int err = 0;
> > > +
> > > +	mutex_lock(&udev->mutex);
> > > +	if (udev->ref_cnt++ == 0) {
> > > +		if (udev->msix)
> > > +			err = pci_enable_msix(udev->pdev, udev->msix,
> > > +					      udev->max_vectors);
> > > +	}
> > > +	mutex_unlock(&udev->mutex);
> > > +
> > > +	return err;
> > > +}
> > > +
> > > +/* Last close of the UIO device releases/disables all IRQ's */
> > > +static int
> > > +uio_msi_release(struct uio_info *info, struct inode *inode)
> > > +{
> > > +	struct uio_msi_pci_dev *udev
> > > +		= container_of(info, struct uio_msi_pci_dev, info);
> > > +	int i;
> > > +
> > > +	mutex_lock(&udev->mutex);
> > > +	if (--udev->ref_cnt == 0) {
> > > +		for (i = 0; i < udev->max_vectors; i++) {
> > > +			int irq = udev->msix[i].vector;
> > > +			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
> > > +
> > > +			if (!trigger)
> > > +				continue;
> > > +
> > > +			free_irq(irq, trigger);
> > > +			eventfd_ctx_put(trigger);
> > > +			udev->ctx[i].trigger = NULL;
> > > +		}
> > > +
> > > +		if (udev->msix)
> > > +			pci_disable_msix(udev->pdev);
> > > +	}
> > > +	mutex_unlock(&udev->mutex);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* Unmap previously ioremap'd resources */
> > > +static void
> > > +release_iomaps(struct uio_mem *mem)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> > > +		if (mem->internal_addr)
> > > +			iounmap(mem->internal_addr);
> > > +	}
> > > +}
> > > +
> > > +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);
> > > +	return err;
> > > +}
> > > +
> > > +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > > +	struct uio_msi_pci_dev *udev;
> > > +	int i, err, vectors;
> > > +
> > > +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> > > +	if (!udev)
> > > +		return -ENOMEM;
> > > +
> > > +	err = pci_enable_device(pdev);
> > > +	if (err != 0) {
> > > +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> > > +		goto fail_free;
> > > +	}
> > > +
> > > +	err = pci_request_regions(pdev, "uio_msi");
> > > +	if (err != 0) {
> > > +		dev_err(&pdev->dev, "Cannot request regions\n");
> > > +		goto fail_disable;
> > > +	}
> > > +
> > > +	pci_set_master(pdev);
> > > +
> > > +	/* remap resources */
> > > +	err = setup_maps(pdev, &udev->info);
> > > +	if (err)
> > > +		goto fail_release_iomem;
> > > +
> > > +	/* fill uio infos */
> > > +	udev->info.name = "uio_msi";
> > > +	udev->info.version = DRIVER_VERSION;
> > > +	udev->info.priv = udev;
> > > +	udev->pdev = pdev;
> > > +	udev->info.ioctl = uio_msi_ioctl;
> > > +	udev->info.open = uio_msi_open;
> > > +	udev->info.release = uio_msi_release;
> > > +	udev->info.irq = UIO_IRQ_CUSTOM;
> > > +	mutex_init(&udev->mutex);
> > > +
> > > +	vectors = pci_msix_vec_count(pdev);
> > > +	if (vectors > 0) {
> > > +		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
> > > +		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
> > > +			udev->max_vectors);
> > > +
> > > +		err = -ENOMEM;
> > > +		udev->msix = kcalloc(udev->max_vectors,
> > > +				     sizeof(struct msix_entry), GFP_KERNEL);
> > > +		if (!udev->msix)
> > > +			goto fail_release_iomem;
> > > +	} else if (!pci_intx_mask_supported(pdev)) {
> > > +		dev_err(&pdev->dev,
> > > +			"device does not support MSI-X or INTX\n");
> > > +		err = -EINVAL;
> > > +		goto fail_release_iomem;
> > > +	} else {
> > > +		dev_notice(&pdev->dev, "using INTX\n");
> > > +		udev->info.irq_flags = IRQF_SHARED;
> > > +		udev->max_vectors = 1;
> > > +	}
> > > +
> > > +	udev->ctx = kcalloc(udev->max_vectors,
> > > +			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
> > > +	if (!udev->ctx)
> > > +		goto fail_free_msix;
> > > +
> > > +	for (i = 0; i < udev->max_vectors; i++) {
> > > +		udev->msix[i].entry = i;
> > > +
> > > +		udev->ctx[i].name = kasprintf(GFP_KERNEL,
> > > +					      KBUILD_MODNAME "[%d](%s)",
> > > +					      i, pci_name(pdev));
> > > +		if (!udev->ctx[i].name)
> > > +			goto fail_free_ctx;
> > > +	}
> > > +
> > > +	/* register uio driver */
> > > +	err = uio_register_device(&pdev->dev, &udev->info);
> > > +	if (err != 0)
> > > +		goto fail_free_ctx;
> > > +
> > > +	pci_set_drvdata(pdev, udev);
> > > +	return 0;
> > > +
> > > +fail_free_ctx:
> > > +	for (i = 0; i < udev->max_vectors; i++)
> > > +		kfree(udev->ctx[i].name);
> > > +	kfree(udev->ctx);
> > > +fail_free_msix:
> > > +	kfree(udev->msix);
> > > +fail_release_iomem:
> > > +	release_iomaps(udev->info.mem);
> > > +	pci_release_regions(pdev);
> > > +fail_disable:
> > > +	pci_disable_device(pdev);
> > > +fail_free:
> > > +	kfree(udev);
> > > +
> > > +	pr_notice("%s ret %d\n", __func__, err);
> > > +	return err;
> > > +}
> > > +
> > > +static void uio_msi_remove(struct pci_dev *pdev)
> > > +{
> > > +	struct uio_info *info = pci_get_drvdata(pdev);
> > > +	struct uio_msi_pci_dev *udev
> > > +		= container_of(info, struct uio_msi_pci_dev, info);
> > > +	int i;
> > > +
> > > +	uio_unregister_device(info);
> > > +	release_iomaps(info->mem);
> > > +
> > > +	pci_release_regions(pdev);
> > > +	for (i = 0; i < udev->max_vectors; i++)
> > > +		kfree(udev->ctx[i].name);
> > > +	kfree(udev->ctx);
> > > +	kfree(udev->msix);
> > > +	pci_disable_device(pdev);
> > > +
> > > +	pci_set_drvdata(pdev, NULL);
> > > +	kfree(udev);
> > > +}
> > > +
> > > +static struct pci_driver uio_msi_pci_driver = {
> > > +	.name = "uio_msi",
> > > +	.probe = uio_msi_probe,
> > > +	.remove = uio_msi_remove,
> > > +};
> > > +
> > > +module_pci_driver(uio_msi_pci_driver);
> > > +MODULE_VERSION(DRIVER_VERSION);
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_AUTHOR("Stephen Hemminger <stephen@networkplumber.org>");
> > > +MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
> > > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > > index f7b2db4..d9497691 100644
> > > --- a/include/uapi/linux/Kbuild
> > > +++ b/include/uapi/linux/Kbuild
> > > @@ -411,6 +411,7 @@ header-y += udp.h
> > >  header-y += uhid.h
> > >  header-y += uinput.h
> > >  header-y += uio.h
> > > +header-y += uio_msi.h
> > >  header-y += ultrasound.h
> > >  header-y += un.h
> > >  header-y += unistd.h
> > > diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
> > > new file mode 100644
> > > index 0000000..297de00
> > > --- /dev/null
> > > +++ b/include/uapi/linux/uio_msi.h
> > > @@ -0,0 +1,22 @@
> > > +/*
> > > + * UIO_MSI API definition
> > > + *
> > > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > > + * All rights reserved.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +#ifndef _UIO_PCI_MSI_H
> > > +#define _UIO_PCI_MSI_H
> > > +
> > > +struct uio_msi_irq_set {
> > > +	u32 vec;
> > > +	int fd;
> > > +};
> > > +
> > > +#define UIO_MSI_BASE	0x86
> > > +#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
> > > +
> > > +#endif
> > > -- 
> > > 2.1.4
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237484 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, 1 Oct 2015 11:33:06 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > This driver allows using PCI device with Message Signalled Interrupt
> > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > file descriptors similar to VFIO.
> >
> > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > have to work in environments where IOMMU support (real or emulated) is
> > not available.  All UIO drivers that support DMA are not secure against
> > rogue userspace applications programming DMA hardware to access
> > private memory; this driver is no less secure than existing code.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> I don't think copying the igb_uio interface is a good idea.
> What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> is abusing the sysfs BAR access to provide unlimited
> access to hardware.
> 
> MSI messages are memory writes so any generic device capable
> of MSI is capable of corrupting kernel memory.
> This means that a bug in userspace will lead to kernel memory corruption
> and crashes.  This is something distributions can't support.
> 
> uio_pci_generic is already abused like that, mostly
> because when I wrote it, I didn't add enough protections
> against using it with DMA capable devices,
> and we can't go back and break working userspace.
> But at least it does not bind to VFs which all of
> them are capable of DMA.
> 
> The result of merging this driver will be userspace abusing the
> sysfs BAR access with VFs as well, and we do not want that.
> 
> 
> Just forwarding events is not enough to make a valid driver.
> What is missing is a way to access the device in a safe way.
> 
> On a more positive note:
> 
> What would be a reasonable interface? One that does the following
> in kernel:
> 
> 1. initializes device rings (can be in pinned userspace memory,
>    but can not be writeable by userspace), brings up interface link
> 2. pins userspace memory (unless using e.g. hugetlbfs)
> 3. gets request, make sure it's valid and belongs to
>    the correct task, put it in the ring
> 4. in the reverse direction, notify userspace when buffers
>    are available in the ring
> 5. notify userspace about MSI (what this driver does)
> 
> What userspace can be allowed to do:
> 
> 	format requests (e.g. transmit, receive) in userspace
> 	read ring contents
> 
> What userspace can't be allowed to do:
> 
> 	access BAR
> 	write rings
> 
> 
> This means that the driver can not be a generic one,
> and there will be a system call overhead when you
> write the ring, but that's the price you have to
> pay for ability to run on systems without an IOMMU.

I think I understand what you are proposing, but it really doesn't
fit into the high speed userspace networking model.

1. Device rings are device specific, can't be in a generic driver.
2. DPDK uses huge mememory.
3. Performance requires all ring requests be done in pure userspace,
   (ie no syscalls)
4. Ditto, can't have kernel to userspace notification per packet

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237505 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 07:50:37AM -0700, Stephen Hemminger wrote:
> On Thu, 1 Oct 2015 11:33:06 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > > This driver allows using PCI device with Message Signalled Interrupt
> > > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > > file descriptors similar to VFIO.
> > >
> > > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > > have to work in environments where IOMMU support (real or emulated) is
> > > not available.  All UIO drivers that support DMA are not secure against
> > > rogue userspace applications programming DMA hardware to access
> > > private memory; this driver is no less secure than existing code.
> > > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > I don't think copying the igb_uio interface is a good idea.
> > What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> > is abusing the sysfs BAR access to provide unlimited
> > access to hardware.
> > 
> > MSI messages are memory writes so any generic device capable
> > of MSI is capable of corrupting kernel memory.
> > This means that a bug in userspace will lead to kernel memory corruption
> > and crashes.  This is something distributions can't support.
> > 
> > uio_pci_generic is already abused like that, mostly
> > because when I wrote it, I didn't add enough protections
> > against using it with DMA capable devices,
> > and we can't go back and break working userspace.
> > But at least it does not bind to VFs which all of
> > them are capable of DMA.
> > 
> > The result of merging this driver will be userspace abusing the
> > sysfs BAR access with VFs as well, and we do not want that.
> > 
> > 
> > Just forwarding events is not enough to make a valid driver.
> > What is missing is a way to access the device in a safe way.
> > 
> > On a more positive note:
> > 
> > What would be a reasonable interface? One that does the following
> > in kernel:
> > 
> > 1. initializes device rings (can be in pinned userspace memory,
> >    but can not be writeable by userspace), brings up interface link
> > 2. pins userspace memory (unless using e.g. hugetlbfs)
> > 3. gets request, make sure it's valid and belongs to
> >    the correct task, put it in the ring
> > 4. in the reverse direction, notify userspace when buffers
> >    are available in the ring
> > 5. notify userspace about MSI (what this driver does)
> > 
> > What userspace can be allowed to do:
> > 
> > 	format requests (e.g. transmit, receive) in userspace
> > 	read ring contents
> > 
> > What userspace can't be allowed to do:
> > 
> > 	access BAR
> > 	write rings
> > 
> > 
> > This means that the driver can not be a generic one,
> > and there will be a system call overhead when you
> > write the ring, but that's the price you have to
> > pay for ability to run on systems without an IOMMU.
> 
> I think I understand what you are proposing, but it really doesn't
> fit into the high speed userspace networking model.

I'm aware of the fact currently the model does everything including
bringing up the link in user-space.
But there's really no justification for this.
Only data path things should be in userspace.

A userspace bug should not be able to do things like over-writing the
on-device EEPROM.


> 1. Device rings are device specific, can't be in a generic driver.

So that's more work, and it is not going to happen if people
can get by with insecure hacks.

> 2. DPDK uses huge mememory.

Hugetlbfs? Don't see why this is an issue. Might make things simpler.

> 3. Performance requires all ring requests be done in pure userspace,
>    (ie no syscalls)

Make only the TX ring writeable then. At least you won't be
able to corrupt the kernel memory.

> 4. Ditto, can't have kernel to userspace notification per packet

RX ring can be read-only, so userspace can read it directly.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237557 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > This driver allows using PCI device with Message Signalled Interrupt
> > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > file descriptors similar to VFIO.
> >
> > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > have to work in environments where IOMMU support (real or emulated) is
> > not available.  All UIO drivers that support DMA are not secure against
> > rogue userspace applications programming DMA hardware to access
> > private memory; this driver is no less secure than existing code.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> I don't think copying the igb_uio interface is a good idea.
> What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> is abusing the sysfs BAR access to provide unlimited
> access to hardware.
> 
> MSI messages are memory writes so any generic device capable
> of MSI is capable of corrupting kernel memory.
> This means that a bug in userspace will lead to kernel memory corruption
> and crashes.  This is something distributions can't support.
> 
> uio_pci_generic is already abused like that, mostly
> because when I wrote it, I didn't add enough protections
> against using it with DMA capable devices,
> and we can't go back and break working userspace.
> But at least it does not bind to VFs which all of
> them are capable of DMA.
> 
> The result of merging this driver will be userspace abusing the
> sysfs BAR access with VFs as well, and we do not want that.
> 
> 
> Just forwarding events is not enough to make a valid driver.
> What is missing is a way to access the device in a safe way.
> 
> On a more positive note:
> 
> What would be a reasonable interface? One that does the following
> in kernel:
> 
> 1. initializes device rings (can be in pinned userspace memory,
>    but can not be writeable by userspace), brings up interface link
> 2. pins userspace memory (unless using e.g. hugetlbfs)
> 3. gets request, make sure it's valid and belongs to
>    the correct task, put it in the ring
> 4. in the reverse direction, notify userspace when buffers
>    are available in the ring
> 5. notify userspace about MSI (what this driver does)
> 
> What userspace can be allowed to do:
> 
> 	format requests (e.g. transmit, receive) in userspace
> 	read ring contents
> 
> What userspace can't be allowed to do:
> 
> 	access BAR
> 	write rings
> 
> 
> This means that the driver can not be a generic one,
> and there will be a system call overhead when you
> write the ring, but that's the price you have to
> pay for ability to run on systems without an IOMMU.
> 


The device specific parts can be taken from John Fastabend's patches
BTW:

https://patchwork.ozlabs.org/patch/396713/

IIUC what was missing there was exactly the memory protection
we are looking for here.


> 
> 
> > ---
> >  drivers/uio/Kconfig          |   9 ++
> >  drivers/uio/Makefile         |   1 +
> >  drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/Kbuild    |   1 +
> >  include/uapi/linux/uio_msi.h |  22 +++
> >  5 files changed, 411 insertions(+)
> >  create mode 100644 drivers/uio/uio_msi.c
> >  create mode 100644 include/uapi/linux/uio_msi.h
> > 
> > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > index 52c98ce..04adfa0 100644
> > --- a/drivers/uio/Kconfig
> > +++ b/drivers/uio/Kconfig
> > @@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
> >  	  primarily, for virtualization scenarios.
> >  	  If you compile this as a module, it will be called uio_pci_generic.
> >  
> > +config UIO_PCI_MSI
> > +       tristate "Generic driver supporting MSI-x on PCI Express cards"
> > +	depends on PCI
> > +	help
> > +	  Generic driver that provides Message Signalled IRQ events
> > +	  similar to VFIO. If IOMMMU is available please use VFIO
> > +	  instead since it provides more security.
> > +	  If you compile this as a module, it will be called uio_msi.
> > +
> >  config UIO_NETX
> >  	tristate "Hilscher NetX Card driver"
> >  	depends on PCI
> > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> > index 8560dad..62fc44b 100644
> > --- a/drivers/uio/Makefile
> > +++ b/drivers/uio/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
> >  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
> >  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
> >  obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
> > +obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
> > diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
> > new file mode 100644
> > index 0000000..802b5c4
> > --- /dev/null
> > +++ b/drivers/uio/uio_msi.c
> > @@ -0,0 +1,378 @@
> > +/*-
> > + *
> > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > + * Author: Stephen Hemminger <stephen@networkplumber.org>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 only.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/msi.h>
> > +#include <linux/uio_msi.h>
> > +
> > +#define DRIVER_VERSION		"0.1.1"
> > +#define MAX_MSIX_VECTORS	64
> > +
> > +/* MSI-X vector information */
> > +struct uio_msi_pci_dev {
> > +	struct uio_info info;		/* UIO driver info */
> > +	struct pci_dev *pdev;		/* PCI device */
> > +	struct mutex	mutex;		/* open/release/ioctl mutex */
> > +	int		ref_cnt;	/* references to device */
> > +	unsigned int	max_vectors;	/* MSI-X slots available */
> > +	struct msix_entry *msix;	/* MSI-X vector table */
> > +	struct uio_msi_irq_ctx {
> > +		struct eventfd_ctx *trigger; /* vector to eventfd */
> > +		char *name;		/* name in /proc/interrupts */
> > +	} *ctx;
> > +};
> > +
> > +static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
> > +{
> > +	struct uio_msi_pci_dev *udev = arg;
> > +
> > +	if (pci_check_and_mask_intx(udev->pdev)) {
> > +		eventfd_signal(udev->ctx->trigger, 1);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
> > +{
> > +	struct eventfd_ctx *trigger = arg;
> > +
> > +	eventfd_signal(trigger, 1);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/* set the mapping between vector # and existing eventfd. */
> > +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> > +{
> > +	struct eventfd_ctx *trigger;
> > +	int irq, err;
> > +
> > +	if (vec >= udev->max_vectors) {
> > +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> > +			   vec, udev->max_vectors);
> > +		return -ERANGE;
> > +	}
> > +
> > +	irq = udev->msix[vec].vector;
> > +	trigger = udev->ctx[vec].trigger;
> > +	if (trigger) {
> > +		/* Clearup existing irq mapping */
> > +		free_irq(irq, trigger);
> > +		eventfd_ctx_put(trigger);
> > +		udev->ctx[vec].trigger = NULL;
> > +	}
> > +
> > +	/* Passing -1 is used to disable interrupt */
> > +	if (fd < 0)
> > +		return 0;
> > +
> > +	trigger = eventfd_ctx_fdget(fd);
> > +	if (IS_ERR(trigger)) {
> > +		err = PTR_ERR(trigger);
> > +		dev_notice(&udev->pdev->dev,
> > +			   "eventfd ctx get failed: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	if (udev->msix)
> > +		err = request_irq(irq, uio_msi_irqhandler, 0,
> > +				  udev->ctx[vec].name, trigger);
> > +	else
> > +		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
> > +				  udev->ctx[vec].name, udev);
> > +
> > +	if (err) {
> > +		dev_notice(&udev->pdev->dev,
> > +			   "request irq failed: %d\n", err);
> > +		eventfd_ctx_put(trigger);
> > +		return err;
> > +	}
> > +
> > +	udev->ctx[vec].trigger = trigger;
> > +	return 0;
> > +}
> > +
> > +static int
> > +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	struct uio_msi_irq_set hdr;
> > +	int err;
> > +
> > +	switch (cmd) {
> > +	case UIO_MSI_IRQ_SET:
> > +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> > +			return -EFAULT;
> > +
> > +		mutex_lock(&udev->mutex);
> > +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> > +		mutex_unlock(&udev->mutex);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +	}
> > +	return err;
> > +}
> > +
> > +/* Opening the UIO device for first time enables MSI-X */
> > +static int
> > +uio_msi_open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int err = 0;
> > +
> > +	mutex_lock(&udev->mutex);
> > +	if (udev->ref_cnt++ == 0) {
> > +		if (udev->msix)
> > +			err = pci_enable_msix(udev->pdev, udev->msix,
> > +					      udev->max_vectors);
> > +	}
> > +	mutex_unlock(&udev->mutex);
> > +
> > +	return err;
> > +}
> > +
> > +/* Last close of the UIO device releases/disables all IRQ's */
> > +static int
> > +uio_msi_release(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int i;
> > +
> > +	mutex_lock(&udev->mutex);
> > +	if (--udev->ref_cnt == 0) {
> > +		for (i = 0; i < udev->max_vectors; i++) {
> > +			int irq = udev->msix[i].vector;
> > +			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
> > +
> > +			if (!trigger)
> > +				continue;
> > +
> > +			free_irq(irq, trigger);
> > +			eventfd_ctx_put(trigger);
> > +			udev->ctx[i].trigger = NULL;
> > +		}
> > +
> > +		if (udev->msix)
> > +			pci_disable_msix(udev->pdev);
> > +	}
> > +	mutex_unlock(&udev->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Unmap previously ioremap'd resources */
> > +static void
> > +release_iomaps(struct uio_mem *mem)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> > +		if (mem->internal_addr)
> > +			iounmap(mem->internal_addr);
> > +	}
> > +}
> > +
> > +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);
> > +	return err;
> > +}
> > +
> > +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct uio_msi_pci_dev *udev;
> > +	int i, err, vectors;
> > +
> > +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> > +	if (!udev)
> > +		return -ENOMEM;
> > +
> > +	err = pci_enable_device(pdev);
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> > +		goto fail_free;
> > +	}
> > +
> > +	err = pci_request_regions(pdev, "uio_msi");
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "Cannot request regions\n");
> > +		goto fail_disable;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	/* remap resources */
> > +	err = setup_maps(pdev, &udev->info);
> > +	if (err)
> > +		goto fail_release_iomem;
> > +
> > +	/* fill uio infos */
> > +	udev->info.name = "uio_msi";
> > +	udev->info.version = DRIVER_VERSION;
> > +	udev->info.priv = udev;
> > +	udev->pdev = pdev;
> > +	udev->info.ioctl = uio_msi_ioctl;
> > +	udev->info.open = uio_msi_open;
> > +	udev->info.release = uio_msi_release;
> > +	udev->info.irq = UIO_IRQ_CUSTOM;
> > +	mutex_init(&udev->mutex);
> > +
> > +	vectors = pci_msix_vec_count(pdev);
> > +	if (vectors > 0) {
> > +		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
> > +		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
> > +			udev->max_vectors);
> > +
> > +		err = -ENOMEM;
> > +		udev->msix = kcalloc(udev->max_vectors,
> > +				     sizeof(struct msix_entry), GFP_KERNEL);
> > +		if (!udev->msix)
> > +			goto fail_release_iomem;
> > +	} else if (!pci_intx_mask_supported(pdev)) {
> > +		dev_err(&pdev->dev,
> > +			"device does not support MSI-X or INTX\n");
> > +		err = -EINVAL;
> > +		goto fail_release_iomem;
> > +	} else {
> > +		dev_notice(&pdev->dev, "using INTX\n");
> > +		udev->info.irq_flags = IRQF_SHARED;
> > +		udev->max_vectors = 1;
> > +	}
> > +
> > +	udev->ctx = kcalloc(udev->max_vectors,
> > +			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
> > +	if (!udev->ctx)
> > +		goto fail_free_msix;
> > +
> > +	for (i = 0; i < udev->max_vectors; i++) {
> > +		udev->msix[i].entry = i;
> > +
> > +		udev->ctx[i].name = kasprintf(GFP_KERNEL,
> > +					      KBUILD_MODNAME "[%d](%s)",
> > +					      i, pci_name(pdev));
> > +		if (!udev->ctx[i].name)
> > +			goto fail_free_ctx;
> > +	}
> > +
> > +	/* register uio driver */
> > +	err = uio_register_device(&pdev->dev, &udev->info);
> > +	if (err != 0)
> > +		goto fail_free_ctx;
> > +
> > +	pci_set_drvdata(pdev, udev);
> > +	return 0;
> > +
> > +fail_free_ctx:
> > +	for (i = 0; i < udev->max_vectors; i++)
> > +		kfree(udev->ctx[i].name);
> > +	kfree(udev->ctx);
> > +fail_free_msix:
> > +	kfree(udev->msix);
> > +fail_release_iomem:
> > +	release_iomaps(udev->info.mem);
> > +	pci_release_regions(pdev);
> > +fail_disable:
> > +	pci_disable_device(pdev);
> > +fail_free:
> > +	kfree(udev);
> > +
> > +	pr_notice("%s ret %d\n", __func__, err);
> > +	return err;
> > +}
> > +
> > +static void uio_msi_remove(struct pci_dev *pdev)
> > +{
> > +	struct uio_info *info = pci_get_drvdata(pdev);
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int i;
> > +
> > +	uio_unregister_device(info);
> > +	release_iomaps(info->mem);
> > +
> > +	pci_release_regions(pdev);
> > +	for (i = 0; i < udev->max_vectors; i++)
> > +		kfree(udev->ctx[i].name);
> > +	kfree(udev->ctx);
> > +	kfree(udev->msix);
> > +	pci_disable_device(pdev);
> > +
> > +	pci_set_drvdata(pdev, NULL);
> > +	kfree(udev);
> > +}
> > +
> > +static struct pci_driver uio_msi_pci_driver = {
> > +	.name = "uio_msi",
> > +	.probe = uio_msi_probe,
> > +	.remove = uio_msi_remove,
> > +};
> > +
> > +module_pci_driver(uio_msi_pci_driver);
> > +MODULE_VERSION(DRIVER_VERSION);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Stephen Hemminger <stephen@networkplumber.org>");
> > +MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
> > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > index f7b2db4..d9497691 100644
> > --- a/include/uapi/linux/Kbuild
> > +++ b/include/uapi/linux/Kbuild
> > @@ -411,6 +411,7 @@ header-y += udp.h
> >  header-y += uhid.h
> >  header-y += uinput.h
> >  header-y += uio.h
> > +header-y += uio_msi.h
> >  header-y += ultrasound.h
> >  header-y += un.h
> >  header-y += unistd.h
> > diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
> > new file mode 100644
> > index 0000000..297de00
> > --- /dev/null
> > +++ b/include/uapi/linux/uio_msi.h
> > @@ -0,0 +1,22 @@
> > +/*
> > + * UIO_MSI API definition
> > + *
> > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > + * All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef _UIO_PCI_MSI_H
> > +#define _UIO_PCI_MSI_H
> > +
> > +struct uio_msi_irq_set {
> > +	u32 vec;
> > +	int fd;
> > +};
> > +
> > +#define UIO_MSI_BASE	0x86
> > +#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
> > +
> > +#endif
> > -- 
> > 2.1.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237590 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, 1 Oct 2015 19:31:08 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> > On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > > This driver allows using PCI device with Message Signalled Interrupt
> > > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > > file descriptors similar to VFIO.
> > >
> > > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > > have to work in environments where IOMMU support (real or emulated) is
> > > not available.  All UIO drivers that support DMA are not secure against
> > > rogue userspace applications programming DMA hardware to access
> > > private memory; this driver is no less secure than existing code.
> > > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > I don't think copying the igb_uio interface is a good idea.
> > What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> > is abusing the sysfs BAR access to provide unlimited
> > access to hardware.
> > 
> > MSI messages are memory writes so any generic device capable
> > of MSI is capable of corrupting kernel memory.
> > This means that a bug in userspace will lead to kernel memory corruption
> > and crashes.  This is something distributions can't support.
> > 
> > uio_pci_generic is already abused like that, mostly
> > because when I wrote it, I didn't add enough protections
> > against using it with DMA capable devices,
> > and we can't go back and break working userspace.
> > But at least it does not bind to VFs which all of
> > them are capable of DMA.
> > 
> > The result of merging this driver will be userspace abusing the
> > sysfs BAR access with VFs as well, and we do not want that.
> > 
> > 
> > Just forwarding events is not enough to make a valid driver.
> > What is missing is a way to access the device in a safe way.
> > 
> > On a more positive note:
> > 
> > What would be a reasonable interface? One that does the following
> > in kernel:
> > 
> > 1. initializes device rings (can be in pinned userspace memory,
> >    but can not be writeable by userspace), brings up interface link
> > 2. pins userspace memory (unless using e.g. hugetlbfs)
> > 3. gets request, make sure it's valid and belongs to
> >    the correct task, put it in the ring
> > 4. in the reverse direction, notify userspace when buffers
> >    are available in the ring
> > 5. notify userspace about MSI (what this driver does)
> > 
> > What userspace can be allowed to do:
> > 
> > 	format requests (e.g. transmit, receive) in userspace
> > 	read ring contents
> > 
> > What userspace can't be allowed to do:
> > 
> > 	access BAR
> > 	write rings
> > 
> > 
> > This means that the driver can not be a generic one,
> > and there will be a system call overhead when you
> > write the ring, but that's the price you have to
> > pay for ability to run on systems without an IOMMU.
> > 
> 
> 
> The device specific parts can be taken from John Fastabend's patches
> BTW:
> 
> https://patchwork.ozlabs.org/patch/396713/
> 
> IIUC what was missing there was exactly the memory protection
> we are looking for here.

The bifuricated drivers are interesting from an architecture
point of view, but do nothing to solve the immediate use case.
The problem is not on bare metal environment, most of those already have IOMMU.
The issues are on environments like VMWare with SRIOV or vmxnet3,
neither of those are really helped by bifirucated driver or VFIO.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237633 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 10:26:19AM -0700, Stephen Hemminger wrote:
> On Thu, 1 Oct 2015 19:31:08 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > > > This driver allows using PCI device with Message Signalled Interrupt
> > > > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > > > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > > > file descriptors similar to VFIO.
> > > >
> > > > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > > > have to work in environments where IOMMU support (real or emulated) is
> > > > not available.  All UIO drivers that support DMA are not secure against
> > > > rogue userspace applications programming DMA hardware to access
> > > > private memory; this driver is no less secure than existing code.
> > > > 
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > 
> > > I don't think copying the igb_uio interface is a good idea.
> > > What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> > > is abusing the sysfs BAR access to provide unlimited
> > > access to hardware.
> > > 
> > > MSI messages are memory writes so any generic device capable
> > > of MSI is capable of corrupting kernel memory.
> > > This means that a bug in userspace will lead to kernel memory corruption
> > > and crashes.  This is something distributions can't support.
> > > 
> > > uio_pci_generic is already abused like that, mostly
> > > because when I wrote it, I didn't add enough protections
> > > against using it with DMA capable devices,
> > > and we can't go back and break working userspace.
> > > But at least it does not bind to VFs which all of
> > > them are capable of DMA.
> > > 
> > > The result of merging this driver will be userspace abusing the
> > > sysfs BAR access with VFs as well, and we do not want that.
> > > 
> > > 
> > > Just forwarding events is not enough to make a valid driver.
> > > What is missing is a way to access the device in a safe way.
> > > 
> > > On a more positive note:
> > > 
> > > What would be a reasonable interface? One that does the following
> > > in kernel:
> > > 
> > > 1. initializes device rings (can be in pinned userspace memory,
> > >    but can not be writeable by userspace), brings up interface link
> > > 2. pins userspace memory (unless using e.g. hugetlbfs)
> > > 3. gets request, make sure it's valid and belongs to
> > >    the correct task, put it in the ring
> > > 4. in the reverse direction, notify userspace when buffers
> > >    are available in the ring
> > > 5. notify userspace about MSI (what this driver does)
> > > 
> > > What userspace can be allowed to do:
> > > 
> > > 	format requests (e.g. transmit, receive) in userspace
> > > 	read ring contents
> > > 
> > > What userspace can't be allowed to do:
> > > 
> > > 	access BAR
> > > 	write rings
> > > 
> > > 
> > > This means that the driver can not be a generic one,
> > > and there will be a system call overhead when you
> > > write the ring, but that's the price you have to
> > > pay for ability to run on systems without an IOMMU.
> > > 
> > 
> > 
> > The device specific parts can be taken from John Fastabend's patches
> > BTW:
> > 
> > https://patchwork.ozlabs.org/patch/396713/
> > 
> > IIUC what was missing there was exactly the memory protection
> > we are looking for here.
> 
> The bifuricated drivers are interesting from an architecture
> point of view, but do nothing to solve the immediate use case.
> The problem is not on bare metal environment, most of those already have IOMMU.
> The issues are on environments like VMWare with SRIOV or vmxnet3,
> neither of those are really helped by bifirucated driver or VFIO.

Two points I tried to make (and apparently failed, so I'm trying again,
more verbosely):
- bifurcated drivers do DMA into both kernel and userspace
  memory from same pci address (bus/dev/fn). As IOMMU uses this
  source address to validated accesses, there is no way to
  have IOMMU prevent userspace from accessing kernel memory.
  If you are prepared to use dynamic mappings for kernel
  memory, it might be possible to limit the harm that userspace
  can do, but this will slow down kernel networking
  (changing IOMMU mappings is expensive) and userspace will
  likely be able to at least disrupt kernel networking.
  So what I am discussing might still have value there.

- bifurcated drivers have code to bring up link and map rings into
  userspace (they also map other rings into kernel, and tweak rx filter
  in hardware, that might not be necessary for this usecase).
  What I proposed above can use that code, with
  the twist that the RX ring is made RO for userspace, and a system call
  to safely copy from userspace ring there is supported.
  In other words, here's the device specific part in
  kernel that you wanted to use, which will only
  need some tweaks.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239988 — Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> Just forwarding events is not enough to make a valid driver.
> What is missing is a way to access the device in a safe way.

Thinking about it some more, maybe some devices don't do DMA, and merely
signal events with MSI/MSI-X.

The fact you mention igb_uio in the cover letter seems to hint that this
isn't the case, and that the real intent is to abuse it for DMA-capable
devices, but still ...

If we assume such a simple device, we need to block userspace from
tweaking at least the MSI control and the MSI-X table.
And changing BARs might make someone else corrupt the MSI-X
table, so we need to block it from changing BARs, too.

Things like device reset will clear the table.  I guess this means we
need to track access to reset, too, make sure we restore the
table to a sane config.

PM  capability can be used to reset things tooI think. Better be
careful about that.

And a bunch of devices could be doing weird things that need
to be special-cased.

All of this is what VFIO is already dealing with.

Maybe extending VFIO for this usecase, or finding another way to share
code might be a better idea than duplicating the code within uio?

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237835 — Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X

On 09/30/2015 03:28 PM, Stephen Hemminger wrote:
> This driver allows using PCI device with Message Signalled Interrupt
> from userspace. The API is similar to the igb_uio driver used by the DPDK.
> Via ioctl it provides a mechanism to map MSI-X interrupts into event
> file descriptors similar to VFIO.
>
> VFIO is a better choice if IOMMU is available, but often userspace drivers
> have to work in environments where IOMMU support (real or emulated) is
> not available.  All UIO drivers that support DMA are not secure against
> rogue userspace applications programming DMA hardware to access
> private memory; this driver is no less secure than existing code.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/uio/Kconfig          |   9 ++
>   drivers/uio/Makefile         |   1 +
>   drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/Kbuild    |   1 +
>   include/uapi/linux/uio_msi.h |  22 +++
>   5 files changed, 411 insertions(+)
>   create mode 100644 drivers/uio/uio_msi.c
>   create mode 100644 include/uapi/linux/uio_msi.h
>
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 52c98ce..04adfa0 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
>   	  primarily, for virtualization scenarios.
>   	  If you compile this as a module, it will be called uio_pci_generic.
>
> +config UIO_PCI_MSI
> +       tristate "Generic driver supporting MSI-x on PCI Express cards"
> +	depends on PCI
> +	help
> +	  Generic driver that provides Message Signalled IRQ events
> +	  similar to VFIO. If IOMMMU is available please use VFIO
> +	  instead since it provides more security.
> +	  If you compile this as a module, it will be called uio_msi.
> +
>   config UIO_NETX
>   	tristate "Hilscher NetX Card driver"
>   	depends on PCI

Should you maybe instead depend on CONFIG_PCI_MSI.  Without MSI this is 
essentially just uio_pci_generic with a bit more greedy mapping setup.

> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 8560dad..62fc44b 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
>   obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
>   obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
>   obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
> +obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
> diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
> new file mode 100644
> index 0000000..802b5c4
> --- /dev/null
> +++ b/drivers/uio/uio_msi.c
> @@ -0,0 +1,378 @@
> +/*-
> + *
> + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> + * Author: Stephen Hemminger <stephen@networkplumber.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 only.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/eventfd.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/uio_driver.h>
> +#include <linux/msi.h>
> +#include <linux/uio_msi.h>
> +
> +#define DRIVER_VERSION		"0.1.1"
> +#define MAX_MSIX_VECTORS	64
> +
> +/* MSI-X vector information */
> +struct uio_msi_pci_dev {
> +	struct uio_info info;		/* UIO driver info */
> +	struct pci_dev *pdev;		/* PCI device */
> +	struct mutex	mutex;		/* open/release/ioctl mutex */
> +	int		ref_cnt;	/* references to device */
> +	unsigned int	max_vectors;	/* MSI-X slots available */
> +	struct msix_entry *msix;	/* MSI-X vector table */
> +	struct uio_msi_irq_ctx {
> +		struct eventfd_ctx *trigger; /* vector to eventfd */
> +		char *name;		/* name in /proc/interrupts */
> +	} *ctx;
> +};
> +

I would move the definition of uio_msi_irq_ctx out of uio_msi_pci_dev. 
It would help to make this a bit more readable.

> +static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
> +{
> +	struct uio_msi_pci_dev *udev = arg;
> +
> +	if (pci_check_and_mask_intx(udev->pdev)) {
> +		eventfd_signal(udev->ctx->trigger, 1);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +

I would really prefer to see the intx handling dropped since there are 
already 2 different UIO drivers setup for handling INTx style 
interrupts.  Lets focus on the parts from the last decade and drop 
support for INTx now in favor of MSI-X and maybe MSI.  If we _REALLY_ 
need it we can always come back later and add it.

> +static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
> +{
> +	struct eventfd_ctx *trigger = arg;
> +
> +	eventfd_signal(trigger, 1);
> +	return IRQ_HANDLED;
> +}
> +
> +/* set the mapping between vector # and existing eventfd. */
> +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> +{
> +	struct eventfd_ctx *trigger;
> +	int irq, err;
> +
> +	if (vec >= udev->max_vectors) {
> +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> +			   vec, udev->max_vectors);
> +		return -ERANGE;
> +	}
> +
> +	irq = udev->msix[vec].vector;
> +	trigger = udev->ctx[vec].trigger;
> +	if (trigger) {
> +		/* Clearup existing irq mapping */

Minor spelling issue here, "Clear up" should be 2 words, "Cleanup" can 
be one.

> +		free_irq(irq, trigger);
> +		eventfd_ctx_put(trigger);
> +		udev->ctx[vec].trigger = NULL;
> +	}
> +
> +	/* Passing -1 is used to disable interrupt */
> +	if (fd < 0)
> +		return 0;
> +
> +	trigger = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(trigger)) {
> +		err = PTR_ERR(trigger);
> +		dev_notice(&udev->pdev->dev,
> +			   "eventfd ctx get failed: %d\n", err);
> +		return err;
> +	}
> +
> +	if (udev->msix)
> +		err = request_irq(irq, uio_msi_irqhandler, 0,
> +				  udev->ctx[vec].name, trigger);
> +	else
> +		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
> +				  udev->ctx[vec].name, udev);
> +
> +	if (err) {
> +		dev_notice(&udev->pdev->dev,
> +			   "request irq failed: %d\n", err);
> +		eventfd_ctx_put(trigger);
> +		return err;
> +	}
> +
> +	udev->ctx[vec].trigger = trigger;
> +	return 0;
> +}
> +
> +static int
> +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	struct uio_msi_irq_set hdr;
> +	int err;
> +
> +	switch (cmd) {
> +	case UIO_MSI_IRQ_SET:
> +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> +			return -EFAULT;
> +
> +		mutex_lock(&udev->mutex);
> +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> +		mutex_unlock(&udev->mutex);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +	return err;
> +}
> +
> +/* Opening the UIO device for first time enables MSI-X */
> +static int
> +uio_msi_open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	int err = 0;
> +
> +	mutex_lock(&udev->mutex);
> +	if (udev->ref_cnt++ == 0) {
> +		if (udev->msix)
> +			err = pci_enable_msix(udev->pdev, udev->msix,
> +					      udev->max_vectors);
> +	}
> +	mutex_unlock(&udev->mutex);
> +
> +	return err;
> +}
> +

I agree with some other reviewers.  Why call pci_enable_msix in open? 
It seems like it would make much more sense to do this on probe, and 
then disable MSI-X on free.  I can only assume you are trying to do it 
to save on resources but the fact is this is a driver you have to 
explicitly force onto a device so you would probably be safe to assume 
that they plan to use it in the near future.

> +/* Last close of the UIO device releases/disables all IRQ's */
> +static int
> +uio_msi_release(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	int i;
> +
> +	mutex_lock(&udev->mutex);
> +	if (--udev->ref_cnt == 0) {
> +		for (i = 0; i < udev->max_vectors; i++) {
> +			int irq = udev->msix[i].vector;
> +			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
> +
> +			if (!trigger)
> +				continue;
> +
> +			free_irq(irq, trigger);
> +			eventfd_ctx_put(trigger);
> +			udev->ctx[i].trigger = NULL;
> +		}
> +
> +		if (udev->msix)
> +			pci_disable_msix(udev->pdev);
> +	}
> +	mutex_unlock(&udev->mutex);
> +
> +	return 0;
> +}
> +
> +/* Unmap previously ioremap'd resources */
> +static void
> +release_iomaps(struct uio_mem *mem)
> +{
> +	int i;
> +
> +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> +		if (mem->internal_addr)
> +			iounmap(mem->internal_addr);
> +	}
> +}
> +
> +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);
> +	return err;
> +}
> +

Do you really need to map IORESOURCE bars?  Most drivers I can think of 
don't use IO BARs anymore.  Maybe we could look at just dropping the 
code and adding it back later if we have a use case that absolutely 
needs it.

Also how many devices actually need resources beyond BAR 0?  I'm just 
curious as I know BAR 2 on many of the Intel devices is the register 
space related to MSI-X so now we have both the PCIe subsystem and user 
space with access to this region.

> +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct uio_msi_pci_dev *udev;
> +	int i, err, vectors;
> +
> +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> +	if (!udev)
> +		return -ENOMEM;
> +
> +	err = pci_enable_device(pdev);
> +	if (err != 0) {
> +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> +		goto fail_free;
> +	}
> +
> +	err = pci_request_regions(pdev, "uio_msi");
> +	if (err != 0) {
> +		dev_err(&pdev->dev, "Cannot request regions\n");
> +		goto fail_disable;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	/* remap resources */
> +	err = setup_maps(pdev, &udev->info);
> +	if (err)
> +		goto fail_release_iomem;
> +
> +	/* fill uio infos */
> +	udev->info.name = "uio_msi";
> +	udev->info.version = DRIVER_VERSION;
> +	udev->info.priv = udev;
> +	udev->pdev = pdev;
> +	udev->info.ioctl = uio_msi_ioctl;
> +	udev->info.open = uio_msi_open;
> +	udev->info.release = uio_msi_release;
> +	udev->info.irq = UIO_IRQ_CUSTOM;
> +	mutex_init(&udev->mutex);
> +
> +	vectors = pci_msix_vec_count(pdev);
> +	if (vectors > 0) {
> +		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
> +		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
> +			udev->max_vectors);
> +
> +		err = -ENOMEM;
> +		udev->msix = kcalloc(udev->max_vectors,
> +				     sizeof(struct msix_entry), GFP_KERNEL);
> +		if (!udev->msix)
> +			goto fail_release_iomem;
> +	} else if (!pci_intx_mask_supported(pdev)) {
> +		dev_err(&pdev->dev,
> +			"device does not support MSI-X or INTX\n");
> +		err = -EINVAL;
> +		goto fail_release_iomem;
> +	} else {
> +		dev_notice(&pdev->dev, "using INTX\n");
> +		udev->info.irq_flags = IRQF_SHARED;
> +		udev->max_vectors = 1;
> +	}
> +
> +	udev->ctx = kcalloc(udev->max_vectors,
> +			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
> +	if (!udev->ctx)
> +		goto fail_free_msix;
> +
> +	for (i = 0; i < udev->max_vectors; i++) {
> +		udev->msix[i].entry = i;
> +
> +		udev->ctx[i].name = kasprintf(GFP_KERNEL,
> +					      KBUILD_MODNAME "[%d](%s)",
> +					      i, pci_name(pdev));
> +		if (!udev->ctx[i].name)
> +			goto fail_free_ctx;
> +	}
> +
> +	/* register uio driver */
> +	err = uio_register_device(&pdev->dev, &udev->info);
> +	if (err != 0)
> +		goto fail_free_ctx;
> +
> +	pci_set_drvdata(pdev, udev);
> +	return 0;
> +
> +fail_free_ctx:
> +	for (i = 0; i < udev->max_vectors; i++)
> +		kfree(udev->ctx[i].name);
> +	kfree(udev->ctx);
> +fail_free_msix:
> +	kfree(udev->msix);
> +fail_release_iomem:
> +	release_iomaps(udev->info.mem);
> +	pci_release_regions(pdev);
> +fail_disable:
> +	pci_disable_device(pdev);
> +fail_free:
> +	kfree(udev);
> +
> +	pr_notice("%s ret %d\n", __func__, err);
> +	return err;
> +}
> +
> +static void uio_msi_remove(struct pci_dev *pdev)
> +{
> +	struct uio_info *info = pci_get_drvdata(pdev);
> +	struct uio_msi_pci_dev *udev
> +		= container_of(info, struct uio_msi_pci_dev, info);
> +	int i;
> +
> +	uio_unregister_device(info);
> +	release_iomaps(info->mem);
> +
> +	pci_release_regions(pdev);
> +	for (i = 0; i < udev->max_vectors; i++)
> +		kfree(udev->ctx[i].name);
> +	kfree(udev->ctx);
> +	kfree(udev->msix);
> +	pci_disable_device(pdev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +	kfree(udev);
> +}
> +
> +static struct pci_driver uio_msi_pci_driver = {
> +	.name = "uio_msi",
> +	.probe = uio_msi_probe,
> +	.remove = uio_msi_remove,
> +};
> +
> +module_pci_driver(uio_msi_pci_driver);
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Stephen Hemminger <stephen@networkplumber.org>");
> +MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index f7b2db4..d9497691 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -411,6 +411,7 @@ header-y += udp.h
>   header-y += uhid.h
>   header-y += uinput.h
>   header-y += uio.h
> +header-y += uio_msi.h
>   header-y += ultrasound.h
>   header-y += un.h
>   header-y += unistd.h
> diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
> new file mode 100644
> index 0000000..297de00
> --- /dev/null
> +++ b/include/uapi/linux/uio_msi.h
> @@ -0,0 +1,22 @@
> +/*
> + * UIO_MSI API definition
> + *
> + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _UIO_PCI_MSI_H
> +#define _UIO_PCI_MSI_H
> +
> +struct uio_msi_irq_set {
> +	u32 vec;
> +	int fd;
> +};
> +
> +#define UIO_MSI_BASE	0x86
> +#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
> +
> +#endif
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237838 — Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, 1 Oct 2015 16:40:10 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> Do you really need to map IORESOURCE bars?  Most drivers I can think of 
> don't use IO BARs anymore.  Maybe we could look at just dropping the 
> code and adding it back later if we have a use case that absolutely 
> needs it.
Mapping is not strictly necessary, but for virtio it acts a way to communicate
the regions.

> Also how many devices actually need resources beyond BAR 0?  I'm just 
> curious as I know BAR 2 on many of the Intel devices is the register 
> space related to MSI-X so now we have both the PCIe subsystem and user 
> space with access to this region.

VMXNet3 needs 2 bars. Most use only one.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237883 — Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X

On 10/01/2015 05:04 PM, Stephen Hemminger wrote:
> On Thu, 1 Oct 2015 16:40:10 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> Do you really need to map IORESOURCE bars?  Most drivers I can think of
>> don't use IO BARs anymore.  Maybe we could look at just dropping the
>> code and adding it back later if we have a use case that absolutely
>> needs it.
> Mapping is not strictly necessary, but for virtio it acts a way to communicate
> the regions.

I think I see what you are saying.  I was hoping we could get away from 
having to map any I/O ports but it looks like virtio is still using them 
for BAR 0, or at least that is what I am seeing on my VM with 
virtio_net.  I was really hoping we could get away from that since a 16b 
address space is far too restrictive anyway.

>> Also how many devices actually need resources beyond BAR 0?  I'm just
>> curious as I know BAR 2 on many of the Intel devices is the register
>> space related to MSI-X so now we have both the PCIe subsystem and user
>> space with access to this region.
> VMXNet3 needs 2 bars. Most use only one.

So essentially we are needing to make exceptions for the virtual interfaces.

I guess there isn't much we can do then and we probably need to map any 
and all base address registers we can find for the given device.  I was 
hoping for something a bit more surgical since we are opening a security 
hole of sorts, but I guess it can't be helped if we want to support 
multiple devices and they all have such radically different configurations.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237842 — Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, 1 Oct 2015 16:40:10 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> I agree with some other reviewers.  Why call pci_enable_msix in open? 
> It seems like it would make much more sense to do this on probe, and 
> then disable MSI-X on free.  I can only assume you are trying to do it 
> to save on resources but the fact is this is a driver you have to 
> explicitly force onto a device so you would probably be safe to assume 
> that they plan to use it in the near future.

Because if interface is not up, the MSI handle doesn't have to be open.
This saves resources and avoids some races.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237870 — Re: [dpdk-dev] [PATCH 2/2] uio: new driver to support PCI MSI-X

On 10/01/2015 05:01 PM, Stephen Hemminger wrote:
> On Thu, 1 Oct 2015 16:40:10 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> I agree with some other reviewers.  Why call pci_enable_msix in open?
>> It seems like it would make much more sense to do this on probe, and
>> then disable MSI-X on free.  I can only assume you are trying to do it
>> to save on resources but the fact is this is a driver you have to
>> explicitly force onto a device so you would probably be safe to assume
>> that they plan to use it in the near future.
> Because if interface is not up, the MSI handle doesn't have to be open.
> This saves resources and avoids some races.

Yes, but it makes things a bit messier for the interrupts.  Most drivers 
take care of interrupts during probe so that if there are any allocation 
problems they can take care of them then instead of leaving an interface 
out that will later fail when it is brought up.  It ends up being a way 
to deal with the whole MSI-X fall-back issue.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236833 — [PATCH 1/2] uio: add support for ioctls

Allow UIO device driver to provide ioctl interface.
This allow additional API's for UIO.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 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..5ab32ab 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -576,6 +576,20 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
 	return retval ? retval : sizeof(s32);
 }
 
+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 int uio_find_mem_index(struct vm_area_struct *vma)
 {
 	struct uio_device *idev = vma->vm_private_data;
@@ -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.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237138

On Wed, Sep 30, 2015 at 03:28:56PM -0700, Stephen Hemminger wrote:
> This is a new UIO device driver to allow supporting MSI-X and MSI devices
> in userspace.  It has been used in environments like VMware and older versions
> of QEMU/KVM where no IOMMU support is available.
> 
> Stephen Hemminger (2):
> 
> *** BLURB HERE ***

I think this needs some more work to make sure
userspace can drive devices without a risk of
crashing the kernel even when there's no IOMMU.

I sent some comments in response to the patch itself.

Please copy me on future versions of this driver.

Thanks!

> Stephen Hemminger (2):
>   uio: add support for ioctls
>   uio: new driver to support PCI MSI-X
> 
>  drivers/uio/Kconfig          |   9 ++
>  drivers/uio/Makefile         |   1 +
>  drivers/uio/uio.c            |  15 ++
>  drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/uio_driver.h   |   3 +
>  include/uapi/linux/Kbuild    |   1 +
>  include/uapi/linux/uio_msi.h |  22 +++
>  7 files changed, 429 insertions(+)
>  create mode 100644 drivers/uio/uio_msi.c
>  create mode 100644 include/uapi/linux/uio_msi.h
> 
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237265 — Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver

On 10/01/2015 01:28 AM, Stephen Hemminger wrote:
> This is a new UIO device driver to allow supporting MSI-X and MSI devices
> in userspace.  It has been used in environments like VMware and older versions
> of QEMU/KVM where no IOMMU support is available.

Why not add msi/msix support to uio_pci_generic?

> Stephen Hemminger (2):
>
> *** BLURB HERE ***
>
> Stephen Hemminger (2):
>    uio: add support for ioctls
>    uio: new driver to support PCI MSI-X
>
>   drivers/uio/Kconfig          |   9 ++
>   drivers/uio/Makefile         |   1 +
>   drivers/uio/uio.c            |  15 ++
>   drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
>   include/linux/uio_driver.h   |   3 +
>   include/uapi/linux/Kbuild    |   1 +
>   include/uapi/linux/uio_msi.h |  22 +++
>   7 files changed, 429 insertions(+)
>   create mode 100644 drivers/uio/uio_msi.c
>   create mode 100644 include/uapi/linux/uio_msi.h
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237481 — Re: [dpdk-dev] [PATCH 0/2] uio_msi: device driver

On Thu, 1 Oct 2015 13:59:02 +0300
Avi Kivity <avi@scylladb.com> wrote:

> On 10/01/2015 01:28 AM, Stephen Hemminger wrote:
> > This is a new UIO device driver to allow supporting MSI-X and MSI devices
> > in userspace.  It has been used in environments like VMware and older versions
> > of QEMU/KVM where no IOMMU support is available.  
> 
> Why not add msi/msix support to uio_pci_generic?

That is possible but that would meet ABI and other resistance from the author.
Also, uio_pci_generic makes it harder to find resources since it doesn't fully
utilize UIO infrastructure.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web