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

Groups > linux.kernel > #1239211 > unrolled thread

[PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200
    [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 05:20 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 09:50 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 11:50 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 12:50 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 13:10 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 15:10 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 13:10 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 13:50 +0200
                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 13:50 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 14:00 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 10:30 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 11:50 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 12:30 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Stephen Hemminger <stephen@networkplumber.org> - 2015-10-05 10:50 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 11:10 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 12:10 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 22:20 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 11:20 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 21:20 +0200
    Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200
    Re: [PATCH v3 1/3] uio: add ioctl support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 05:10 +0200
      Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 09:40 +0200
        Re: [PATCH v3 1/3] uio: add ioctl support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 11:50 +0200
          Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 12:40 +0200
            Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 22:10 +0200
              Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 00:30 +0200
    Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 22:00 +0200

Page 1 of 2  [1] 2  Next page →

#1239211 — [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver

This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
 
Currently uio_pci_generic supports only legacy INT#x interrupts source. However
there are situations when this is not enough, for instance SR-IOV VF devices that
simply don't have INT#x capability. For such devices uio_pci_generic will simply
fail (more specifically probe() will fail).
 
When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to performance
overhead and thus VFIO is not an option users that develop user-space drivers are left
without any option but to develop some proprietary UIO drivers (e.g. igb_uio driver in Intel's
DPDK) just to be able to use UIO infrastructure.
 
This series provides a generic solution for this problem while preserving the original behaviour
for devices for which the original uio_pci_generic had worked before (i.e. INT#x will be used by default).

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

New in v2:
   - Added #include <linux/uaccess.h> to uio_pci_generic.c

Vlad Zolotarov (3):
  uio: add ioctl support
  uio_pci_generic: add MSI/MSI-X support
  Documentation: update uio-howto

 Documentation/DocBook/uio-howto.tmpl |  29 ++-
 drivers/uio/uio.c                    |  15 ++
 drivers/uio/uio_pci_generic.c        | 410 +++++++++++++++++++++++++++++++++--
 include/linux/uio_driver.h           |   3 +
 include/linux/uio_pci_generic.h      |  36 +++
 5 files changed, 467 insertions(+), 26 deletions(-)
 create mode 100644 include/linux/uio_pci_generic.h

-- 
2.1.0

--
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]

#1239213 — [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

Add support for MSI and MSI-X interrupt modes:
   - Interrupt mode selection order is:
        INT#X (for backward compatibility) -> MSI-X -> MSI.
   - Add ioctl() commands:
      - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode.
      - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs.
      - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio).
   - Add mappings to all bars (memory and portio): some devices have
     registers related to MSI/MSI-X handling outside BAR0.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v3:
   - Add __iomem qualifier to temp buffer receiving ioremap value.

New in v2:
   - Added #include <linux/uaccess.h> to uio_pci_generic.c

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/uio/uio_pci_generic.c   | 410 +++++++++++++++++++++++++++++++++++++---
 include/linux/uio_pci_generic.h |  36 ++++
 2 files changed, 423 insertions(+), 23 deletions(-)
 create mode 100644 include/linux/uio_pci_generic.h

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index d0b508b..6b8b1789 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -22,16 +22,32 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/msi.h>
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
+#include <linux/uio_pci_generic.h>
+#include <linux/eventfd.h>
+#include <linux/uaccess.h>
 
 #define DRIVER_VERSION	"0.01.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
 #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
 
+struct msix_info {
+	int num_irqs;
+	struct msix_entry *table;
+	struct uio_msix_irq_ctx {
+		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
+		char *name;			/* name in /proc/interrupts */
+	} *ctx;
+};
+
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
+	struct mutex msix_state_lock;		/* ioctl mutex */
+	enum uio_int_mode int_mode;
+	struct msix_info msix;
 };
 
 static inline struct uio_pci_generic_dev *
@@ -40,9 +56,177 @@ to_uio_pci_generic_dev(struct uio_info *info)
 	return container_of(info, struct uio_pci_generic_dev, info);
 }
 
-/* Interrupt handler. Read/modify/write the command register to disable
- * the interrupt. */
-static irqreturn_t irqhandler(int irq, struct uio_info *info)
+/* Unmap previously ioremap'd resources */
+static void release_iomaps(struct uio_pci_generic_dev *gdev)
+{
+	int i;
+	struct uio_mem *mem = gdev->info.mem;
+
+	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
+		if (mem->internal_addr) {
+			iounmap(mem->internal_addr);
+			mem->internal_addr = NULL;
+		}
+	}
+}
+
+static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
+{
+	int i, m = 0, p = 0, err;
+	static const char * const bar_names[] = {
+		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
+	};
+
+	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
+		unsigned long start = pci_resource_start(pdev, i);
+		unsigned long flags = pci_resource_flags(pdev, i);
+		unsigned long len = pci_resource_len(pdev, i);
+
+		if (start == 0 || len == 0)
+			continue;
+
+		if (flags & IORESOURCE_MEM) {
+			void __iomem *addr;
+
+			if (m >= MAX_UIO_MAPS)
+				continue;
+
+			addr = ioremap(start, len);
+			if (addr == NULL) {
+				err = -EINVAL;
+				goto fail;
+			}
+
+			info->mem[m].name = bar_names[i];
+			info->mem[m].addr = start;
+			info->mem[m].internal_addr = addr;
+			info->mem[m].size = len;
+			info->mem[m].memtype = UIO_MEM_PHYS;
+			++m;
+		} else if (flags & IORESOURCE_IO) {
+			if (p >= MAX_UIO_PORT_REGIONS)
+				continue;
+
+			info->port[p].name = bar_names[i];
+			info->port[p].start = start;
+			info->port[p].size = len;
+			info->port[p].porttype = UIO_PORT_X86;
+			++p;
+		}
+	}
+
+	return 0;
+fail:
+	for (i = 0; i < m; i++) {
+		iounmap(info->mem[i].internal_addr);
+		info->mem[i].internal_addr = NULL;
+	}
+
+	return err;
+}
+
+static irqreturn_t msix_irqhandler(int irq, void *arg);
+
+/* set the mapping between vector # and existing eventfd. */
+static int set_irq_eventfd(struct uio_pci_generic_dev *gdev, int vec, int fd)
+{
+	struct uio_msix_irq_ctx *ctx;
+	struct eventfd_ctx *trigger;
+	struct pci_dev *pdev = gdev->pdev;
+	int irq, err;
+
+	if (vec >= gdev->msix.num_irqs) {
+		dev_notice(&gdev->pdev->dev, "vec %u >= num_vec %u\n",
+			   vec, gdev->msix.num_irqs);
+		return -ERANGE;
+	}
+
+	irq = gdev->msix.table[vec].vector;
+
+	/* Cleanup existing irq mapping */
+	ctx = &gdev->msix.ctx[vec];
+	if (ctx->trigger) {
+		free_irq(irq, ctx->trigger);
+		eventfd_ctx_put(ctx->trigger);
+		ctx->trigger = NULL;
+	}
+
+	/* Passing -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(&gdev->pdev->dev,
+			   "eventfd ctx get failed: %d\n", err);
+		return err;
+	}
+
+	err = request_irq(irq, msix_irqhandler, 0, ctx->name, trigger);
+	if (err) {
+		dev_notice(&pdev->dev, "request irq failed: %d\n", err);
+		eventfd_ctx_put(trigger);
+		return err;
+	}
+
+	dev_dbg(&pdev->dev, "map vector %u to fd %d trigger %p\n",
+		vec, fd, trigger);
+	ctx->trigger = trigger;
+
+	return 0;
+}
+
+static int uio_pci_generic_ioctl(struct uio_info *info, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+	struct uio_pci_generic_irq_set hdr;
+	int err;
+
+	switch (cmd) {
+	case UIO_PCI_GENERIC_IRQ_SET:
+		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
+			return -EFAULT;
+
+		/* Locking is needed to ensure two things:
+		 *  1) Two IRQ_SET ioctl()'s are not running in parallel.
+		 *  2) IRQ_SET ioctl() is not running in parallel with remove().
+		 */
+		mutex_lock(&gdev->msix_state_lock);
+		if (gdev->int_mode != UIO_INT_MODE_MSIX) {
+			mutex_unlock(&gdev->msix_state_lock);
+			return -EOPNOTSUPP;
+		}
+
+		err = set_irq_eventfd(gdev, hdr.vec, hdr.fd);
+		mutex_unlock(&gdev->msix_state_lock);
+
+		break;
+	case UIO_PCI_GENERIC_IRQ_NUM_GET:
+		if (gdev->int_mode == UIO_INT_MODE_NONE)
+			err = put_user(0, (u32 __user *)arg);
+		else if (gdev->int_mode != UIO_INT_MODE_MSIX)
+			err = put_user(1, (u32 __user *)arg);
+		else
+			err = put_user(gdev->msix.num_irqs,
+				       (u32 __user *)arg);
+
+		break;
+	case UIO_PCI_GENERIC_INT_MODE_GET:
+		err = put_user(gdev->int_mode, (u32 __user *)arg);
+
+		break;
+	default:
+		err = -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
+/* INT#X interrupt handler. */
+static irqreturn_t intx_irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
 
@@ -53,8 +237,162 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 	return IRQ_HANDLED;
 }
 
