Re: UIO: missing resource mapping
Hello Hans Am Donnerstag, den 19.07.2012, 01:47 +0200 schrieb Hans J. Koch: > You'll hear from me soon, thanks for your work! Comments and reviews > from others are welcome... Is there any progress on this topic? -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 69 79844114 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UIO: missing resource mapping
Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> and post it for review.
>
Here we go ...
>
Signed-off-by: Dominic Eschweiler
diff --git a/drivers/uio/uio_pci_generic.c
b/drivers/uio/uio_pci_generic.c
index 0bd08ef..e25991e 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -25,10 +25,12 @@
#include
#include
-#define DRIVER_VERSION "0.01.0"
+#define DRIVER_VERSION "0.02.0"
#define DRIVER_AUTHOR "Michael S. Tsirkin "
#define DRIVER_DESC"Generic UIO driver for PCI 2.3 devices"
+#define DRV_NAME "uio_pci_generic"
+
struct uio_pci_generic_dev {
struct uio_info info;
struct pci_dev *pdev;
@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
{
struct uio_pci_generic_dev *gdev;
int err;
+ int i;
err = pci_enable_device(pdev);
if (err) {
@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
}
if (!pdev->irq) {
- dev_warn(&pdev->dev, "No IRQ assigned to device: "
-"no support for interrupts?\n");
+ dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
interrupts?\n");
pci_disable_device(pdev);
return -ENODEV;
}
@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
gdev->info.handler = irqhandler;
gdev->pdev = pdev;
+ /* request regions */
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+ return err;
+ }
+
+ /* create attributes for BAR mappings */
+ for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+ if (pdev->resource[i].flags &&
+ (pdev->resource[i].flags & IORESOURCE_MEM)) {
+ gdev->info.mem[i].addr = pci_resource_start(pdev, i);
+ gdev->info.mem[i].size = pci_resource_len(pdev, i);
+ gdev->info.mem[i].internal_addr = NULL;
+ gdev->info.mem[i].memtype = UIO_MEM_PHYS;
+ }
+ }
+
if (uio_register_device(&pdev->dev, &gdev->info))
goto err_register;
pci_set_drvdata(pdev, gdev);
+ pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
+ pdev->vendor, pdev->device);
+
return 0;
err_register:
kfree(gdev);
@@ -107,17 +130,21 @@ err_verify:
static void remove(struct pci_dev *pdev)
{
struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
uio_unregister_device(&gdev->info);
+
+ pci_release_regions(pdev);
pci_disable_device(pdev);
kfree(gdev);
+
+ pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
+ pdev->vendor, pdev->device);
}
static struct pci_driver driver = {
- .name = "uio_pci_generic",
+ .name = DRV_NAME,
.id_table = NULL, /* only dynamic id's */
- .probe = probe,
- .remove = remove,
+ .probe= probe,
+ .remove = remove,
};
static int __init init(void)
--
Gruß
Dominic
Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany
Phone: +49 69 79844114
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UIO: missing resource mapping
Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin: > > UIO has the same property, doesn't it? Multiple users can > access device memory through sysfs. Indeed, that's a similar problem. I haven't tried it (yet), but this particular problem can maybe circumvented by using mmap with the MAP_PRIVATE flag. Doing so is the responsibility of the driver programmer (like Hans already said). Even if that mmap trick does not work, it is pretty much sure that a BAR is already used by another program, if a related kernel driver is loaded. In that case the kernel has a chance to avoid such BAR race conditions by not giving the possibility to map them to the userspace. Nevertheless, I'm pretty sure that the possibility via sysfs to access BARs, which are already managed by a kernel driver, opens the door for denial of service attacks. On the other hand, I'm quite a newbie on this topic and maybe I don't see the big picture here. Therefore it is up to you guys to make the right decision (if needed). -- Gruß Dominic -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UIO: missing resource mapping
Am Freitag, den 13.07.2012, 16:18 +0200 schrieb Hans J. Koch: > If somebody maps the card's memory through the UIO driver and somebody > else also maps it using sysfs, that is possible. What happens if both > write to the same hardware registers is a different topic. Writing a > driver implies some responsibility, even if it's done in userspace. That's true and I would not complain. I'm just saying that there is the possibility to disturb a kernel driver by mapping memory etc. I think it is worth to considerate moving the BAR mapping code to UIO. Especially, when I look at the discussions about what effects mappable DMA memory can have on the system security. -- Gruß Dominic -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UIO: missing resource mapping
Am Freitag, den 13.07.2012, 16:22 +0300 schrieb Michael S. Tsirkin: > Could you give an example of the problem? How do you bind > both UIO and another driver to the same device? Sorry, I'm looking on it from the user-space perspective. Maybe I'm wrong, but I can give you an example : lspci -v ... 03:00.0 Network controller: Broadcom Corporation BCM4331 802.11a/b/g/n (rev 02) Subsystem: Apple Inc. AirPort Extreme Flags: bus master, fast devsel, latency 0, IRQ 17 Memory at b060 (64-bit, non-prefetchable) [size=16K] Capabilities: [40] Power Management version 3 Capabilities: [58] Vendor Specific Information: Len=78 Capabilities: [48] MSI: Enable- Count=1/1 Maskable- 64bit+ Capabilities: [d0] Express Endpoint, MSI 00 Capabilities: [100] Advanced Error Reporting Capabilities: [13c] Virtual Channel Capabilities: [160] Device Serial Number 85-f2-6d-ff-ff-42-68-a8 Capabilities: [16c] Power Budgeting Kernel driver in use: bcma-pci-bridge ... This Device has one 64 Bit Bar. When I look at the related sysfs entry ... ls /sys/bus/pci/devices/:03:00.0 ... --w--- 1 root root 4.0K Jul 13 16:35 reset -r--r--r-- 1 root root 4.0K Jul 12 21:43 resource -rw--- 1 root root 16K Jul 13 16:35 resource0 lrwxrwxrwx 1 root root0 Jul 12 23:41 subsystem -> ../../../../bus/pci ... ... I can see that it should be possible to map resource0 and directly write into a BAR which is already managed by a kernel drivers. Moving this functionality to UIO would only generate those resource files, if the device is handled by UIO and therefore intended to be managed from the user-space. -- Gruß Dominic -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UIO: missing resource mapping
Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin: > My concern was people will ask for more and more stuff that pci > sysfs already has. > If we do add these is there a way to not duplicate code from pci? I have some concerns about the placing for the BAR mapping code inside the kernel. The point is, that sysfs currently makes it possible to map BARs of all card which are handled by any driver. This is fine in case of UIO, because it is intended that a user-space program maps BARs, but it is also possible to map BARs that are already handle by a kernel driver. It i therefore possible to jam the system by confusing sysfs entries. I don't know which implications this has, but I would move the BAR mapping capabilities completely to UIO. This should ensure that only BARs can be mapped, which are handled by UIO and no other kernel-space driver. -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 69 79844114 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uio_pci_generic does not export memory resources
Am Freitag, den 08.06.2012, 18:37 +0200 schrieb Hans J. Koch: > It does, the documentation is just not very explicit when it comes to > memory mapping. OK, quoting the other mail: Am Freitag, den 08.06.2012, 19:39 +0300 schrieb Michael S. Tsirkin: > Documentation says this: > > Userspace driver can use pci sysfs interface, or the > libpci libray that wraps it, to talk to the device and to > re-enable interrupts by writing to the command register. I think should ditch the discussion about the patch and I'm going to revise the documentation? I read this part a couple of times and allways thought the "sysfs interface" refers to the UIO sysfs entry. Therefore, would it be OK if I send a patch for the documentation next week? -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 (0) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uio_pci_generic does not export memory resources
Am Freitag, den 08.06.2012, 17:57 +0200 schrieb Hans J. Koch: > What problem do you have with this approach? Nothing itself. There is obviously a documentation issue, which really confuses people who are new to that stuff. I thought that fixing it, by just adding a view lines of code (really a view, when I remove the DMA masque part) might enhance this particular module. Anyway, what is wrong when the uio_pci_generic module works like described in the related documentation? -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 (0) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uio_pci_generic does not export memory resources
Am Freitag, den 08.06.2012, 17:18 +0200 schrieb Hans J. Koch: > Then there's something fundamentally wrong in your driver. Check the > return value of uio_register_device(). > I'm talking about the uio_pci_generic module, not about UIO in general. > > I was very confused when I tried UIO the first time > > and it did not behave like it is described in the documentation. > > UIO is the mainline since 2007, and I can assure you it works like > described. Lots of people use it. > I know and I really don't have a problem with UIO itself. The uio_pci_generic module does not export correctly, maybe you take a look on its code? My guess is, that the uio_pci_generic module isn't commonly used and therefore the problem wasn't reported before. -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 (0) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uio_pci_generic does not export memory resources
Am Freitag, den 08.06.2012, 08:16 -0600 schrieb Alex Williamson: > Yes, thanks Jan. This is exactly what VFIO does. VFIO provides > secure config space access, resource access, DMA mapping services, and > full interrupt support to userspace. I know about VFIO, but we need some support for that stuff relatively soon. That's the reason why I'm currently working on it to make UIO DMA capable. My extensions probably do not play well with IOMMUs and they therefore won't make it to mainline anyhow (i learned that today ;-). > I'm currently working to get this upstream, probably targeting 3.6 at > this point, and would love to have more users to help make that > happen. Yes, you are faster than I'm able to ask. 3.6 is a good target and really would like to support that. > Please take a look at the vfio-3.4 branch in the tree above. See this > tree for Qemu's usage of VFIO for device assignment ... OK, I take look at it and report back (off the list). Please write me an email if you need support. -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 69 79844114 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uio_pci_generic does not export memory resources
Am Freitag, den 08.06.2012, 16:03 +0300 schrieb Michael S. Tsirkin: > Why is this needed? > What's wrong with mapping resources through > /sys/bus/pci/devices/XresourceX > ? > Mmmh ok, the problem here is, that the UIO documentation states: "/dev/uioX is used to access the address space of the card. Just use mmap() to access registers or RAM locations of your card." and "From userspace, the different mappings are distinguished by adjusting the offset parameter of the mmap() call." This does not work and the "/sys/class/uio/uioX/maps/mapX/" directories do also not appear. I was very confused when I tried UIO the first time and it did not behave like it is described in the documentation. Anyway, the patch adds the described behaviour and I would more than happy, if it would be accepted for the mainline kernel. > > uio currently only supports devices which do not > do DMA. > > DMA from uio controlled devices is a no no unless > it's behind an IOMMU which can protect us from > random memory corruptions this could cause. > > In the later case it's OK but we need some code > to check this and program the IOMMU appropriately. OK, I see. Might it be OK if I just remove the DMA stuff from this patch and resubmit it? -- Gruß Dominic Frankfurt Institute for Advanced Studies (FIAS) Ruth-Moufang-Straße 1 D-60438 Frankfurt am Main Germany Phone: +49 69 79844114 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] uio_pci_generic does not export memory resources
Hello,
the current version of the uio_pci_generic module does not export memory
resources, such as BARs. As far as I can see, the related module does
only support interrupts, which is not really useful. My suggestion in
this case would be to either fix this issue, or to completely remove it.
The latter would not be an option for us, since we need this
functionality at some stuff at CERN.
Therefore, here is a patch that fixes the issue. Please give me further
advice, since I'm doing this the first time ...
Signed-off-by: Dominic Eschweiler
---
diff -uNr linux-3.4_old/drivers/uio/uio_pci_generic.c
linux-3.4_new/drivers/uio/uio_pci_generic.c
--- linux-3.4_old/drivers/uio/uio_pci_generic.c 2012-05-21
00:29:13.0 +0200
+++ linux-3.4_new/drivers/uio/uio_pci_generic.c 2012-06-08
13:01:12.0 +0200
@@ -25,10 +25,12 @@
#include
#include
-#define DRIVER_VERSION "0.01.0"
+#define DRIVER_VERSION "0.02.0"
#define DRIVER_AUTHOR "Michael S. Tsirkin "
#define DRIVER_DESC"Generic UIO driver for PCI 2.3 devices"
+#define DRV_NAME "uio_pci_generic"
+
struct uio_pci_generic_dev {
struct uio_info info;
struct pci_dev *pdev;
@@ -58,6 +60,7 @@
{
struct uio_pci_generic_dev *gdev;
int err;
+ int i;
err = pci_enable_device(pdev);
if (err) {
@@ -66,9 +69,33 @@
return err;
}
+/* set master */
+ pci_set_master(pdev);
+
+/* set DMA mask */
+ err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
+ if (err) {
+ dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA
mask.\n");
+ err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+ if (err) {
+ dev_err(&pdev->dev, "Can't set PCI DMA mask,
aborting\n");
+ return -ENODEV;
+ }
+ }
+
+ /* set consistent DMA mask */
+ err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
+ if (err) {
+ dev_warn(&pdev->dev, "Warning: couldn't set 64-bit consistent
PCI DMA
mask.\n");
+ err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
+ if (err) {
+ dev_err(&pdev->dev, "Can't set consistent PCI DMA mask,
aborting
\n");
+ return -ENODEV;
+ }
+ }
+
if (!pdev->irq) {
- dev_warn(&pdev->dev, "No IRQ assigned to device: "
-"no support for interrupts?\n");
+ dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
interrupts?\n");
pci_disable_device(pdev);
return -ENODEV;
}
@@ -91,10 +118,40 @@
gdev->info.handler = irqhandler;
gdev->pdev = pdev;
+ /* request regions */
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+ return err;
+ }
+
+ /* create attributes for BAR mappings */
+ for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+ if (pdev->resource[i].flags &&
+ (pdev->resource[i].flags & IORESOURCE_IO)) {
+ gdev->info.port[i].size = 0;
+ gdev->info.port[i].porttype = UIO_PORT_OTHER;
+ #ifdef CONFIG_X86
+ gdev->info.port[i].porttype = UIO_PORT_X86;
+ #endif
+ }
+
+ if (pdev->resource[i].flags &&
+ (pdev->resource[i].flags & IORESOURCE_MEM)) {
+ gdev->info.mem[i].addr = pci_resource_start(pdev, i);
+ gdev->info.mem[i].size = pci_resource_len(pdev, i);
+ gdev->info.mem[i].internal_addr = NULL;
+ gdev->info.mem[i].memtype = UIO_MEM_PHYS;
+ }
+ }
+
if (uio_register_device(&pdev->dev, &gdev->info))
goto err_register;
pci_set_drvdata(pdev, gdev);
+ pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
+ pdev->vendor, pdev->device);
+
return 0;
err_register:
kfree(gdev);
@@ -107,17 +164,21 @@
static void remove(struct pci_dev *pdev)
{
struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
uio_unregister_device(&gdev->info);
+
+ pci_release_regions(pdev);
pci_disable_device(pdev);
kfree(gdev);
+
+ pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
+ pdev->vendor, pdev->device);
}
static struct pci_driver driver = {
- .name = "uio_pci_generic",
+ .name = DRV_NAME,
.id_ta
