| Message ID | 1467372912-31113-1-git-send-email-ferruh.yigit@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Thank you Ferruh for taking care of igb_uio. 2016-07-01 12:35, Ferruh Yigit: > With kernels enabled CONFIG_IO_STRICT_DEVMEM option mmap the iomem area > to userspace fails: Maybe some words are missing. Please check punctuation of the whole commit message to make it easier to understand. > EAL: pci_map_resource(): > cannot mmap(39, 0x7f1c51800000, 0x100000, 0x0): > Invalid argument (0xffffffffffffffff) > > As a workaround igb_uio can stop reserving PCI memory resources, from > kernel point of view io-memory region looks like idle and mmap works > again. > > With this update device io-memory range is not protected against any > other kernel driver claim ownership on those resources, which shouldn't > be a problem for dpdk usage module. Why it should not be a problem? Please could you give an example of what could happen? This patch fixes a problem with recent kernels (not mentioned above) which offer the uio_pci_generic alternative. That's why I think we should fix it only if there is absolutely no regression for older kernels.
On 7/1/2016 1:47 PM, Thomas Monjalon wrote: > Thank you Ferruh for taking care of igb_uio. > > 2016-07-01 12:35, Ferruh Yigit: >> With kernels enabled CONFIG_IO_STRICT_DEVMEM option mmap the iomem area >> to userspace fails: > > Maybe some words are missing. > Please check punctuation of the whole commit message to make it easier > to understand. I will re-word. > >> EAL: pci_map_resource(): >> cannot mmap(39, 0x7f1c51800000, 0x100000, 0x0): >> Invalid argument (0xffffffffffffffff) >> >> As a workaround igb_uio can stop reserving PCI memory resources, from >> kernel point of view io-memory region looks like idle and mmap works >> again. >> >> With this update device io-memory range is not protected against any >> other kernel driver claim ownership on those resources, which shouldn't >> be a problem for dpdk usage module. > > Why it should not be a problem? request_mem_region() is a way for driver informing the rest of the kernel that memory region is used. And with CONFIG_IO_STRICT_DEVMEM=y, userspace also prevented to touch that ares. But for igb_uio, we explicitly want userspace to access that memory range. > Please could you give an example of what could happen? Technically device lost a protection of its memory region against any other driver, but I am not sure if this is real threat in practical life. Also this is same in uio_pci_generic, it doesn't reserve the memory. > > This patch fixes a problem with recent kernels (not mentioned above) > which offer the uio_pci_generic alternative. Will give kernel version information. > That's why I think we should fix it only if there is absolutely no > regression for older kernels. > Totally agreed, that is why I expressed my concern, let this patch hang around a little.
2016-07-01 15:39, Ferruh Yigit: > On 7/1/2016 1:47 PM, Thomas Monjalon wrote: > >> As a workaround igb_uio can stop reserving PCI memory resources, from > >> kernel point of view io-memory region looks like idle and mmap works > >> again. > >> > >> With this update device io-memory range is not protected against any > >> other kernel driver claim ownership on those resources, which shouldn't > >> be a problem for dpdk usage module. > > > > Why it should not be a problem? > > request_mem_region() is a way for driver informing the rest of the > kernel that memory region is used. > And with CONFIG_IO_STRICT_DEVMEM=y, userspace also prevented to touch > that ares. > But for igb_uio, we explicitly want userspace to access that memory range. > > > Please could you give an example of what could happen? > > Technically device lost a protection of its memory region against any > other driver, but I am not sure if this is real threat in practical life. > Also this is same in uio_pci_generic, it doesn't reserve the memory. OK thanks for the explanations. So we are not sure how this memory region can be stolen and we assume it won't. > > This patch fixes a problem with recent kernels (not mentioned above) > > which offer the uio_pci_generic alternative. > > Will give kernel version information. > > > That's why I think we should fix it only if there is absolutely no > > regression for older kernels. > > Totally agreed, that is why I expressed my concern, let this patch hang > around a little. It may be valuable to have in 16.07. I suggest to wait RC3 (mid-July) to integrate it. We will have a RC4 to test it.
diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c index 45a5720..df41e45 100644 --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c @@ -342,16 +342,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) goto fail_free; } - /* - * reserve device's PCI memory regions for use by this - * module - */ - err = pci_request_regions(dev, "igb_uio"); - if (err != 0) { - dev_err(&dev->dev, "Cannot request regions\n"); - goto fail_disable; - } - /* enable bus mastering on the device */ pci_set_master(dev); @@ -441,8 +431,6 @@ fail_release_iomem: igbuio_pci_release_iomem(&udev->info); if (udev->mode == RTE_INTR_MODE_MSIX) pci_disable_msix(udev->pdev); - pci_release_regions(dev); -fail_disable: pci_disable_device(dev); fail_free: kfree(udev); @@ -460,7 +448,6 @@ igbuio_pci_remove(struct pci_dev *dev) igbuio_pci_release_iomem(&udev->info); if (udev->mode == RTE_INTR_MODE_MSIX) pci_disable_msix(dev); - pci_release_regions(dev); pci_disable_device(dev); pci_set_drvdata(dev, NULL); kfree(udev);
With kernels enabled CONFIG_IO_STRICT_DEVMEM option mmap the iomem area to userspace fails: EAL: pci_map_resource(): cannot mmap(39, 0x7f1c51800000, 0x100000, 0x0): Invalid argument (0xffffffffffffffff) As a workaround igb_uio can stop reserving PCI memory resources, from kernel point of view io-memory region looks like idle and mmap works again. With this update device io-memory range is not protected against any other kernel driver claim ownership on those resources, which shouldn't be a problem for dpdk usage module. Fixes: af75078fece3 ("first public release") Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> --- lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 13 ------------- 1 file changed, 13 deletions(-)