-static int probe(struct pci_dev *pdev,
-			   const struct pci_device_id *id)
+/* MSI interrupt handler. */
+static irqreturn_t msi_irqhandler(int irq, struct uio_info *info)
+{
+	/* UIO core will signal the user process. */
+	return IRQ_HANDLED;
+}
+
+/* MSI-X interrupt handler. */
+static irqreturn_t msix_irqhandler(int irq, void *arg)
+{
+	struct eventfd_ctx *trigger = arg;
+
+	pr_devel("irq %u trigger %p\n", irq, trigger);
+
+	eventfd_signal(trigger, 1);
+	return IRQ_HANDLED;
+}
+
+static bool enable_intx(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+
+	if (!pdev->irq || !pci_intx_mask_supported(pdev))
+		return false;
+
+	gdev->int_mode = UIO_INT_MODE_INTX;
+	gdev->info.irq = pdev->irq;
+	gdev->info.irq_flags = IRQF_SHARED;
+	gdev->info.handler = intx_irqhandler;
+
+	return true;
+}
+
+static void set_pci_master(struct pci_dev *pdev)
+{
+	pci_set_master(pdev);
+	dev_warn(&pdev->dev, "Enabling PCI bus mastering. Bogus userspace application is able to trash kernel memory using DMA");
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+}
+
+static bool enable_msi(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+
+	set_pci_master(pdev);
+
+	if (pci_enable_msi(pdev))
+		return false;
+
+	gdev->int_mode = UIO_INT_MODE_MSI;
+	gdev->info.irq = pdev->irq;
+	gdev->info.irq_flags = 0;
+	gdev->info.handler = msi_irqhandler;
+
+	return true;
+}
+
+static bool enable_msix(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+	int i, vectors = pci_msix_vec_count(pdev);
+
+	if (vectors <= 0)
+		return false;
+
+	gdev->msix.table = kcalloc(vectors, sizeof(struct msix_entry),
+				   GFP_KERNEL);
+	if (!gdev->msix.table) {
+		dev_err(&pdev->dev, "Failed to allocate memory for MSI-X table");
+		return false;
+	}
+
+	gdev->msix.ctx = kcalloc(vectors, sizeof(struct uio_msix_irq_ctx),
+				 GFP_KERNEL);
+	if (!gdev->msix.ctx) {
+		dev_err(&pdev->dev, "Failed to allocate memory for MSI-X contexts");
+		goto err_ctx_alloc;
+	}
+
+	for (i = 0; i < vectors; i++) {
+		gdev->msix.table[i].entry = i;
+		gdev->msix.ctx[i].name = kasprintf(GFP_KERNEL,
+						   KBUILD_MODNAME "[%d](%s)",
+						   i, pci_name(pdev));
+		if (!gdev->msix.ctx[i].name)
+			goto err_name_alloc;
+	}
+
+	set_pci_master(pdev);
+
+	if (pci_enable_msix(pdev, gdev->msix.table, vectors))
+		goto err_msix_enable;
+
+	gdev->int_mode = UIO_INT_MODE_MSIX;
+	gdev->info.irq = UIO_IRQ_CUSTOM;
+	gdev->msix.num_irqs = vectors;
+
+	return true;
+
+err_msix_enable:
+	pci_clear_master(pdev);
+err_name_alloc:
+	for (i = 0; i < vectors; i++)
+		kfree(gdev->msix.ctx[i].name);
+
+	kfree(gdev->msix.ctx);
+err_ctx_alloc:
+	kfree(gdev->msix.table);
+
+	return false;
+}
+
+/**
+ * Disable interrupts and free related resources.
+ *
+ * @gdev device handle
+ *
+ * This function should be called after the corresponding UIO device has been
+ * unregistered. This will ensure that there are no currently running ioctl()s
+ * and there won't be any new ones until next probe() call.
+ */
+static void disable_intr(struct uio_pci_generic_dev *gdev)
+{
+	struct pci_dev *pdev = gdev->pdev;
+	int i;
+
+	switch (gdev->int_mode) {
+	case UIO_INT_MODE_MSI:
+		pci_disable_msi(pdev);
+		pci_clear_master(pdev);
+
+		break;
+	case UIO_INT_MODE_MSIX:
+		/* No need for locking here since there shouldn't be any
+		 * ioctl()s running by now.
+		 */
+		for (i = 0; i < gdev->msix.num_irqs; i++) {
+			if (gdev->msix.ctx[i].trigger)
+				set_irq_eventfd(gdev, i, -1);
+
+			kfree(gdev->msix.ctx[i].name);
+		}
+
+		pci_disable_msix(pdev);
+		pci_clear_master(pdev);
+		kfree(gdev->msix.ctx);
+		kfree(gdev->msix.table);
+
+		break;
+	default:
+		break;
+	}
+}
+
+
+static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
@@ -66,42 +404,64 @@ static int probe(struct pci_dev *pdev,
 		return err;
 	}
 
-	if (!pdev->irq) {
-		dev_warn(&pdev->dev, "No IRQ assigned to device: "
-			 "no support for interrupts?\n");
-		pci_disable_device(pdev);
-		return -ENODEV;
-	}
-
-	if (!pci_intx_mask_supported(pdev)) {
-		err = -ENODEV;
-		goto err_verify;
-	}
-
 	gdev = kzalloc(sizeof(struct uio_pci_generic_dev), GFP_KERNEL);
 	if (!gdev) {
 		err = -ENOMEM;
 		goto err_alloc;
 	}
 
+	gdev->pdev = pdev;
 	gdev->info.name = "uio_pci_generic";
 	gdev->info.version = DRIVER_VERSION;
