* [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver @ 2015-10-04 20:39 Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-04 20:39 UTC (permalink / raw) To: linux-kernel, mst, hjk, regkh, corbet Cc: bruce.richardson, avi, gleb, stephen, alexander.duyck, Vlad Zolotarov 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/3] uio: add ioctl support 2015-10-04 20:39 [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov @ 2015-10-04 20:39 ` Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 3/3] Documentation: update uio-howto Vlad Zolotarov 2 siblings, 0 replies; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-04 20:39 UTC (permalink / raw) To: linux-kernel, mst, hjk, regkh, corbet Cc: bruce.richardson, avi, gleb, stephen, alexander.duyck, Vlad Zolotarov Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> --- drivers/uio/uio.c | 15 +++++++++++++++ include/linux/uio_driver.h | 3 +++ 2 files changed, 18 insertions(+) diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 8196581..714b0e5 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma) } } +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) +{ + struct uio_listener *listener = filep->private_data; + struct uio_device *idev = listener->dev; + + if (!idev->info) + return -EIO; + + if (!idev->info->ioctl) + return -ENOTTY; + + return idev->info->ioctl(idev->info, cmd, arg); +} + static const struct file_operations uio_fops = { .owner = THIS_MODULE, .open = uio_open, @@ -712,6 +726,7 @@ static const struct file_operations uio_fops = { .write = uio_write, .mmap = uio_mmap, .poll = uio_poll, + .unlocked_ioctl = uio_ioctl, .fasync = uio_fasync, .llseek = noop_llseek, }; diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h index 32c0e83..10d7833 100644 --- a/include/linux/uio_driver.h +++ b/include/linux/uio_driver.h @@ -89,6 +89,7 @@ struct uio_device { * @mmap: mmap operation for this uio device * @open: open operation for this uio device * @release: release operation for this uio device + * @ioctl: ioctl handler * @irqcontrol: disable/enable irqs when 0/1 is written to /dev/uioX */ struct uio_info { @@ -105,6 +106,8 @@ struct uio_info { int (*open)(struct uio_info *info, struct inode *inode); int (*release)(struct uio_info *info, struct inode *inode); int (*irqcontrol)(struct uio_info *info, s32 irq_on); + int (*ioctl)(struct uio_info *info, unsigned int cmd, + unsigned long arg); }; extern int __must_check -- 2.1.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support 2015-10-04 20:39 [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov @ 2015-10-04 20:39 ` Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 3/3] Documentation: update uio-howto Vlad Zolotarov 2 siblings, 0 replies; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-04 20:39 UTC (permalink / raw) To: linux-kernel, mst, hjk, regkh, corbet Cc: bruce.richardson, avi, gleb, stephen, alexander.duyck, Vlad Zolotarov Add support for MSI and MSI-X interrupt modes: - Interrupt mode selection order is: INT#X (for backward compatibility) -> MSI-X -> MSI. - Add ioctl() commands: - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode. - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs. - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio). - 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 3/3] Documentation: update uio-howto 2015-10-04 20:39 [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov @ 2015-10-04 20:39 ` Vlad Zolotarov 2 siblings, 0 replies; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-04 20:39 UTC (permalink / raw) To: linux-kernel, mst, hjk, regkh, corbet Cc: bruce.richardson, avi, gleb, stephen, alexander.duyck, Vlad Zolotarov Change the chapters related to uio_pci_generic that refer interrupt mode. Add the relevant explanation regarding MSI and MSI-X interrupt modes support. Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> --- Documentation/DocBook/uio-howto.tmpl | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl index cd0e452..a176129 100644 --- a/Documentation/DocBook/uio-howto.tmpl +++ b/Documentation/DocBook/uio-howto.tmpl @@ -46,6 +46,12 @@ GPL version 2. <revhistory> <revision> + <revnumber>0.10</revnumber> + <date>2015-10-04</date> + <authorinitials>vz</authorinitials> + <revremark>Added MSI and MSI-X support to uio_pci_generic.</revremark> + </revision> + <revision> <revnumber>0.9</revnumber> <date>2009-07-16</date> <authorinitials>mst</authorinitials> @@ -935,15 +941,32 @@ and look in the output for failure reasons <sect1 id="uio_pci_generic_internals"> <title>Things to know about uio_pci_generic</title> <para> -Interrupts are handled using the Interrupt Disable bit in the PCI command +Interrupts are handled either as MSI-X or MSI interrupts (if the device supports it) or +as legacy INTx interrupts. By default INTx interrupts are used. + </para> + <para> +uio_pci_generic automatically configures a device to use INTx interrupt for backward +compatibility. If INTx are not available MSI-X interrupts will be used if the device +supports it and if not MSI interrupts are going to be used. If none of the interrupts +modes is supported probe() will fail. + </para> + <para> +To get the used interrupt mode application has to use UIO_PCI_GENERIC_INT_MODE_GET ioctl +command. +UIO_PCI_GENERIC_IRQ_NUM_GET ioctl command may be used to get the total number of IRQs. +Then UIO_PCI_GENERIC_IRQ_SET ioctl command may be used to bind a specific eventfd to a specific +IRQ vector. + </para> + <para> +Legacy interrupts are handled using the Interrupt Disable bit in the PCI command register and Interrupt Status bit in the PCI status register. All devices compliant to PCI 2.3 (circa 2002) and all compliant PCI Express devices should support these bits. uio_pci_generic detects this support, and won't bind to devices which do not support the Interrupt Disable Bit in the command register. </para> <para> -On each interrupt, uio_pci_generic sets the Interrupt Disable bit. -This prevents the device from generating further interrupts +If legacy interrupts are used, uio_pci_generic sets the Interrupt Disable bit on +each interrupt. This prevents the device from generating further interrupts until the bit is cleared. The userspace driver should clear this bit before blocking and waiting for more interrupts. </para> -- 2.1.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver @ 2015-10-04 20:43 Vlad Zolotarov 2015-10-04 20:43 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov 0 siblings, 1 reply; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-04 20:43 UTC (permalink / raw) To: linux-kernel, mst, hjk, corbet, gregkh Cc: bruce.richardson, avi, gleb, stephen, alexander.duyck, Vlad Zolotarov 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/3] uio: add ioctl support 2015-10-04 20:43 [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov @ 2015-10-04 20:43 ` Vlad Zolotarov 2015-10-05 3:03 ` Greg KH 0 siblings, 1 reply; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-04 20:43 UTC (permalink / raw) To: linux-kernel, mst, hjk, corbet, gregkh Cc: bruce.richardson, avi, gleb, stephen, alexander.duyck, Vlad Zolotarov Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> --- drivers/uio/uio.c | 15 +++++++++++++++ include/linux/uio_driver.h | 3 +++ 2 files changed, 18 insertions(+) diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 8196581..714b0e5 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma) } } +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) +{ + struct uio_listener *listener = filep->private_data; + struct uio_device *idev = listener->dev; + + if (!idev->info) + return -EIO; + + if (!idev->info->ioctl) + return -ENOTTY; + + return idev->info->ioctl(idev->info, cmd, arg); +} + static const struct file_operations uio_fops = { .owner = THIS_MODULE, .open = uio_open, @@ -712,6 +726,7 @@ static const struct file_operations uio_fops = { .write = uio_write, .mmap = uio_mmap, .poll = uio_poll, + .unlocked_ioctl = uio_ioctl, .fasync = uio_fasync, .llseek = noop_llseek, }; diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h index 32c0e83..10d7833 100644 --- a/include/linux/uio_driver.h +++ b/include/linux/uio_driver.h @@ -89,6 +89,7 @@ struct uio_device { * @mmap: mmap operation for this uio device * @open: open operation for this uio device * @release: release operation for this uio device + * @ioctl: ioctl handler * @irqcontrol: disable/enable irqs when 0/1 is written to /dev/uioX */ struct uio_info { @@ -105,6 +106,8 @@ struct uio_info { int (*open)(struct uio_info *info, struct inode *inode); int (*release)(struct uio_info *info, struct inode *inode); int (*irqcontrol)(struct uio_info *info, s32 irq_on); + int (*ioctl)(struct uio_info *info, unsigned int cmd, + unsigned long arg); }; extern int __must_check -- 2.1.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-04 20:43 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov @ 2015-10-05 3:03 ` Greg KH 2015-10-05 7:33 ` Vlad Zolotarov 0 siblings, 1 reply; 17+ messages in thread From: Greg KH @ 2015-10-05 3:03 UTC (permalink / raw) To: Vlad Zolotarov Cc: linux-kernel, mst, hjk, corbet, bruce.richardson, avi, gleb, stephen, alexander.duyck On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote: > Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> > --- > drivers/uio/uio.c | 15 +++++++++++++++ > include/linux/uio_driver.h | 3 +++ > 2 files changed, 18 insertions(+) You add an ioctl yet fail to justify _why_ you need/want that ioctl, and you don't document it at all? Come on, you know better than that, no one can take a patch that has no changelog comments at all like this :( Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so you had better have a really compelling argument as to why this is the _ONLY_ way you can solve this unknown problem by using such a horrid thing... thanks, greg k-h ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-05 3:03 ` Greg KH @ 2015-10-05 7:33 ` Vlad Zolotarov 2015-10-05 8:01 ` Greg KH 0 siblings, 1 reply; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-05 7:33 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, mst, hjk, corbet, bruce.richardson, avi, gleb, stephen, alexander.duyck On 10/05/15 06:03, Greg KH wrote: > On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote: >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> >> --- >> drivers/uio/uio.c | 15 +++++++++++++++ >> include/linux/uio_driver.h | 3 +++ >> 2 files changed, 18 insertions(+) > You add an ioctl yet fail to justify _why_ you need/want that ioctl, and > you don't document it at all? Come on, you know better than that, no > one can take a patch that has no changelog comments at all like this :( My bad. U are absolutely right here - it was late and I was tired that I missed that to someone it may not be so "crystal clear" like it is to me... :) Again, my bad - let me clarify it here and if we agree I'll respin the series with all relevant updates including the changelog. > > Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so > you had better have a really compelling argument as to why this is the > _ONLY_ way you can solve this unknown problem by using such a horrid > thing... Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, but only lets the underlying PCI drivers to have them. UIO in this case is only a proxy. The main idea of this series is, as mentioned in PATCH0, to add the MSI and MSI-X support for uio_pci_generic driver. While with MSI the things are quite simple and we may just ride the existing infrastructure, with the MSI-X the things get a bit more complicated since we may have more than one interrupt vector. Therefore we have to decide which interface we want to give to the user. One option could be to make all existing interrupts trigger the same objects in UIO as the current single interrupt does, however this would create an awkward, quite not-flexible semantics. For instance a regular (kernel) driver has a separate state machine for each interrupt line, which sometimes runs on a separate CPU, etc. This way we get to the second option - allow indication for each separate interrupt vector. And for obvious reasons (mentioned above) we (Stephen has sent a similar series on a dpdk-dev list) chose a second approach. In order not to invent the wheel we mimicked the VFIO approach, which allows to bind the pre-allocated eventfd descriptor to the specific interrupt vector using the ioctl(). The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is: struct uio_pci_generic_irq_set { int vec; /* index of the IRQ to connect to starting from 0 */ int fd; }; where "vec" is an index of the IRQ starting from 0 and "fd" is an eventfd file descriptor a user wants to poll() for in order to get the interrupt indications. If "fd" is less than 0, ioctl() will unbind the interrupt from the previously bound eventfd descriptor. This way a user may poll() for any IRQ it wants separately, or epoll() for any subset of them, or do whatever he/she wants to do. That's why we needed the ioctl(). I admit that it may not be the _ONLY_ way to achieve the goal described above but again we took VFIO approach as a template for a solution and just followed it. If u think there is more elegant/robust/better way to do so, pls., share. :) thanks, vlad > > thanks, > > greg k-h On 10/05/15 06:03, Greg KH wrote: > On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote: >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> >> --- >> drivers/uio/uio.c | 15 +++++++++++++++ >> include/linux/uio_driver.h | 3 +++ >> 2 files changed, 18 insertions(+) > You add an ioctl yet fail to justify _why_ you need/want that ioctl, and > you don't document it at all? Come on, you know better than that, no > one can take a patch that has no changelog comments at all like this :( > > Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so > you had better have a really compelling argument as to why this is the > _ONLY_ way you can solve this unknown problem by using such a horrid > thing... > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-05 7:33 ` Vlad Zolotarov @ 2015-10-05 8:01 ` Greg KH 2015-10-05 10:36 ` Vlad Zolotarov 0 siblings, 1 reply; 17+ messages in thread From: Greg KH @ 2015-10-05 8:01 UTC (permalink / raw) To: Vlad Zolotarov Cc: linux-kernel, mst, hjk, corbet, bruce.richardson, avi, gleb, stephen, alexander.duyck On Mon, Oct 05, 2015 at 10:33:20AM +0300, Vlad Zolotarov wrote: > On 10/05/15 06:03, Greg KH wrote: > >On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote: > >>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> > >>--- > >> drivers/uio/uio.c | 15 +++++++++++++++ > >> include/linux/uio_driver.h | 3 +++ > >> 2 files changed, 18 insertions(+) > >You add an ioctl yet fail to justify _why_ you need/want that ioctl, and > >you don't document it at all? Come on, you know better than that, no > >one can take a patch that has no changelog comments at all like this :( > > My bad. U are absolutely right here - it was late and I was tired that I > missed that to someone it may not be so "crystal clear" like it is to me... > :) > Again, my bad - let me clarify it here and if we agree I'll respin the > series with all relevant updates including the changelog. > > > > >Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so > >you had better have a really compelling argument as to why this is the > >_ONLY_ way you can solve this unknown problem by using such a horrid > >thing... > > Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, but > only lets the underlying PCI drivers to have them. UIO in this case is only > a proxy. Exactly, and I don't want to provide an ioctl "proxy" for UIO drivers. That way lies madness and horrid code, and other nasty things (hint, each ioctl is a custom syscall, so you are opening up the box for all sorts of bad things to happen in drivers...) For example, your ioctl you use here is incorrect, and will fail horribly on a large majority of systems. I don't want to open up the requirements that more people have to know how to "do it right" in order to use the UIO interface for their drivers, as people will get it wrong (as this patch series shows...) > The main idea of this series is, as mentioned in PATCH0, to add the MSI and > MSI-X support for uio_pci_generic driver. Yes, I know that, but I don't see anything that shows _how_ to use this api. And then there's the issue of why we even need this, why not just write a whole new driver for this, like the previous driver did (which also used ioctls, yes, I didn't have the chance to object to that before everyone else did...) > While with MSI the things are quite simple and we may just ride the existing > infrastructure, with the MSI-X the things get a bit more complicated since > we may have more than one interrupt vector. Therefore we have to decide > which interface we want to give to the user. > > One option could be to make all existing interrupts trigger the same objects > in UIO as the current single interrupt does, however this would create an > awkward, quite not-flexible semantics. For instance a regular (kernel) > driver has a separate state machine for each interrupt line, which sometimes > runs on a separate CPU, etc. This way we get to the second option - allow > indication for each separate interrupt vector. And for obvious reasons > (mentioned above) we (Stephen has sent a similar series on a dpdk-dev list) > chose a second approach. > > In order not to invent the wheel we mimicked the VFIO approach, which allows > to bind the pre-allocated eventfd descriptor to the specific interrupt > vector using the ioctl(). > > The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is: > > struct uio_pci_generic_irq_set { > int vec; /* index of the IRQ to connect to starting from 0 */ > int fd; > }; And that's broken :( NEVER use an "int" for an ioctl, it is wrong and will cause horrible issues on a large number of systems. That is what the __u16 and friends variable types are for. You know better than this :) greg k-h ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-05 8:01 ` Greg KH @ 2015-10-05 10:36 ` Vlad Zolotarov 2015-10-05 20:02 ` Michael S. Tsirkin 0 siblings, 1 reply; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-05 10:36 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, mst, hjk, corbet, bruce.richardson, avi, gleb, stephen, alexander.duyck On 10/05/15 11:01, Greg KH wrote: > On Mon, Oct 05, 2015 at 10:33:20AM +0300, Vlad Zolotarov wrote: >> On 10/05/15 06:03, Greg KH wrote: >>> On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote: >>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> >>>> --- >>>> drivers/uio/uio.c | 15 +++++++++++++++ >>>> include/linux/uio_driver.h | 3 +++ >>>> 2 files changed, 18 insertions(+) >>> You add an ioctl yet fail to justify _why_ you need/want that ioctl, and >>> you don't document it at all? Come on, you know better than that, no >>> one can take a patch that has no changelog comments at all like this :( >> My bad. U are absolutely right here - it was late and I was tired that I >> missed that to someone it may not be so "crystal clear" like it is to me... >> :) >> Again, my bad - let me clarify it here and if we agree I'll respin the >> series with all relevant updates including the changelog. >> >>> Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so >>> you had better have a really compelling argument as to why this is the >>> _ONLY_ way you can solve this unknown problem by using such a horrid >>> thing... >> Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, but >> only lets the underlying PCI drivers to have them. UIO in this case is only >> a proxy. > Exactly, and I don't want to provide an ioctl "proxy" for UIO drivers. > That way lies madness and horrid code, and other nasty things (hint, > each ioctl is a custom syscall, so you are opening up the box for all > sorts of bad things to happen in drivers...) > > For example, your ioctl you use here is incorrect, and will fail > horribly on a large majority of systems. I don't want to open up the > requirements that more people have to know how to "do it right" in order > to use the UIO interface for their drivers, as people will get it wrong > (as this patch series shows...) Sometimes there is no other (better) way to get things done. And bugs - isn't it what code review is for? ;) I'll fix the "int" issue. > >> The main idea of this series is, as mentioned in PATCH0, to add the MSI and >> MSI-X support for uio_pci_generic driver. > Yes, I know that, but I don't see anything that shows _how_ to use this > api. I get that, i'll extend PATCH3 of this series with a detailed description in v4. U use it as follows: 1. Bind the PCI function to uio_pci_generic. 2. Query for its interrupt mode with UIO_PCI_GENERIC_INT_MODE_GET ioctl. 3. If interrupt mode is INT#x or MSI - use the current UIO interface for polling, namely use the UIO file descriptor. 4. Else 1. Query for the number of MSI-X vectors with UIO_PCI_GENERIC_IRQ_NUM_GET ioctl. 2. Allocate the required number of eventfd descriptors using eventfd() from sys/eventfd.h. 3. Bind them to the required IRQs with UIO_PCI_GENERIC_IRQ_SET ioctl. 5. When done, just unbind the PCI function from the uio_pci_generic. > And then there's the issue of why we even need this, why not just > write a whole new driver for this, like the previous driver did (which > also used ioctls, yes, I didn't have the chance to object to that before > everyone else did...) Which "previous driver" do u refer here? IMHO writing something instead of UIO (not just uio_pci_generic) seems like an overkill for solving this issue. Supporting MSI-X interrupts seem like a very beneficial feature for uio_pci_generic and it's really not _THAT_ complicated API - just look at VFIO for a comparison... ;) uio_pci_generic is clearly missing this important feature. And creating another user space driver infrastructure just to add it seems extremely unjustified. > >> While with MSI the things are quite simple and we may just ride the existing >> infrastructure, with the MSI-X the things get a bit more complicated since >> we may have more than one interrupt vector. Therefore we have to decide >> which interface we want to give to the user. >> >> One option could be to make all existing interrupts trigger the same objects >> in UIO as the current single interrupt does, however this would create an >> awkward, quite not-flexible semantics. For instance a regular (kernel) >> driver has a separate state machine for each interrupt line, which sometimes >> runs on a separate CPU, etc. This way we get to the second option - allow >> indication for each separate interrupt vector. And for obvious reasons >> (mentioned above) we (Stephen has sent a similar series on a dpdk-dev list) >> chose a second approach. >> >> In order not to invent the wheel we mimicked the VFIO approach, which allows >> to bind the pre-allocated eventfd descriptor to the specific interrupt >> vector using the ioctl(). >> >> The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is: >> >> struct uio_pci_generic_irq_set { >> int vec; /* index of the IRQ to connect to starting from 0 */ >> int fd; >> }; > And that's broken :( Good catch. Thanks. Will fix. I'm not a big ioctl fan myself but unfortunately I don't see a good alternative here. proc? Would it make it cleaner? > > NEVER use an "int" for an ioctl, it is wrong and will cause horrible > issues on a large number of systems. That is what the __u16 and friends > variable types are for. You know better than this :) > > greg k-h ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-05 10:36 ` Vlad Zolotarov @ 2015-10-05 20:02 ` Michael S. Tsirkin [not found] ` <CAOYyTHZ2=UCYxuJKvd5S6qxp=84DBq5bMadg5wL0rFLZBh2-8Q@mail.gmail.com> 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2015-10-05 20:02 UTC (permalink / raw) To: Vlad Zolotarov Cc: Greg KH, linux-kernel, hjk, corbet, bruce.richardson, avi, gleb, stephen, alexander.duyck On Mon, Oct 05, 2015 at 01:36:35PM +0300, Vlad Zolotarov wrote: > >And then there's the issue of why we even need this, why not just > >write a whole new driver for this, like the previous driver did (which > >also used ioctls, yes, I didn't have the chance to object to that before > >everyone else did...) > > Which "previous driver" do u refer here? > IMHO writing something instead of UIO (not just uio_pci_generic) seems like > an overkill for solving this issue. Supporting MSI-X interrupts seem like a > very beneficial feature for uio_pci_generic and it's really not _THAT_ > complicated API - just look at VFIO for a comparison... ;) Except most things VFIO does is actually there for security. Which, for a device that can do DMA and isn't even behind an IOMMU, sounds like a pretty big deal actually. > uio_pci_generic is clearly missing this important feature. And creating > another user space driver infrastructure just to add it seems extremely > unjustified. uio_pci_generic was always intended to be used with extremely simple devices which don't do DMA, or where someone else has set up the IOMMU (like kvm does with CONFIG_KVM_DEVICE_ASSIGNMENT). We need to be much more careful with MSI if there's no IOMMU. -- MST ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAOYyTHZ2=UCYxuJKvd5S6qxp=84DBq5bMadg5wL0rFLZBh2-8Q@mail.gmail.com>]
* Re: [PATCH v3 1/3] uio: add ioctl support [not found] ` <CAOYyTHZ2=UCYxuJKvd5S6qxp=84DBq5bMadg5wL0rFLZBh2-8Q@mail.gmail.com> @ 2015-10-05 22:29 ` Michael S. Tsirkin 2015-10-06 8:33 ` Vlad Zolotarov 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2015-10-05 22:29 UTC (permalink / raw) To: Vladislav Zolotarov Cc: Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On Tue, Oct 06, 2015 at 12:43:45AM +0300, Vladislav Zolotarov wrote: > So, like it has already been asked in a different thread I'm going to > ask a rhetorical question: what adding an MSI and MSI-X interrupts support to > uio_pci_generic has to do with security? memory protection is a better term than security. It's very simple: you enable bus mastering and you ask userspace to map all device BARs. One of these BARs holds the address to which device writes to trigger MSI-X interrupt. This is how MSI-X works, internally: from the point of view of PCI it's a memory write. It just so happens that the destination address is in the interrupt controller, that triggers an interrupt. But a bug in this userspace application can corrupt the MSI-X table, which in turn can easily corrupt kernel memory, or unrelated processes's memory. This is in my opinion unacceptable. So you need to be very careful - probably need to reset device before you even enable bus master - prevent userspace from touching msi config - prevent userspace from moving BARs since msi-x config is within a BAR - detect reset and prevent linux from touching device while it's under reset The list goes on and on. This is pretty much what VFIO spent the last 3 years doing, except VFIO also can do IOMMU groups. > What "security threat" does it add > that u don't already have today? Yes, userspace can create this today if it tweaks PCI config space to enable MSI-X, then corrupts the MSI-X table. It's unfortunate that we don't yet prevent this, but at least you need two things to go wrong for this to trigger. The reason, as I tried to point out, is simply that I didn't think uio_pci_generic will be used for these configurations. But there's nothing fundamental here that makes them secure and that therefore makes your patches secure as well. Fixing this to make uio_pci_generic write-protect MSI/MSI-X enable registers sounds kind of reasonable, this shouldn't be too hard. -- MST ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-05 22:29 ` Michael S. Tsirkin @ 2015-10-06 8:33 ` Vlad Zolotarov 2015-10-06 14:19 ` Michael S. Tsirkin 0 siblings, 1 reply; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-06 8:33 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On 10/06/15 01:29, Michael S. Tsirkin wrote: > On Tue, Oct 06, 2015 at 12:43:45AM +0300, Vladislav Zolotarov wrote: >> So, like it has already been asked in a different thread I'm going to >> ask a rhetorical question: what adding an MSI and MSI-X interrupts support to >> uio_pci_generic has to do with security? > memory protection is a better term than security. > > It's very simple: you enable bus mastering and you ask userspace to map > all device BARs. One of these BARs holds the address to which device > writes to trigger MSI-X interrupt. > > This is how MSI-X works, internally: from the point of view of > PCI it's a memory write. It just so happens that the destination > address is in the interrupt controller, that triggers an interrupt. > > But a bug in this userspace application can corrupt the MSI-X table, > which in turn can easily corrupt kernel memory, or unrelated processes's > memory. This is in my opinion unacceptable. > > So you need to be very careful > - probably need to reset device before you even enable bus master > - prevent userspace from touching msi config > - prevent userspace from moving BARs since msi-x config is within a BAR > - detect reset and prevent linux from touching device while it's under > reset > > The list goes on and on. > > This is pretty much what VFIO spent the last 3 years doing, except VFIO > also can do IOMMU groups. > >> What "security threat" does it add >> that u don't already have today? > Yes, userspace can create this today if it tweaks PCI config space to > enable MSI-X, then corrupts the MSI-X table. It's unfortunate that we > don't yet prevent this, but at least you need two things to go wrong for > this to trigger. > > The reason, as I tried to point out, is simply that I didn't think > uio_pci_generic will be used for these configurations. > But there's nothing fundamental here that makes them secure > and that therefore makes your patches secure as well. > > Fixing this to make uio_pci_generic write-protect MSI/MSI-X enable > registers sounds kind of reasonable, this shouldn't be too hard. Sure. But like u've just pointed out yourself - this is a general issue and it has nothing to do with the ability to get notifications per MSI-X/MSI interrupts, which this series adds (bus mastering may and is easily enabled from the user space - look for pci_uio_set_bus_master() function in the DPDK). So, while I absolutely agree with u in regard to the fact that we have a security/memory corruption threat in the current in-tree uio_pci_generic - the solution u propose should be a matter of a separate patch and is obviously orthogonal to this series. thanks, vlad > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-06 8:33 ` Vlad Zolotarov @ 2015-10-06 14:19 ` Michael S. Tsirkin 2015-10-06 14:30 ` Gleb Natapov 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2015-10-06 14:19 UTC (permalink / raw) To: Vlad Zolotarov Cc: Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote: > the solution u propose should be a matter of a separate patch and is > obviously orthogonal to this series. Doesn't work this way, sorry. You want a patch enabling MSI merged, you need to secure the MSI configuration. And it's going to be a lot of work, duplicating a bunch of code from VFIO. -- MST ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-06 14:19 ` Michael S. Tsirkin @ 2015-10-06 14:30 ` Gleb Natapov 2015-10-06 15:19 ` Michael S. Tsirkin 0 siblings, 1 reply; 17+ messages in thread From: Gleb Natapov @ 2015-10-06 14:30 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Vlad Zolotarov, Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote: > On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote: > > the solution u propose should be a matter of a separate patch and is > > obviously orthogonal to this series. > > Doesn't work this way, sorry. You want a patch enabling MSI merged, > you need to secure the MSI configuration. > MSI can be enabled right now without the patch by writing directly into PCI bar. The only thing this patch adds is forwarding the interrupt to an eventfd. -- Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-06 14:30 ` Gleb Natapov @ 2015-10-06 15:19 ` Michael S. Tsirkin 2015-10-06 15:31 ` Vlad Zolotarov 2015-10-06 15:57 ` Gleb Natapov 0 siblings, 2 replies; 17+ messages in thread From: Michael S. Tsirkin @ 2015-10-06 15:19 UTC (permalink / raw) To: Gleb Natapov Cc: Vlad Zolotarov, Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote: > On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote: > > On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote: > > > the solution u propose should be a matter of a separate patch and is > > > obviously orthogonal to this series. > > > > Doesn't work this way, sorry. You want a patch enabling MSI merged, > > you need to secure the MSI configuration. > > > MSI can be enabled right now without the patch by writing directly into > PCI bar. By poking at config registers in sysfs? We can block this, or we can log this, pretty easily. We don't ATM but it's not hard to do. > The only thing this patch adds is forwarding the interrupt to > an eventfd. This one just adds a bunch of ioctls. The next ones do more than you describe. > -- > Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-06 15:19 ` Michael S. Tsirkin @ 2015-10-06 15:31 ` Vlad Zolotarov 2015-10-06 15:57 ` Gleb Natapov 1 sibling, 0 replies; 17+ messages in thread From: Vlad Zolotarov @ 2015-10-06 15:31 UTC (permalink / raw) To: Michael S. Tsirkin, Gleb Natapov Cc: Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On 10/06/15 18:19, Michael S. Tsirkin wrote: > On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote: >> On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote: >>> On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote: >>>> the solution u propose should be a matter of a separate patch and is >>>> obviously orthogonal to this series. >>> Doesn't work this way, sorry. You want a patch enabling MSI merged, >>> you need to secure the MSI configuration. >>> >> MSI can be enabled right now without the patch by writing directly into >> PCI bar. > By poking at config registers in sysfs? We can block this, or we > can log this, pretty easily. We don't ATM but it's not hard to do. > >> The only thing this patch adds is forwarding the interrupt to >> an eventfd. > This one just adds a bunch of ioctls. The next ones do > more than you describe. This one adds zero ioctls and the next one does exactly what Gleb describes. > >> -- >> Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] uio: add ioctl support 2015-10-06 15:19 ` Michael S. Tsirkin 2015-10-06 15:31 ` Vlad Zolotarov @ 2015-10-06 15:57 ` Gleb Natapov 1 sibling, 0 replies; 17+ messages in thread From: Gleb Natapov @ 2015-10-06 15:57 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Vlad Zolotarov, Greg KH, Bruce Richardson, linux-kernel, hjk, avi, corbet, alexander.duyck, gleb, stephen On Tue, Oct 06, 2015 at 06:19:34PM +0300, Michael S. Tsirkin wrote: > On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote: > > On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote: > > > On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote: > > > > the solution u propose should be a matter of a separate patch and is > > > > obviously orthogonal to this series. > > > > > > Doesn't work this way, sorry. You want a patch enabling MSI merged, > > > you need to secure the MSI configuration. > > > > > MSI can be enabled right now without the patch by writing directly into > > PCI bar. > > By poking at config registers in sysfs? We can block this, or we > can log this, pretty easily. We don't ATM but it's not hard to do. > Blocking this will break userspace API. As a maintainer you should know that we do not break userspace APIs. Logging this is fine, but how exactly it helps you with "security"? The patch in question already taints the kernel which is much stronger than logging. > > The only thing this patch adds is forwarding the interrupt to > > an eventfd. > > This one just adds a bunch of ioctls. The next ones do > more than you describe. > Yes, it adds bunch of ioctls to do exactly what I wrote above. What point have you tried to make by this statement? It eluded me. -- Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-10-06 15:57 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-04 20:39 [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov 2015-10-04 20:39 ` [PATCH v3 3/3] Documentation: update uio-howto Vlad Zolotarov 2015-10-04 20:43 [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov 2015-10-04 20:43 ` [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov 2015-10-05 3:03 ` Greg KH 2015-10-05 7:33 ` Vlad Zolotarov 2015-10-05 8:01 ` Greg KH 2015-10-05 10:36 ` Vlad Zolotarov 2015-10-05 20:02 ` Michael S. Tsirkin [not found] ` <CAOYyTHZ2=UCYxuJKvd5S6qxp=84DBq5bMadg5wL0rFLZBh2-8Q@mail.gmail.com> 2015-10-05 22:29 ` Michael S. Tsirkin 2015-10-06 8:33 ` Vlad Zolotarov 2015-10-06 14:19 ` Michael S. Tsirkin 2015-10-06 14:30 ` Gleb Natapov 2015-10-06 15:19 ` Michael S. Tsirkin 2015-10-06 15:31 ` Vlad Zolotarov 2015-10-06 15:57 ` Gleb Natapov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).