-	gdev->info.irq = pdev->irq;
-	gdev->info.irq_flags = IRQF_SHARED;
-	gdev->info.handler = irqhandler;
-	gdev->pdev = pdev;
+	gdev->info.ioctl = uio_pci_generic_ioctl;
+	mutex_init(&gdev->msix_state_lock);
+
+	err = pci_request_regions(pdev, "uio_pci_generic");
+	if (err != 0) {
+		dev_err(&pdev->dev, "Cannot request regions\n");
+		goto err_request_regions;
+	}
+
+	/* Enable the corresponding interrupt mode. Try to enable INT#X first
+	 * for backward compatibility.
+	 */
+	if (enable_intx(gdev))
+		dev_info(&pdev->dev, "Using INT#x mode: IRQ %ld",
+			 gdev->info.irq);
+	else if (enable_msix(gdev))
+		dev_info(&pdev->dev, "Using MSI-X mode: number of IRQs %d",
+			 gdev->msix.num_irqs);
+	else if (enable_msi(gdev))
+		dev_info(&pdev->dev, "Using MSI mode: IRQ %ld", gdev->info.irq);
+	else {
+		err = -ENODEV;
+		goto err_verify;
+	}
+
+	/* remap resources */
+	err = setup_maps(pdev, &gdev->info);
+	if (err)
+		goto err_maps;
 
 	err = uio_register_device(&pdev->dev, &gdev->info);
 	if (err)
 		goto err_register;
+
 	pci_set_drvdata(pdev, gdev);
 
 	return 0;
+
 err_register:
+	release_iomaps(gdev);
+err_maps:
+	disable_intr(gdev);
+err_verify:
+	pci_release_regions(pdev);
+err_request_regions:
 	kfree(gdev);
 err_alloc:
-err_verify:
 	pci_disable_device(pdev);
+
 	return err;
 }
 
@@ -110,8 +470,12 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
-	pci_disable_device(pdev);
+	disable_intr(gdev);
+	release_iomaps(gdev);
+	pci_release_regions(pdev);
 	kfree(gdev);
+	pci_disable_device(pdev);
+	pci_set_drvdata(pdev, NULL);
 }
 
 static struct pci_driver uio_pci_driver = {
diff --git a/include/linux/uio_pci_generic.h b/include/linux/uio_pci_generic.h
new file mode 100644
index 0000000..10716fc
--- /dev/null
+++ b/include/linux/uio_pci_generic.h
@@ -0,0 +1,36 @@
+/*
+ * include/linux/uio_pci_generic.h
+ *
+ * Userspace generic PCI IO driver.
+ *
+ * Licensed under the GPLv2 only.
+ */
+
+#ifndef _UIO_PCI_GENERIC_H_
+#define _UIO_PCI_GENERIC_H_
+
+#include <linux/ioctl.h>
+
+enum uio_int_mode {
+	UIO_INT_MODE_NONE,
+	UIO_INT_MODE_INTX,
+	UIO_INT_MODE_MSI,
+	UIO_INT_MODE_MSIX
+};
+
+/* bind the requested IRQ to the given eventfd */
+struct uio_pci_generic_irq_set {
+	int vec; /* index of the IRQ to connect to starting from 0 */
+	int fd;
+};
+
+#define UIO_PCI_GENERIC_BASE		0x86
+
+#define UIO_PCI_GENERIC_IRQ_SET		_IOW('I', UIO_PCI_GENERIC_BASE + 1, \
+		struct uio_pci_generic_irq_set)
+#define UIO_PCI_GENERIC_IRQ_NUM_GET	_IOW('I', UIO_PCI_GENERIC_BASE + 2, \
+		uint32_t)
+#define UIO_PCI_GENERIC_INT_MODE_GET	_IOW('I', UIO_PCI_GENERIC_BASE + 3, \
+		uint32_t)
+
+#endif /* _UIO_PCI_GENERIC_H_ */
-- 
2.1.0

--
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]

#1239267 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Sun, Oct 04, 2015 at 11:43:17PM +0300, Vlad Zolotarov wrote:
> Add support for MSI and MSI-X interrupt modes:
>    - Interrupt mode selection order is:
>         INT#X (for backward compatibility) -> MSI-X -> MSI.
>    - Add ioctl() commands:
>       - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode.
>       - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs.
>       - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio).
>    - Add mappings to all bars (memory and portio): some devices have
>      registers related to MSI/MSI-X handling outside BAR0.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v3:
>    - Add __iomem qualifier to temp buffer receiving ioremap value.
> 
> New in v2:
>    - Added #include <linux/uaccess.h> to uio_pci_generic.c
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  drivers/uio/uio_pci_generic.c   | 410 +++++++++++++++++++++++++++++++++++++---
>  include/linux/uio_pci_generic.h |  36 ++++
>  2 files changed, 423 insertions(+), 23 deletions(-)
>  create mode 100644 include/linux/uio_pci_generic.h
> 
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index d0b508b..6b8b1789 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -22,16 +22,32 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/msi.h>
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
> +#include <linux/uio_pci_generic.h>
> +#include <linux/eventfd.h>
> +#include <linux/uaccess.h>
>  
>  #define DRIVER_VERSION	"0.01.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>  #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
>  
> +struct msix_info {
> +	int num_irqs;
> +	struct msix_entry *table;
> +	struct uio_msix_irq_ctx {
> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */

Why are you using eventfd for msi vectors?  What's the reason for
needing this?

You haven't documented how this api works at all, you are going to have
to a lot more work to justify this, as this greatly increases the
complexity of the user/kernel api in unknown ways.

greg k-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]

#1239325 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 06:11, Greg KH wrote:
> On Sun, Oct 04, 2015 at 11:43:17PM +0300, Vlad Zolotarov wrote:
>> Add support for MSI and MSI-X interrupt modes:
>>     - Interrupt mode selection order is:
>>          INT#X (for backward compatibility) -> MSI-X -> MSI.
>>     - Add ioctl() commands:
>>        - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode.
>>        - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs.
>>        - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio).
>>     - Add mappings to all bars (memory and portio): some devices have
>>       registers related to MSI/MSI-X handling outside BAR0.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v3:
>>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>>
>> New in v2:
>>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/uio/uio_pci_generic.c   | 410 +++++++++++++++++++++++++++++++++++++---
>>   include/linux/uio_pci_generic.h |  36 ++++
>>   2 files changed, 423 insertions(+), 23 deletions(-)
>>   create mode 100644 include/linux/uio_pci_generic.h
>>
>> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
>> index d0b508b..6b8b1789 100644
>> --- a/drivers/uio/uio_pci_generic.c
>> +++ b/drivers/uio/uio_pci_generic.c
>> @@ -22,16 +22,32 @@
>>   #include <linux/device.h>
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>> +#include <linux/msi.h>
>>   #include <linux/slab.h>
>>   #include <linux/uio_driver.h>
>> +#include <linux/uio_pci_generic.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/uaccess.h>
>>   
>>   #define DRIVER_VERSION	"0.01.0"
>>   #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>>   #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
>>   
>> +struct msix_info {
>> +	int num_irqs;
>> +	struct msix_entry *table;
>> +	struct uio_msix_irq_ctx {
>> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
> Why are you using eventfd for msi vectors?  What's the reason for
> needing this?

A small correction - for MSI-X vectors. There may be only one MSI vector 
per PCI function and if it's used it would use the same interface as a 
legacy INT#x interrupt uses at the moment.
So, for MSI-X case the reason is that there may be (in most cases there 
will be) more than one interrupt vector. Thus, as I've explained in a 
PATCH1 thread we need a way to indicated each of them separately. 
eventfd seems like a good way of doing so. If u have better ideas, pls., 
share.

>
> You haven't documented how this api works at all, you are going to have
> to a lot more work to justify this, as this greatly increases the
> complexity of the user/kernel api in unknown ways.

I actually do documented it a bit. Pls., check PATCH3 out. I admit that 
I could do a better job by for instance providing a code example. I'll 
improve this in v4 once we agree on all other details.

thanks,
vlad

>
> greg k-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]

#1239389 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
> >>+struct msix_info {
> >>+	int num_irqs;
> >>+	struct msix_entry *table;
> >>+	struct uio_msix_irq_ctx {
> >>+		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
> >Why are you using eventfd for msi vectors?  What's the reason for
> >needing this?
> 
> A small correction - for MSI-X vectors. There may be only one MSI vector per
> PCI function and if it's used it would use the same interface as a legacy
> INT#x interrupt uses at the moment.
> So, for MSI-X case the reason is that there may be (in most cases there will
> be) more than one interrupt vector. Thus, as I've explained in a PATCH1
> thread we need a way to indicated each of them separately. eventfd seems
> like a good way of doing so. If u have better ideas, pls., share.

You need to document what you are doing here, I don't see any
explaination for using eventfd at all.

And no, I don't know of any other solution as I don't know what you are
trying to do here (hint, the changelog didn't document it...)

> >You haven't documented how this api works at all, you are going to have
> >to a lot more work to justify this, as this greatly increases the
> >complexity of the user/kernel api in unknown ways.
> 
> I actually do documented it a bit. Pls., check PATCH3 out.

That provided no information at all about how to use the api.

If it did, you would see that your api is broken for 32/64bit kernels
and will fall over into nasty pieces the first time you try to use it
there, which means it hasn't been tested at all :(

thanks,

greg k-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]

#1239440 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 10:56, Greg KH wrote:
> On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
>>>> +struct msix_info {
>>>> +	int num_irqs;
>>>> +	struct msix_entry *table;
>>>> +	struct uio_msix_irq_ctx {
>>>> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
>>> Why are you using eventfd for msi vectors?  What's the reason for
>>> needing this?
>> A small correction - for MSI-X vectors. There may be only one MSI vector per
>> PCI function and if it's used it would use the same interface as a legacy
>> INT#x interrupt uses at the moment.
>> So, for MSI-X case the reason is that there may be (in most cases there will
>> be) more than one interrupt vector. Thus, as I've explained in a PATCH1
>> thread we need a way to indicated each of them separately. eventfd seems
>> like a good way of doing so. If u have better ideas, pls., share.
> You need to document what you are doing here, I don't see any
> explaination for using eventfd at all.
>
> And no, I don't know of any other solution as I don't know what you are
> trying to do here (hint, the changelog didn't document it...)
>
>>> You haven't documented how this api works at all, you are going to have
>>> to a lot more work to justify this, as this greatly increases the
>>> complexity of the user/kernel api in unknown ways.
>> I actually do documented it a bit. Pls., check PATCH3 out.
> That provided no information at all about how to use the api.
>
> If it did, you would see that your api is broken for 32/64bit kernels
> and will fall over into nasty pieces the first time you try to use it
> there, which means it hasn't been tested at all :(

It has been tested of course ;)
I tested it only in 64 bit environment however where both kernel and 
user space applications were compiled on the same machine with the same 
compiler and it could be that "int" had the same number of bytes both in 
kernel and in user space application. Therefore it worked perfectly - I 
patched DPDK to use the new uio_pci_generic MSI-X API to test this and I 
have verified that all 3 interrupt modes work: MSI-X with SR-IOV VF 
device in Amazon EC2 guest and INT#x and MSI with a PF device on bare 
metal server.

However I agree using uint32_t for "vec" and "fd" would be much more 
correct.

>
> thanks,
>
> greg k-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]

#1239460 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/2015 01:57 PM, Greg KH wrote:
> On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
>>
>> On 10/05/15 10:56, Greg KH wrote:
>>> On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
>>>>>> +struct msix_info {
>>>>>> +	int num_irqs;
>>>>>> +	struct msix_entry *table;
>>>>>> +	struct uio_msix_irq_ctx {
>>>>>> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
>>>>> Why are you using eventfd for msi vectors?  What's the reason for
>>>>> needing this?
>>>> A small correction - for MSI-X vectors. There may be only one MSI vector per
>>>> PCI function and if it's used it would use the same interface as a legacy
>>>> INT#x interrupt uses at the moment.
>>>> So, for MSI-X case the reason is that there may be (in most cases there will
>>>> be) more than one interrupt vector. Thus, as I've explained in a PATCH1
>>>> thread we need a way to indicated each of them separately. eventfd seems
>>>> like a good way of doing so. If u have better ideas, pls., share.
>>> You need to document what you are doing here, I don't see any
>>> explaination for using eventfd at all.
>>>
>>> And no, I don't know of any other solution as I don't know what you are
>>> trying to do here (hint, the changelog didn't document it...)
>>>
>>>>> You haven't documented how this api works at all, you are going to have
>>>>> to a lot more work to justify this, as this greatly increases the
>>>>> complexity of the user/kernel api in unknown ways.
>>>> I actually do documented it a bit. Pls., check PATCH3 out.
>>> That provided no information at all about how to use the api.
>>>
>>> If it did, you would see that your api is broken for 32/64bit kernels
>>> and will fall over into nasty pieces the first time you try to use it
>>> there, which means it hasn't been tested at all :(
>> It has been tested of course ;)
>> I tested it only in 64 bit environment however where both kernel and user
>> space applications were compiled on the same machine with the same compiler
>> and it could be that "int" had the same number of bytes both in kernel and
>> in user space application. Therefore it worked perfectly - I patched DPDK to
>> use the new uio_pci_generic MSI-X API to test this and I have verified that
>> all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon EC2 guest
>> and INT#x and MSI with a PF device on bare metal server.
>>
>> However I agree using uint32_t for "vec" and "fd" would be much more
>> correct.
> I don't think file descriptors are __u32 on a 64bit arch, are they?
>
> And NEVER use the _t types in kernel code, the namespaces is all wrong
> and it is not applicable for us, sorry.

Wasn't the real reason that they aren't defined (or reserved) by C89, 
and therefore could clash with a user identifier, rather than some 
inherent wrongness?

--
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]

#1239526 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 02:09:32PM +0300, Avi Kivity wrote:
> On 10/05/2015 01:57 PM, Greg KH wrote:
> >On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
> >>
> >>On 10/05/15 10:56, Greg KH wrote:
> >>>On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
> >>>>>>+struct msix_info {
> >>>>>>+	int num_irqs;
> >>>>>>+	struct msix_entry *table;
> >>>>>>+	struct uio_msix_irq_ctx {
> >>>>>>+		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
> >>>>>Why are you using eventfd for msi vectors?  What's the reason for
> >>>>>needing this?
> >>>>A small correction - for MSI-X vectors. There may be only one MSI vector per
> >>>>PCI function and if it's used it would use the same interface as a legacy
> >>>>INT#x interrupt uses at the moment.
> >>>>So, for MSI-X case the reason is that there may be (in most cases there will
> >>>>be) more than one interrupt vector. Thus, as I've explained in a PATCH1
> >>>>thread we need a way to indicated each of them separately. eventfd seems
> >>>>like a good way of doing so. If u have better ideas, pls., share.
> >>>You need to document what you are doing here, I don't see any
> >>>explaination for using eventfd at all.
> >>>
> >>>And no, I don't know of any other solution as I don't know what you are
> >>>trying to do here (hint, the changelog didn't document it...)
> >>>
> >>>>>You haven't documented how this api works at all, you are going to have
> >>>>>to a lot more work to justify this, as this greatly increases the
> >>>>>complexity of the user/kernel api in unknown ways.
> >>>>I actually do documented it a bit. Pls., check PATCH3 out.
> >>>That provided no information at all about how to use the api.
> >>>
> >>>If it did, you would see that your api is broken for 32/64bit kernels
> >>>and will fall over into nasty pieces the first time you try to use it
> >>>there, which means it hasn't been tested at all :(
> >>It has been tested of course ;)
> >>I tested it only in 64 bit environment however where both kernel and user
> >>space applications were compiled on the same machine with the same compiler
> >>and it could be that "int" had the same number of bytes both in kernel and
> >>in user space application. Therefore it worked perfectly - I patched DPDK to
> >>use the new uio_pci_generic MSI-X API to test this and I have verified that
> >>all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon EC2 guest
> >>and INT#x and MSI with a PF device on bare metal server.
> >>
> >>However I agree using uint32_t for "vec" and "fd" would be much more
> >>correct.
> >I don't think file descriptors are __u32 on a 64bit arch, are they?
> >
> >And NEVER use the _t types in kernel code, the namespaces is all wrong
> >and it is not applicable for us, sorry.
> 
> Wasn't the real reason that they aren't defined (or reserved) by C89, and
> therefore could clash with a user identifier, rather than some inherent
> wrongness?

Kind of, my memory is vague.  There's a great rant from Linus about why
they don't work in the kernel somewhere in the lkml archives...
--
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]

#1239465 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
> 
> 
> On 10/05/15 10:56, Greg KH wrote:
> >On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
> >>>>+struct msix_info {
> >>>>+	int num_irqs;
> >>>>+	struct msix_entry *table;
> >>>>+	struct uio_msix_irq_ctx {
> >>>>+		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
> >>>Why are you using eventfd for msi vectors?  What's the reason for
> >>>needing this?
> >>A small correction - for MSI-X vectors. There may be only one MSI vector per
> >>PCI function and if it's used it would use the same interface as a legacy
> >>INT#x interrupt uses at the moment.
> >>So, for MSI-X case the reason is that there may be (in most cases there will
> >>be) more than one interrupt vector. Thus, as I've explained in a PATCH1
> >>thread we need a way to indicated each of them separately. eventfd seems
> >>like a good way of doing so. If u have better ideas, pls., share.
> >You need to document what you are doing here, I don't see any
> >explaination for using eventfd at all.
> >
> >And no, I don't know of any other solution as I don't know what you are
> >trying to do here (hint, the changelog didn't document it...)
> >
> >>>You haven't documented how this api works at all, you are going to have
> >>>to a lot more work to justify this, as this greatly increases the
> >>>complexity of the user/kernel api in unknown ways.
> >>I actually do documented it a bit. Pls., check PATCH3 out.
> >That provided no information at all about how to use the api.
> >
> >If it did, you would see that your api is broken for 32/64bit kernels
> >and will fall over into nasty pieces the first time you try to use it
> >there, which means it hasn't been tested at all :(
> 
> It has been tested of course ;)
> I tested it only in 64 bit environment however where both kernel and user
> space applications were compiled on the same machine with the same compiler
> and it could be that "int" had the same number of bytes both in kernel and
> in user space application. Therefore it worked perfectly - I patched DPDK to
> use the new uio_pci_generic MSI-X API to test this and I have verified that
> all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon EC2 guest
> and INT#x and MSI with a PF device on bare metal server.
> 
> However I agree using uint32_t for "vec" and "fd" would be much more
> correct.

I don't think file descriptors are __u32 on a 64bit arch, are they?

And NEVER use the _t types in kernel code, the namespaces is all wrong
and it is not applicable for us, sorry.

thanks,

greg k-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]

#1239485 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 13:57, Greg KH wrote:
> On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
>>
>> On 10/05/15 10:56, Greg KH wrote:
>>> On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
>>>>>> +struct msix_info {
>>>>>> +	int num_irqs;
>>>>>> +	struct msix_entry *table;
>>>>>> +	struct uio_msix_irq_ctx {
>>>>>> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
>>>>> Why are you using eventfd for msi vectors?  What's the reason for
>>>>> needing this?
>>>> A small correction - for MSI-X vectors. There may be only one MSI vector per
>>>> PCI function and if it's used it would use the same interface as a legacy
>>>> INT#x interrupt uses at the moment.
>>>> So, for MSI-X case the reason is that there may be (in most cases there will
>>>> be) more than one interrupt vector. Thus, as I've explained in a PATCH1
>>>> thread we need a way to indicated each of them separately. eventfd seems
>>>> like a good way of doing so. If u have better ideas, pls., share.
>>> You need to document what you are doing here, I don't see any
>>> explaination for using eventfd at all.
>>>
>>> And no, I don't know of any other solution as I don't know what you are
>>> trying to do here (hint, the changelog didn't document it...)
>>>
>>>>> You haven't documented how this api works at all, you are going to have
>>>>> to a lot more work to justify this, as this greatly increases the
>>>>> complexity of the user/kernel api in unknown ways.
>>>> I actually do documented it a bit. Pls., check PATCH3 out.
>>> That provided no information at all about how to use the api.
>>>
>>> If it did, you would see that your api is broken for 32/64bit kernels
>>> and will fall over into nasty pieces the first time you try to use it
>>> there, which means it hasn't been tested at all :(
>> It has been tested of course ;)
>> I tested it only in 64 bit environment however where both kernel and user
>> space applications were compiled on the same machine with the same compiler
>> and it could be that "int" had the same number of bytes both in kernel and
>> in user space application. Therefore it worked perfectly - I patched DPDK to
>> use the new uio_pci_generic MSI-X API to test this and I have verified that
>> all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon EC2 guest
>> and INT#x and MSI with a PF device on bare metal server.
>>
>> However I agree using uint32_t for "vec" and "fd" would be much more
>> correct.
> I don't think file descriptors are __u32 on a 64bit arch, are they?

I think they are "int" on all platforms and as far as I know u32 should 
be enough to contain int on any platform.

>
> And NEVER use the _t types in kernel code,

Never meant it - it was for a user space interface. For a kernel it's 
u32 of course.

>   the namespaces is all wrong
> and it is not applicable for us, sorry.
>
> thanks,
>
> greg k-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]

#1239486 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/2015 02:41 PM, Vlad Zolotarov wrote:
>
>
> On 10/05/15 13:57, Greg KH wrote:
>> On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
>>>
>>> On 10/05/15 10:56, Greg KH wrote:
>>>> On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
>>>>>>> +struct msix_info {
>>>>>>> +    int num_irqs;
>>>>>>> +    struct msix_entry *table;
>>>>>>> +    struct uio_msix_irq_ctx {
>>>>>>> +        struct eventfd_ctx *trigger;    /* MSI-x vector to 
>>>>>>> eventfd */
>>>>>> Why are you using eventfd for msi vectors?  What's the reason for
>>>>>> needing this?
>>>>> A small correction - for MSI-X vectors. There may be only one MSI 
>>>>> vector per
>>>>> PCI function and if it's used it would use the same interface as a 
>>>>> legacy
>>>>> INT#x interrupt uses at the moment.
>>>>> So, for MSI-X case the reason is that there may be (in most cases 
>>>>> there will
>>>>> be) more than one interrupt vector. Thus, as I've explained in a 
>>>>> PATCH1
>>>>> thread we need a way to indicated each of them separately. eventfd 
>>>>> seems
>>>>> like a good way of doing so. If u have better ideas, pls., share.
>>>> You need to document what you are doing here, I don't see any
>>>> explaination for using eventfd at all.
>>>>
>>>> And no, I don't know of any other solution as I don't know what you 
>>>> are
>>>> trying to do here (hint, the changelog didn't document it...)
>>>>
>>>>>> You haven't documented how this api works at all, you are going 
>>>>>> to have
>>>>>> to a lot more work to justify this, as this greatly increases the
>>>>>> complexity of the user/kernel api in unknown ways.
>>>>> I actually do documented it a bit. Pls., check PATCH3 out.
>>>> That provided no information at all about how to use the api.
>>>>
>>>> If it did, you would see that your api is broken for 32/64bit kernels
>>>> and will fall over into nasty pieces the first time you try to use it
>>>> there, which means it hasn't been tested at all :(
>>> It has been tested of course ;)
>>> I tested it only in 64 bit environment however where both kernel and 
>>> user
>>> space applications were compiled on the same machine with the same 
>>> compiler
>>> and it could be that "int" had the same number of bytes both in 
>>> kernel and
>>> in user space application. Therefore it worked perfectly - I patched 
>>> DPDK to
>>> use the new uio_pci_generic MSI-X API to test this and I have 
>>> verified that
>>> all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon 
>>> EC2 guest
>>> and INT#x and MSI with a PF device on bare metal server.
>>>
>>> However I agree using uint32_t for "vec" and "fd" would be much more
>>> correct.
>> I don't think file descriptors are __u32 on a 64bit arch, are they?
>
> I think they are "int" on all platforms and as far as I know u32 
> should be enough to contain int on any platform.
>

You need to make sure structures have the same layout on both 32-bit and 
64-bit systems, or you'll have to code compat ioctl translations for 
them.  The best way to do that is to use __u32 so the sizes are obvious, 
even for int, and to pad everything to 64 bit:

> +struct msix_info { 

+    __u32 num_irqs;
+    __u32 pad; // so pointer below is aligned to 64-bit on both 32-bit 
and 64-bit userspace
>
> +    struct msix_entry *table;
> +    struct uio_msix_irq_ctx {
> +        struct eventfd_ctx *trigger;    /* MSI-x vector to eventfd */


>>
>> And NEVER use the _t types in kernel code,
>
> Never meant it - it was for a user space interface. For a kernel it's 
> u32 of course.
>

For interfaces, use __u32.  You can't use uint32_t because if someone 
uses C89 in 2015, they may not have <cstdint.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]

#1239488 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 14:47, Avi Kivity wrote:
> On 10/05/2015 02:41 PM, Vlad Zolotarov wrote:
>>
>>
>> On 10/05/15 13:57, Greg KH wrote:
>>> On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
>>>>
>>>> On 10/05/15 10:56, Greg KH wrote:
>>>>> On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
>>>>>>>> +struct msix_info {
>>>>>>>> +    int num_irqs;
>>>>>>>> +    struct msix_entry *table;
>>>>>>>> +    struct uio_msix_irq_ctx {
>>>>>>>> +        struct eventfd_ctx *trigger;    /* MSI-x vector to 
>>>>>>>> eventfd */
>>>>>>> Why are you using eventfd for msi vectors?  What's the reason for
>>>>>>> needing this?
>>>>>> A small correction - for MSI-X vectors. There may be only one MSI 
>>>>>> vector per
>>>>>> PCI function and if it's used it would use the same interface as 
>>>>>> a legacy
>>>>>> INT#x interrupt uses at the moment.
>>>>>> So, for MSI-X case the reason is that there may be (in most cases 
>>>>>> there will
>>>>>> be) more than one interrupt vector. Thus, as I've explained in a 
>>>>>> PATCH1
>>>>>> thread we need a way to indicated each of them separately. 
>>>>>> eventfd seems
>>>>>> like a good way of doing so. If u have better ideas, pls., share.
>>>>> You need to document what you are doing here, I don't see any
>>>>> explaination for using eventfd at all.
>>>>>
>>>>> And no, I don't know of any other solution as I don't know what 
>>>>> you are
>>>>> trying to do here (hint, the changelog didn't document it...)
>>>>>
>>>>>>> You haven't documented how this api works at all, you are going 
>>>>>>> to have
>>>>>>> to a lot more work to justify this, as this greatly increases the
>>>>>>> complexity of the user/kernel api in unknown ways.
>>>>>> I actually do documented it a bit. Pls., check PATCH3 out.
>>>>> That provided no information at all about how to use the api.
>>>>>
>>>>> If it did, you would see that your api is broken for 32/64bit kernels
>>>>> and will fall over into nasty pieces the first time you try to use it
>>>>> there, which means it hasn't been tested at all :(
>>>> It has been tested of course ;)
>>>> I tested it only in 64 bit environment however where both kernel 
>>>> and user
>>>> space applications were compiled on the same machine with the same 
>>>> compiler
>>>> and it could be that "int" had the same number of bytes both in 
>>>> kernel and
>>>> in user space application. Therefore it worked perfectly - I 
>>>> patched DPDK to
>>>> use the new uio_pci_generic MSI-X API to test this and I have 
>>>> verified that
>>>> all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon 
>>>> EC2 guest
>>>> and INT#x and MSI with a PF device on bare metal server.
>>>>
>>>> However I agree using uint32_t for "vec" and "fd" would be much more
>>>> correct.
>>> I don't think file descriptors are __u32 on a 64bit arch, are they?
>>
>> I think they are "int" on all platforms and as far as I know u32 
>> should be enough to contain int on any platform.
>>
>
> You need to make sure structures have the same layout on both 32-bit 
> and 64-bit systems, or you'll have to code compat ioctl translations 
> for them.  The best way to do that is to use __u32 so the sizes are 
> obvious, even for int, and to pad everything to 64 bit:

Sure, but the structure below is not the one that is passed in ioctl() - 
it's an internal uio_pci_generic state and there is nothing to worry about.
The one in question is struct uio_pci_generic_irq_set from 
uio_pci_generic.h:

struct uio_pci_generic_irq_set {
     int vec; /* index of the IRQ to connect to starting from 0 */
     int fd;
};

It should be
struct uio_pci_generic_irq_set {
     __u32 vec; /* index of the IRQ to connect to starting from 0 */
     __u32 fd;
};

instead.


>
>> +struct msix_info { 
>
> +    __u32 num_irqs;
> +    __u32 pad; // so pointer below is aligned to 64-bit on both 
> 32-bit and 64-bit userspace
>>
>> +    struct msix_entry *table;
>> +    struct uio_msix_irq_ctx {
>> +        struct eventfd_ctx *trigger;    /* MSI-x vector to eventfd */
>
>
>>>
>>> And NEVER use the _t types in kernel code,
>>
>> Never meant it - it was for a user space interface. For a kernel it's 
>> u32 of course.
>>
>
> For interfaces, use __u32.  You can't use uint32_t because if someone 
> uses C89 in 2015, they may not have <cstdint.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]

#1239351 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/2015 06:11 AM, Greg KH wrote:
> On Sun, Oct 04, 2015 at 11:43:17PM +0300, Vlad Zolotarov wrote:
>> Add support for MSI and MSI-X interrupt modes:
>>     - Interrupt mode selection order is:
>>          INT#X (for backward compatibility) -> MSI-X -> MSI.
>>     - Add ioctl() commands:
>>        - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode.
>>        - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs.
>>        - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio).
>>     - Add mappings to all bars (memory and portio): some devices have
>>       registers related to MSI/MSI-X handling outside BAR0.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v3:
>>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>>
>> New in v2:
>>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/uio/uio_pci_generic.c   | 410 +++++++++++++++++++++++++++++++++++++---
>>   include/linux/uio_pci_generic.h |  36 ++++
>>   2 files changed, 423 insertions(+), 23 deletions(-)
>>   create mode 100644 include/linux/uio_pci_generic.h
>>
>> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
>> index d0b508b..6b8b1789 100644
>> --- a/drivers/uio/uio_pci_generic.c
>> +++ b/drivers/uio/uio_pci_generic.c
>> @@ -22,16 +22,32 @@
>>   #include <linux/device.h>
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>> +#include <linux/msi.h>
>>   #include <linux/slab.h>
>>   #include <linux/uio_driver.h>
>> +#include <linux/uio_pci_generic.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/uaccess.h>
>>   
>>   #define DRIVER_VERSION	"0.01.0"
>>   #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>>   #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
>>   
>> +struct msix_info {
>> +	int num_irqs;
>> +	struct msix_entry *table;
>> +	struct uio_msix_irq_ctx {
>> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
> Why are you using eventfd for msi vectors?  What's the reason for
> needing this?
>
> You haven't documented how this api works at all, you are going to have
> to a lot more work to justify this, as this greatly increases the
> complexity of the user/kernel api in unknown ways.
>
>

Of course it has to be documented, but this just follows vfio.

Eventfd is a natural enough representation of an interrupt; both kvm and 
vfio use it, and are also able to share the eventfd, allowing a vfio 
interrupt to generate a kvm interrupt, without userspace intervention, 
and one day without even kernel intervention.

--
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]

#1239391 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 11:28:03AM +0300, Avi Kivity wrote:
> Of course it has to be documented, but this just follows vfio.
> 
> Eventfd is a natural enough representation of an interrupt; both kvm and
> vfio use it, and are also able to share the eventfd, allowing a vfio
> interrupt to generate a kvm interrupt, without userspace intervention, and
> one day without even kernel intervention.

That's nice and wonderful, but it's not how UIO works today, so this is
now going to be a mix and match type interface, with no justification so
far as to why to create this new api and exactly how this is all going
to be used from userspace.

Example code would be even better...

thanks,

greg k-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]

#1239424 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/2015 12:49 PM, Greg KH wrote:
> On Mon, Oct 05, 2015 at 11:28:03AM +0300, Avi Kivity wrote:
>> Of course it has to be documented, but this just follows vfio.
>>
>> Eventfd is a natural enough representation of an interrupt; both kvm and
>> vfio use it, and are also able to share the eventfd, allowing a vfio
>> interrupt to generate a kvm interrupt, without userspace intervention, and
>> one day without even kernel intervention.
> That's nice and wonderful, but it's not how UIO works today, so this is
> now going to be a mix and match type interface, with no justification so
> far as to why to create this new api and exactly how this is all going
> to be used from userspace.

The intended user is dpdk (http://dpdk.org), which is a family of 
userspace networking drivers for high performance networking applications.

The natural device driver for dpdk is vfio, which both provides memory 
protection and exposes msi/msix interrupts.  However, in many cases vfio 
cannot be used, either due to the lack of an iommu (for example, in 
virtualized environments) or out of a desire to avoid the iommus 
performance impact.

The challenge in exposing msix interrupts to user space is that there 
are many of them, so you can't simply poll the device fd.  If you do, 
how do you know which interrupt was triggered?  The solution that vfio 
adopted was to associate each interrupt with an eventfd, allowing it to 
be individually polled.  Since you can pass an eventfd with SCM_RIGHTS, 
and since kvm can trigger guest interrupts using an eventfd, the 
solution is very flexible.

> Example code would be even better...
>
>


This is the vfio dpdk interface code:

http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c

basically, the equivalent uio msix code would be very similar if uio 
adopts a similar interface:

http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/eal_pci_uio.c

(current code lacks msi/msix support, of course).
--
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]

#1239361 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Sun,  4 Oct 2015 23:43:17 +0300
Vlad Zolotarov <vladz@cloudius-systems.com> wrote:

> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
> +{
> +	int i, m = 0, p = 0, err;
> +	static const char * const bar_names[] = {
> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
> +		unsigned long start = pci_resource_start(pdev, i);
> +		unsigned long flags = pci_resource_flags(pdev, i);
> +		unsigned long len = pci_resource_len(pdev, i);
> +
> +		if (start == 0 || len == 0)
> +			continue;
> +
> +		if (flags & IORESOURCE_MEM) {
> +			void __iomem *addr;
> +
> +			if (m >= MAX_UIO_MAPS)
> +				continue;
> +
> +			addr = ioremap(start, len);
> +			if (addr == NULL) {
> +				err = -EINVAL;
> +				goto fail;
> +			}
> +
> +			info->mem[m].name = bar_names[i];
> +			info->mem[m].addr = start;
> +			info->mem[m].internal_addr = addr;
> +			info->mem[m].size = len;
> +			info->mem[m].memtype = UIO_MEM_PHYS;
> +			++m;
> +		} else if (flags & IORESOURCE_IO) {
> +			if (p >= MAX_UIO_PORT_REGIONS)
> +				continue;
> +
> +			info->port[p].name = bar_names[i];
> +			info->port[p].start = start;
> +			info->port[p].size = len;
> +			info->port[p].porttype = UIO_PORT_X86;
> +			++p;
> +		}
> +	}
> +
> +	return 0;
> +fail:
> +	for (i = 0; i < m; i++) {
> +		iounmap(info->mem[i].internal_addr);
> +		info->mem[i].internal_addr = NULL;
> +	}
> +
> +	return err;
> +

I wonder do we really have to setup all the BAR's in uio_pci_generic?
The DPDK code works with uio_pci_generic already, and it didn't setup the BAR's.
One possible issue is that without that maybe kernel would not know about the
region used for MSI-X vectors table.
--
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]

#1239367 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 11:41, Stephen Hemminger wrote:
> On Sun,  4 Oct 2015 23:43:17 +0300
> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>
>> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
>> +{
>> +	int i, m = 0, p = 0, err;
>> +	static const char * const bar_names[] = {
>> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>> +		unsigned long start = pci_resource_start(pdev, i);
>> +		unsigned long flags = pci_resource_flags(pdev, i);
>> +		unsigned long len = pci_resource_len(pdev, i);
>> +
>> +		if (start == 0 || len == 0)
>> +			continue;
>> +
>> +		if (flags & IORESOURCE_MEM) {
>> +			void __iomem *addr;
>> +
>> +			if (m >= MAX_UIO_MAPS)
>> +				continue;
>> +
>> +			addr = ioremap(start, len);
>> +			if (addr == NULL) {
>> +				err = -EINVAL;
>> +				goto fail;
>> +			}
>> +
>> +			info->mem[m].name = bar_names[i];
>> +			info->mem[m].addr = start;
>> +			info->mem[m].internal_addr = addr;
>> +			info->mem[m].size = len;
>> +			info->mem[m].memtype = UIO_MEM_PHYS;
>> +			++m;
>> +		} else if (flags & IORESOURCE_IO) {
>> +			if (p >= MAX_UIO_PORT_REGIONS)
>> +				continue;
>> +
>> +			info->port[p].name = bar_names[i];
>> +			info->port[p].start = start;
>> +			info->port[p].size = len;
>> +			info->port[p].porttype = UIO_PORT_X86;
>> +			++p;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	for (i = 0; i < m; i++) {
>> +		iounmap(info->mem[i].internal_addr);
>> +		info->mem[i].internal_addr = NULL;
>> +	}
>> +
>> +	return err;
>> +
> I wonder do we really have to setup all the BAR's in uio_pci_generic?
> The DPDK code works with uio_pci_generic already, and it didn't setup the BAR's.

DPDK never used uio_pci_generic with MSI-X support so far and all MSI-X 
capable UIO DPDK drivers like igb_uio and your newly proposed uio_msi do 
map them all.
U also mentioned in the other thread that virtio requires portio bars too.
The thing is that generally bars are needed for programming the device 
therefore it's logical to have them exposed. In general different 
devices have different registers layout therefore a general driver may 
not know which bar exactly is going to be required for a specific device 
and for a specific usage. That's why I think that "map them all" 
approach is rather generic and appropriate. However if there are other 
motives not to do so that I'm missing, pls., let me know.

thanks,
vlad

> One possible issue is that without that maybe kernel would not know about the
> region used for MSI-X vectors table.

--
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]

#1239402 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 12:08, Vlad Zolotarov wrote:
>
>
> On 10/05/15 11:41, Stephen Hemminger wrote:
>> On Sun,  4 Oct 2015 23:43:17 +0300
>> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>>
>>> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
>>> +{
>>> +    int i, m = 0, p = 0, err;
>>> +    static const char * const bar_names[] = {
>>> +        "BAR0",    "BAR1",    "BAR2",    "BAR3", "BAR4",    "BAR5",
>>> +    };
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>>> +        unsigned long start = pci_resource_start(pdev, i);
>>> +        unsigned long flags = pci_resource_flags(pdev, i);
>>> +        unsigned long len = pci_resource_len(pdev, i);
>>> +
>>> +        if (start == 0 || len == 0)
>>> +            continue;
>>> +
>>> +        if (flags & IORESOURCE_MEM) {
>>> +            void __iomem *addr;
>>> +
>>> +            if (m >= MAX_UIO_MAPS)
>>> +                continue;
>>> +
>>> +            addr = ioremap(start, len);
>>> +            if (addr == NULL) {
>>> +                err = -EINVAL;
>>> +                goto fail;
>>> +            }
>>> +
>>> +            info->mem[m].name = bar_names[i];
>>> +            info->mem[m].addr = start;
>>> +            info->mem[m].internal_addr = addr;
>>> +            info->mem[m].size = len;
>>> +            info->mem[m].memtype = UIO_MEM_PHYS;
>>> +            ++m;
>>> +        } else if (flags & IORESOURCE_IO) {
>>> +            if (p >= MAX_UIO_PORT_REGIONS)
>>> +                continue;
>>> +
>>> +            info->port[p].name = bar_names[i];
>>> +            info->port[p].start = start;
>>> +            info->port[p].size = len;
>>> +            info->port[p].porttype = UIO_PORT_X86;
>>> +            ++p;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +fail:
>>> +    for (i = 0; i < m; i++) {
>>> +        iounmap(info->mem[i].internal_addr);
>>> +        info->mem[i].internal_addr = NULL;
>>> +    }
>>> +
>>> +    return err;
>>> +
>> I wonder do we really have to setup all the BAR's in uio_pci_generic?
>> The DPDK code works with uio_pci_generic already, and it didn't setup 
>> the BAR's.
>
> DPDK never used uio_pci_generic with MSI-X support so far and all 
> MSI-X capable UIO DPDK drivers like igb_uio and your newly proposed 
> uio_msi do map them all.
> U also mentioned in the other thread that virtio requires portio bars 
> too.
> The thing is that generally bars are needed for programming the device 
> therefore it's logical to have them exposed. In general different 
> devices have different registers layout therefore a general driver may 
> not know which bar exactly is going to be required for a specific 
> device and for a specific usage. That's why I think that "map them 
> all" approach is rather generic and appropriate. However if there are 
> other motives not to do so that I'm missing, pls., let me know.

Having said all that however I'd agree if someone would say that 
mappings setting would rather come as a separate patch in this series... ;)
it will in v4...


>
> thanks,
> vlad
>
>> One possible issue is that without that maybe kernel would not know 
>> about the
>> region used for MSI-X vectors table.
>

--
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]

#1239929 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 01:06:09PM +0300, Vlad Zolotarov wrote:
> Having said all that however I'd agree if someone would say that mappings
> setting would rather come as a separate patch in this series... ;)
> it will in v4...

Just drop this is my advice. There are enough controversial things here
as it is.

-- 
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]

#1239379 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 11:41, Stephen Hemminger wrote:
> On Sun,  4 Oct 2015 23:43:17 +0300
> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>
>> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
>> +{
>> +	int i, m = 0, p = 0, err;
>> +	static const char * const bar_names[] = {
>> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>> +		unsigned long start = pci_resource_start(pdev, i);
>> +		unsigned long flags = pci_resource_flags(pdev, i);
>> +		unsigned long len = pci_resource_len(pdev, i);
>> +
>> +		if (start == 0 || len == 0)
>> +			continue;
>> +
>> +		if (flags & IORESOURCE_MEM) {
>> +			void __iomem *addr;
>> +
>> +			if (m >= MAX_UIO_MAPS)
>> +				continue;
>> +
>> +			addr = ioremap(start, len);
>> +			if (addr == NULL) {
>> +				err = -EINVAL;
>> +				goto fail;
>> +			}
>> +
>> +			info->mem[m].name = bar_names[i];
>> +			info->mem[m].addr = start;
>> +			info->mem[m].internal_addr = addr;
>> +			info->mem[m].size = len;
>> +			info->mem[m].memtype = UIO_MEM_PHYS;
>> +			++m;
>> +		} else if (flags & IORESOURCE_IO) {
>> +			if (p >= MAX_UIO_PORT_REGIONS)
>> +				continue;
>> +
>> +			info->port[p].name = bar_names[i];
>> +			info->port[p].start = start;
>> +			info->port[p].size = len;
>> +			info->port[p].porttype = UIO_PORT_X86;
>> +			++p;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	for (i = 0; i < m; i++) {
>> +		iounmap(info->mem[i].internal_addr);
>> +		info->mem[i].internal_addr = NULL;
>> +	}
>> +
>> +	return err;
>> +
> I wonder do we really have to setup all the BAR's in uio_pci_generic?
> The DPDK code works with uio_pci_generic already, and it didn't setup the BAR's.
> One possible issue is that without that maybe kernel would not know about the
> region used for MSI-X vectors table.

So, what's your point? It sounds like u are for setting the mappings in 
the uio_pci_generic after all, aren't u? ;)


--
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