[PATCH] uio_pci_generic does not export memory resources

[PATCH] uio_pci_generic does not export memory resources

[Dominic Eschweiler]

[-- Attachment #1: Type: text/plain, Size: 4419 bytes --]

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 <eschweiler@fias.uni-frankfurt.de>
---
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.000000000 +0200
+++ linux-3.4_new/drivers/uio/uio_pci_generic.c	2012-06-08
13:01:12.000000000 +0200
@@ -25,10 +25,12 @@
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
 
-#define DRIVER_VERSION	"0.01.0"
+#define DRIVER_VERSION	"0.02.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
 #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_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

[-- Attachment #2: uio_pci_generic_0.02.0.diff --]
[-- Type: text/x-patch, Size: 3587 bytes --]

Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
---
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.000000000 +0200
+++ linux-3.4_new/drivers/uio/uio_pci_generic.c	2012-06-08 13:01:12.000000000 +0200
@@ -25,10 +25,12 @@
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
 
-#define DRIVER_VERSION	"0.01.0"
+#define DRIVER_VERSION	"0.02.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
 #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_table = NULL, /* only dynamic id's */
-	.probe = probe,
-	.remove = remove,
+	.probe    = probe,
+	.remove   = remove,
 };
 
 static int __init init(void)

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Fri, Jun 08, 2012 at 01:56:56PM +0200, Dominic Eschweiler wrote:
> 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 ...

Why is this needed?
What's wrong with mapping resources through
 /sys/bus/pci/devices/XXXXXresourceX
?

> 
> Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
> ---
> 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.000000000 +0200
> +++ linux-3.4_new/drivers/uio/uio_pci_generic.c	2012-06-08
> 13:01:12.000000000 +0200
> @@ -25,10 +25,12 @@
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
>  
> -#define DRIVER_VERSION	"0.01.0"
> +#define DRIVER_VERSION	"0.02.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>  #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));

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.

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

Re: [PATCH] uio_pci_generic does not export memory resources

[Jan Kiszka]

On 2012-06-08 15:03, Michael S. Tsirkin wrote:
>> +    /* set master */
>> +	pci_set_master(pdev);
>> +
>> +    /* set DMA mask */
>> +	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> 
> 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.

AKA: VFIO. :)

Dominic, maybe you want to have a look at Alex's work:
https://github.com/awilliam/linux-vfio

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Fri, 2012-06-08 at 15:16 +0200, Jan Kiszka wrote:
> On 2012-06-08 15:03, Michael S. Tsirkin wrote:
> >> +    /* set master */
> >> +	pci_set_master(pdev);
> >> +
> >> +    /* set DMA mask */
> >> +	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> > 
> > 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.
> 
> AKA: VFIO. :)
> 
> Dominic, maybe you want to have a look at Alex's work:
> https://github.com/awilliam/linux-vfio

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'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.  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:
https://github.com/awilliam/qemu-vfio (vfio-ng)

Look forward to your feedback.  Thanks,

Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Dominic Eschweiler]

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

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Fri, 2012-06-08 at 16:47 +0200, Dominic Eschweiler wrote:
> 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 ;-).

Yeah, VFIO was originally started by Tom Lyon as a uio extension to
support DMA mapping and more extensive interrupt handling, but it became
an entire re-write of uio.  Now we've had to extend in to iommu grouping
to get the DMA protection that we're really after.

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

Thanks,
Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Andreas Hartmann]

Hi Dominic,

Dominic Eschweiler wrote:
> 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 not sure if vfio covers your needs completely, but I tested it here
very successfully. I was able to create a patch, which can be applied to
opensuse 3.4.1 kernel and which seams to run well.

I even managed to integrate it into libvirt :-). So it is usable as
every other traditional VM, too.

Besides the problem with AMD IOMMU, which requires to unbind a whole
group of devices in some cases (PCI passthrough - not PCIe), it's really
cool! And it's usable now!


Regards,
Andreas

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Fri, 2012-06-08 at 18:16 +0200, Andreas Hartmann wrote:
> Hi Dominic,
> 
> Dominic Eschweiler wrote:
> > 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 not sure if vfio covers your needs completely, but I tested it here
> very successfully. I was able to create a patch, which can be applied to
> opensuse 3.4.1 kernel and which seams to run well.
> 
> I even managed to integrate it into libvirt :-). So it is usable as
> every other traditional VM, too.

Oh, please post the patch for libvirt!

> Besides the problem with AMD IOMMU, which requires to unbind a whole
> group of devices in some cases (PCI passthrough - not PCIe), it's really
> cool! And it's usable now!

If you're feeling adventurous (and know that this may not make it
upstream), you can do something like this:

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ab0dba0..5c26804 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3161,11 +3161,22 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
        return pci_dev_get(dev);
 }
 
+static int pci_func_supports_acs(struct pci_dev *dev, u16 acs_flags)
+{
+       return 1;
+}
+
 static const struct pci_dev_acs_enabled {
        u16 vendor;
        u16 device;
        int (*acs_enabled)(struct pci_dev *dev, u16 acs_flags);
 } pci_dev_acs_enabled[] = {
+       { 0x1002, 0x4385, pci_func_supports_acs },
+       { 0x1002, 0x439c, pci_func_supports_acs },
+       { 0x1002, 0x4383, pci_func_supports_acs },
+       { 0x1002, 0x439d, pci_func_supports_acs },
+       { 0x1002, 0x4384, pci_func_supports_acs },
+       { 0x1002, 0x4399, pci_func_supports_acs },
        { 0 }
 };

Hmm, I wonder if we should make a kernel boot parameter that allows
whitelisting some devices.  I think it would have to taint the kernel
but there's probably sufficient interest for usability vs
supportability.  Thanks,

Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Andreas Hartmann]

Alex Williamson wrote:
> On Fri, 2012-06-08 at 18:16 +0200, Andreas Hartmann wrote:
[...]
>> Besides the problem with AMD IOMMU, which requires to unbind a whole
>> group of devices in some cases (PCI passthrough - not PCIe), it's really
>> cool! And it's usable now!
> 
> If you're feeling adventurous (and know that this may not make it
> upstream), you can do something like this:

Hmm, even without this patch, it's not necessary to bind all devices to
pci-stub. Just to remember:

-[0000:00]-+-14.0  Advanced Micro Devices [AMD] nee ATI SBx00 SMBus Controller                                                                                                     
           +-14.1  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 IDE Controller                                                                                           
           +-14.2  Advanced Micro Devices [AMD] nee ATI SBx00 Azalia (Intel HDA)                                                                                                   
           +-14.3  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 LPC host controller                                                                                      
           +-14.4-[06]--                                                                                                                                                           
           \-14.5  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 USB OHCI2 Controller

Device 06:07.0 should be passed through.

The devices 14.0, 14.3 and 14.4 need not to be bound to pci-stub - the
VM starts (and seems to work fine) anyway. Maybe because there isn't
any driver anyway for these devices?

The USB device seems to be an internal device only - all of my external
USB jacks are working fine.

The only thing I'm missing is the sound device.

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index ab0dba0..5c26804 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3161,11 +3161,22 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
>         return pci_dev_get(dev);
>  }
>  
> +static int pci_func_supports_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +       return 1;
> +}
> +
>  static const struct pci_dev_acs_enabled {
>         u16 vendor;
>         u16 device;
>         int (*acs_enabled)(struct pci_dev *dev, u16 acs_flags);
>  } pci_dev_acs_enabled[] = {
> +       { 0x1002, 0x4385, pci_func_supports_acs },
> +       { 0x1002, 0x439c, pci_func_supports_acs },
> +       { 0x1002, 0x4383, pci_func_supports_acs },
> +       { 0x1002, 0x439d, pci_func_supports_acs },
> +       { 0x1002, 0x4384, pci_func_supports_acs },
> +       { 0x1002, 0x4399, pci_func_supports_acs },
>         { 0 }
>  };

What's the risk of this patch? Machine crash? Data loss for an active
file in an application? Complete filesystem damage? The latter would be
worse.

> Hmm, I wonder if we should make a kernel boot parameter that allows
> whitelisting some devices.  I think it would have to taint the kernel
> but there's probably sufficient interest for usability vs
> supportability.

Good idea. I would print an additional big fat warning of dataloss /
filesystem damage / crash if this could be the case.


Thanks,
regards,
Andreas

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Sat, 2012-06-09 at 11:28 +0200, Andreas Hartmann wrote:
> Alex Williamson wrote:
> > On Fri, 2012-06-08 at 18:16 +0200, Andreas Hartmann wrote:
> [...]
> >> Besides the problem with AMD IOMMU, which requires to unbind a whole
> >> group of devices in some cases (PCI passthrough - not PCIe), it's really
> >> cool! And it's usable now!
> > 
> > If you're feeling adventurous (and know that this may not make it
> > upstream), you can do something like this:
> 
> Hmm, even without this patch, it's not necessary to bind all devices to
> pci-stub. Just to remember:
> 
> -[0000:00]-+-14.0  Advanced Micro Devices [AMD] nee ATI SBx00 SMBus Controller                                                                                                     
>            +-14.1  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 IDE Controller                                                                                           
>            +-14.2  Advanced Micro Devices [AMD] nee ATI SBx00 Azalia (Intel HDA)                                                                                                   
>            +-14.3  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 LPC host controller                                                                                      
>            +-14.4-[06]--                                                                                                                                                           
>            \-14.5  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 USB OHCI2 Controller
> 
> Device 06:07.0 should be passed through.
> 
> The devices 14.0, 14.3 and 14.4 need not to be bound to pci-stub - the
> VM starts (and seems to work fine) anyway. Maybe because there isn't
> any driver anyway for these devices?

Right, the actual rule is that any device you want to access needs to be
bound to vfio-pci.  The remaining devices within the group can either be
bound to vfio-pci or pci-stub or no driver.  The recommendation for
avoiding that no-driver case is because if later a driver is loaded into
the kernel that would attempt to bind to any of those no-driver devices,
you end up back in the situation of the security of the group being
violated and hit the BUGON you saw previously.

> The USB device seems to be an internal device only - all of my external
> USB jacks are working fine.
> 
> The only thing I'm missing is the sound device.
> 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index ab0dba0..5c26804 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3161,11 +3161,22 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
> >         return pci_dev_get(dev);
> >  }
> >  
> > +static int pci_func_supports_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +       return 1;
> > +}
> > +
> >  static const struct pci_dev_acs_enabled {
> >         u16 vendor;
> >         u16 device;
> >         int (*acs_enabled)(struct pci_dev *dev, u16 acs_flags);
> >  } pci_dev_acs_enabled[] = {
> > +       { 0x1002, 0x4385, pci_func_supports_acs },
> > +       { 0x1002, 0x439c, pci_func_supports_acs },
> > +       { 0x1002, 0x4383, pci_func_supports_acs },
> > +       { 0x1002, 0x439d, pci_func_supports_acs },
> > +       { 0x1002, 0x4384, pci_func_supports_acs },
> > +       { 0x1002, 0x4399, pci_func_supports_acs },
> >         { 0 }
> >  };
> 
> What's the risk of this patch? Machine crash? Data loss for an active
> file in an application? Complete filesystem damage? The latter would be
> worse.

What we're trying to prevent by testing whether devices support ACS is
peer-to-peer transactions that would not be translated via the IOMMU.
For instance, imagine if your WLAN controller behind 14.4 does a DMA
write to an address that happens to be within the MMIO resources of
device 14.2, the audio device.  Instead of the transaction going up to
the IOMMU and resulting in a memory write, internal routing on device
14.x results in that transaction being redirected to the 14.2.  So
you're looking at potential data loss from the guest as well as
corrupting device state in the host.

> > Hmm, I wonder if we should make a kernel boot parameter that allows
> > whitelisting some devices.  I think it would have to taint the kernel
> > but there's probably sufficient interest for usability vs
> > supportability.
> 
> Good idea. I would print an additional big fat warning of dataloss /
> filesystem damage / crash if this could be the case.

Well, outlining the risk above makes me a little more nervous about
making such a config option, even if it taints the kernel, available so
easily... :^\  Thanks,

Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Andreas Hartmann]

Alex Williamson wrote:
> On Sat, 2012-06-09 at 11:28 +0200, Andreas Hartmann wrote:

[...]

>> What's the risk of this patch? Machine crash? Data loss for an active
>> file in an application? Complete filesystem damage? The latter would be
>> worse.
> 
> What we're trying to prevent by testing whether devices support ACS is
> peer-to-peer transactions that would not be translated via the IOMMU.
> For instance, imagine if your WLAN controller behind 14.4 does a DMA
> write to an address that happens to be within the MMIO resources of
> device 14.2, the audio device. 

Why should it do that intentionally - I can't see any reason. Remains
unintentionally. But that's already enough :-) .

> Instead of the transaction going up to
> the IOMMU and resulting in a memory write, internal routing on device
> 14.x results in that transaction being redirected to the 14.2.  So
> you're looking at potential data loss from the guest as well as
> corrupting device state in the host.

My guest does have no data. Besides that, the VM can be easily backuped
before.
The corrupting device state probably is limited to the devices behind
the bridge. Correct?

>>> Hmm, I wonder if we should make a kernel boot parameter that allows
>>> whitelisting some devices.  I think it would have to taint the kernel
>>> but there's probably sufficient interest for usability vs
>>> supportability.
>>
>> Good idea. I would print an additional big fat warning of dataloss /
>> filesystem damage / crash if this could be the case.
> 
> Well, outlining the risk above makes me a little more nervous about
> making such a config option, even if it taints the kernel, available so
> easily... :^\ 

That's true, too. Anyway, out of curiosity, if the corruption of the
device states is limited to the devices behind the bridge, I'm going to
test it. I hope that a hardware damage isn't possible by that action.


Thanks,
regards,
Andreas

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Sat, 2012-06-09 at 18:25 +0200, Andreas Hartmann wrote:
> Alex Williamson wrote:
> > On Sat, 2012-06-09 at 11:28 +0200, Andreas Hartmann wrote:
> 
> [...]
> 
> >> What's the risk of this patch? Machine crash? Data loss for an active
> >> file in an application? Complete filesystem damage? The latter would be
> >> worse.
> > 
> > What we're trying to prevent by testing whether devices support ACS is
> > peer-to-peer transactions that would not be translated via the IOMMU.
> > For instance, imagine if your WLAN controller behind 14.4 does a DMA
> > write to an address that happens to be within the MMIO resources of
> > device 14.2, the audio device. 
> 
> Why should it do that intentionally - I can't see any reason. Remains
> unintentionally. But that's already enough :-) .

We don't know the internal design of multifunction devices.

> > Instead of the transaction going up to
> > the IOMMU and resulting in a memory write, internal routing on device
> > 14.x results in that transaction being redirected to the 14.2.  So
> > you're looking at potential data loss from the guest as well as
> > corrupting device state in the host.
> 
> My guest does have no data. Besides that, the VM can be easily backuped
> before.
> The corrupting device state probably is limited to the devices behind
> the bridge. Correct?

No, we're talking about the multifunction device here.  A legacy PCI bus
is always susceptible to this as it's a shared bus.  Another device on
the bus can claim the transaction.  The IOMMU also only has visibility
to the bridge devices, all devices behind it are masked.  So the
difference we're looking at is whether 14.4 is grouped with all the
other devices on 14.x or not.  Bus 6 will always be grouped with device
14.4.  So then you have to consider what can happen if your guest that
owns 14.4 and bus 6 can corrupt the state of device 14.2 and how that
corrupted state could be propagated out to the host.

> >>> Hmm, I wonder if we should make a kernel boot parameter that allows
> >>> whitelisting some devices.  I think it would have to taint the kernel
> >>> but there's probably sufficient interest for usability vs
> >>> supportability.
> >>
> >> Good idea. I would print an additional big fat warning of dataloss /
> >> filesystem damage / crash if this could be the case.
> > 
> > Well, outlining the risk above makes me a little more nervous about
> > making such a config option, even if it taints the kernel, available so
> > easily... :^\ 
> 
> That's true, too. Anyway, out of curiosity, if the corruption of the
> device states is limited to the devices behind the bridge, I'm going to
> test it. I hope that a hardware damage isn't possible by that action.

Hardware damage should not be possible, but the interactions would be
between host and guest as noted above.  Thanks,

Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Andreas Hartmann]

Alex Williamson wrote:
> On Sat, 2012-06-09 at 18:25 +0200, Andreas Hartmann wrote:
>> Alex Williamson wrote:
>>> On Sat, 2012-06-09 at 11:28 +0200, Andreas Hartmann wrote:
>>
>> [...]
>>
>>>> What's the risk of this patch? Machine crash? Data loss for an active
>>>> file in an application? Complete filesystem damage? The latter would be
>>>> worse.
>>>
>>> What we're trying to prevent by testing whether devices support ACS is
>>> peer-to-peer transactions that would not be translated via the IOMMU.
>>> For instance, imagine if your WLAN controller behind 14.4 does a DMA
>>> write to an address that happens to be within the MMIO resources of
>>> device 14.2, the audio device. 
>>
>> Why should it do that intentionally - I can't see any reason. Remains
>> unintentionally. But that's already enough :-) .
> 
> We don't know the internal design of multifunction devices.

_peer-to-peer transaction_

My current idea: this transaction is done without the knowledge of the
system software at all.
Couldn't this happen too, even if there is no VM at all but bare metal?
What would be the result in this case?

>>> Instead of the transaction going up to
>>> the IOMMU and resulting in a memory write, internal routing on device
>>> 14.x results in that transaction being redirected to the 14.2.  So
>>> you're looking at potential data loss from the guest as well as
>>> corrupting device state in the host.
>>
>> My guest does have no data. Besides that, the VM can be easily backuped
>> before.
>> The corrupting device state probably is limited to the devices behind
>> the bridge. Correct?
> 
> No, we're talking about the multifunction device here.  A legacy PCI bus
> is always susceptible to this as it's a shared bus.  Another device on
> the bus can claim the transaction.  The IOMMU also only has visibility
> to the bridge devices, all devices behind it are masked.  So the
> difference we're looking at is whether 14.4 is grouped with all the
> other devices on 14.x or not.  Bus 6 will always be grouped with device
> 14.4.  So then you have to consider what can happen if your guest that
> owns 14.4 and bus 6 can corrupt the state of device 14.2 and how that
> corrupted state could be propagated out to the host.

To make it short: I applied the patch as proposed. I disabled binding /
unbinding of 14.2 (Sound), started the VM, started hostapd in the VM
(rt2860pci is set to do no hw crypt) and started a netperf session
sending data from a client via AP to host and vice versa. At the same
time, I started some sound device interaction (only output) onto the host.

If there are any problems, I would have expected:
- Encryption / decryption of WLAN data would have failed. I couldn't
  see any errors / warnings at this side. The 4 and 2 way key exchanges
  worked without any problem. EAP-TLS authentication has been working
  fine, too, apart from the already known problems.
- Distortions on sound output: I couldn't hear any.
- Some other malfunction, e.g. on graphics output (fglrx). I couldn't
  see any problem.

These are tests from the point of the view of an user. Do you have any
idea to intentionally activate a failure situation?


Kind regards,
Andreas

Re: [PATCH] uio_pci_generic does not export memory resources

[Andreas Hartmann]

Andreas Hartmann wrote:
> Alex Williamson wrote:
>> On Sat, 2012-06-09 at 18:25 +0200, Andreas Hartmann wrote:
>>> Alex Williamson wrote:
>>>> On Sat, 2012-06-09 at 11:28 +0200, Andreas Hartmann wrote:
>>>
>>> [...]
>>>
>>>>> What's the risk of this patch? Machine crash? Data loss for an active
>>>>> file in an application? Complete filesystem damage? The latter would be
>>>>> worse.
>>>>
>>>> What we're trying to prevent by testing whether devices support ACS is
>>>> peer-to-peer transactions that would not be translated via the IOMMU.
>>>> For instance, imagine if your WLAN controller behind 14.4 does a DMA
>>>> write to an address that happens to be within the MMIO resources of
>>>> device 14.2, the audio device. 
>>>
>>> Why should it do that intentionally - I can't see any reason. Remains
>>> unintentionally. But that's already enough :-) .
>>
>> We don't know the internal design of multifunction devices.
> 
> _peer-to-peer transaction_
> 
> My current idea: this transaction is done without the knowledge of the
> system software at all.
> Couldn't this happen too, even if there is no VM at all but bare metal?
> What would be the result in this case?
> 
>>>> Instead of the transaction going up to
>>>> the IOMMU and resulting in a memory write, internal routing on device
>>>> 14.x results in that transaction being redirected to the 14.2.  So
>>>> you're looking at potential data loss from the guest as well as
>>>> corrupting device state in the host.
>>>
>>> My guest does have no data. Besides that, the VM can be easily backuped
>>> before.
>>> The corrupting device state probably is limited to the devices behind
>>> the bridge. Correct?
>>
>> No, we're talking about the multifunction device here.  A legacy PCI bus
>> is always susceptible to this as it's a shared bus.  Another device on
>> the bus can claim the transaction.  The IOMMU also only has visibility
>> to the bridge devices, all devices behind it are masked.  So the
>> difference we're looking at is whether 14.4 is grouped with all the
>> other devices on 14.x or not.  Bus 6 will always be grouped with device
>> 14.4.  So then you have to consider what can happen if your guest that
>> owns 14.4 and bus 6 can corrupt the state of device 14.2 and how that
>> corrupted state could be propagated out to the host.
> 
> To make it short: I applied the patch as proposed. I disabled binding /
> unbinding of 14.2 (Sound), started the VM, started hostapd in the VM
> (rt2860pci is set to do no hw crypt) and started a netperf session
> sending data from a client via AP to host and vice versa. At the same
> time, I started some sound device interaction (only output) onto the host.
> 
> If there are any problems, I would have expected:
> - Encryption / decryption of WLAN data would have failed. I couldn't
>   see any errors / warnings at this side. The 4 and 2 way key exchanges
>   worked without any problem. EAP-TLS authentication has been working
>   fine, too, apart from the already known problems.
> - Distortions on sound output: I couldn't hear any.
> - Some other malfunction, e.g. on graphics output (fglrx). I couldn't
>   see any problem.
> 
> These are tests from the point of the view of an user. Do you have any
> idea to intentionally activate a failure situation?

I did one more test:

- Set up a sound server (pulseaudio) on the host using the sound device
  14.2 for output.
- Configure the WLAN-client to use this sound device as output device.
- The VM is the AP again, configured with rt2800pci hw encryption on or
  off


Result:
Again, I didn't face any problem (until now). The machine just worked as
expected. Even after a s2ram/resume cycle (all VM's have been shut down
before).

As I don't need the USB or IDE device, the ISA-bridge or the
SMBus-Controller at all here, the patch should be reducible to

+       { 0x1002, 0x4383, pci_func_supports_acs }, // Sound
+       { 0x1002, 0x4384, pci_func_supports_acs }, // PCI-to-PCI bridge

Would this work, too?


BTW:
Isn't there a deterministic possibility to check if the MF device does
peer-to-peer transfers? It could be a external program, which tells the
user, if the hardware could be used the desired way.



Thanks,
kind regards,
Andreas

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Sat, Jun 09, 2012 at 08:50:55AM -0600, Alex Williamson wrote:
> > > Hmm, I wonder if we should make a kernel boot parameter that allows
> > > whitelisting some devices.  I think it would have to taint the kernel
> > > but there's probably sufficient interest for usability vs
> > > supportability.
> > 
> > Good idea. I would print an additional big fat warning of dataloss /
> > filesystem damage / crash if this could be the case.
> 
> Well, outlining the risk above makes me a little more nervous about
> making such a config option, even if it taints the kernel, available so
> easily... :^\  Thanks,
> 
> Alex

Yes, please do not do it. Users will use it and we'll have an
unfixable security bug on our hands.

-- 
MST

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> Hi Dominic,
> 
> Dominic Eschweiler wrote:
> > 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.  

VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
what CPU he's working, so it's not clear if he can use VFIO at all.

UIO is intended for general use with devices that have mappable registers
and don't fit into any other subsystem. No more, no less.

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Jan Kiszka]

On 2012-06-08 18:44, Hans J. Koch wrote:
> On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
>> Hi Dominic,
>>
>> Dominic Eschweiler wrote:
>>> 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.  
> 
> VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> what CPU he's working, so it's not clear if he can use VFIO at all.
> 
> UIO is intended for general use with devices that have mappable registers
> and don't fit into any other subsystem. No more, no less.

Maybe it is worth mentioning in the docs that UIO cannot support DMA,
thus bus master mode. Apparently people do not realize this constraint
and expect more of it than it is supposed to deliver.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > Hi Dominic,
> > 
> > Dominic Eschweiler wrote:
> > > 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.  
> 
> VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> what CPU he's working, so it's not clear if he can use VFIO at all.
> 
> UIO is intended for general use with devices that have mappable registers
> and don't fit into any other subsystem. No more, no less.

VFIO is a secure UIO.  An IOMMU is required for any model that allows a
user to program DMA of a device, whether VFIO or something else.  As
Michael noted, if we allow UIO PCI to enable bus master, we open
ourselves up to a whole world of problems with a user suddenly having
access to and DMA'able memory in the system.  So really, add that the
device must only use PIO to your list of requirements for UIO, which
rules out any kind of high speed device.  Thanks,

Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > Hi Dominic,
> > > 
> > > Dominic Eschweiler wrote:
> > > > 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.  
> > 
> > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > what CPU he's working, so it's not clear if he can use VFIO at all.
> > 
> > UIO is intended for general use with devices that have mappable registers
> > and don't fit into any other subsystem. No more, no less.
> 
> VFIO is a secure UIO.

A secure UIO *for VFs*. I think that's why it's called VFIO :).
Other stuff sometimes also works but no real guarantees, though
VFIO tries to make sure you don't burn yourself too badly
if it breaks.

-- 
MST

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Sun, 2012-06-10 at 17:18 +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> > On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > > Hi Dominic,
> > > > 
> > > > Dominic Eschweiler wrote:
> > > > > 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.  
> > > 
> > > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > > what CPU he's working, so it's not clear if he can use VFIO at all.
> > > 
> > > UIO is intended for general use with devices that have mappable registers
> > > and don't fit into any other subsystem. No more, no less.
> > 
> > VFIO is a secure UIO.
> 
> A secure UIO *for VFs*. I think that's why it's called VFIO :).
> Other stuff sometimes also works but no real guarantees, though
> VFIO tries to make sure you don't burn yourself too badly
> if it breaks.

We do a little better than that.  Multifunction devices that don't
explicitly report ACS support are grouped together, so we have security
for multifunction devices as well.  Either single of multifunction PFs
can have an option ROM, but since there's no defined mechanism to
program the ROM, we can't protect it.  Secure boot actually helps us
here since the ROM loaded by the host BIOS or drivers would need to
verify the ROM before using it.  Note that secure boot will likely close
off the pci-sysfs path uio_pci and KVM device assignment use to get
resources since it allows unprotected access to the system.  VFIO
provides an interface where we control secure access, so should be
compatible with secure boot.  Thanks,

Alex

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Sun, Jun 10, 2012 at 10:09:26AM -0600, Alex Williamson wrote:
> On Sun, 2012-06-10 at 17:18 +0300, Michael S. Tsirkin wrote:
> > On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> > > On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > > > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > > > Hi Dominic,
> > > > > 
> > > > > Dominic Eschweiler wrote:
> > > > > > 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.  
> > > > 
> > > > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > > > what CPU he's working, so it's not clear if he can use VFIO at all.
> > > > 
> > > > UIO is intended for general use with devices that have mappable registers
> > > > and don't fit into any other subsystem. No more, no less.
> > > 
> > > VFIO is a secure UIO.
> > 
> > A secure UIO *for VFs*. I think that's why it's called VFIO :).
> > Other stuff sometimes also works but no real guarantees, though
> > VFIO tries to make sure you don't burn yourself too badly
> > if it breaks.
> 
> We do a little better than that.  Multifunction devices that don't
> explicitly report ACS support are grouped together, so we have security
> for multifunction devices as well.

How can you get security with insecure hardware?

So you prevent the device from writing to host memory? Cool.
Now guest puts a virus on an on-card flash, the
moment device is assigned to another VM it will own that,
or host if it's enabled in host.

I can make up more silliness.  Buggy userspace can brick the device,
e.g. by damaging the internal eeprom memory, and these things were known
to happen even by accident.

Simply put if you want secure userspace drivers you must be able to
trust your hardware for security and the only hardware that promises you
security is a VF in an SRIOV device.

> Either single of multifunction PFs
> can have an option ROM, but since there's no defined mechanism to
> program the ROM, we can't protect it.  Secure boot actually helps us
> here since the ROM loaded by the host BIOS or drivers would need to
> verify the ROM before using it.  Note that secure boot will likely close
> off the pci-sysfs path uio_pci and KVM device assignment use to get
> resources since it allows unprotected access to the system.  VFIO
> provides an interface where we control secure access, so should be
> compatible with secure boot.  Thanks,
> 
> Alex

IMHO all this means VFIO *works* not just for VFs.
Not that it's secure.

-- 
MST

Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson]

On Sun, 2012-06-10 at 19:44 +0300, Michael S. Tsirkin wrote:
> On Sun, Jun 10, 2012 at 10:09:26AM -0600, Alex Williamson wrote:
> > On Sun, 2012-06-10 at 17:18 +0300, Michael S. Tsirkin wrote:
> > > On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> > > > On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > > > > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > > > > Hi Dominic,
> > > > > > 
> > > > > > Dominic Eschweiler wrote:
> > > > > > > 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.  
> > > > > 
> > > > > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > > > > what CPU he's working, so it's not clear if he can use VFIO at all.
> > > > > 
> > > > > UIO is intended for general use with devices that have mappable registers
> > > > > and don't fit into any other subsystem. No more, no less.
> > > > 
> > > > VFIO is a secure UIO.
> > > 
> > > A secure UIO *for VFs*. I think that's why it's called VFIO :).
> > > Other stuff sometimes also works but no real guarantees, though
> > > VFIO tries to make sure you don't burn yourself too badly
> > > if it breaks.
> > 
> > We do a little better than that.  Multifunction devices that don't
> > explicitly report ACS support are grouped together, so we have security
> > for multifunction devices as well.
> 
> How can you get security with insecure hardware?
> 
> So you prevent the device from writing to host memory? Cool.
> Now guest puts a virus on an on-card flash, the
> moment device is assigned to another VM it will own that,
> or host if it's enabled in host.
> 
> I can make up more silliness.  Buggy userspace can brick the device,
> e.g. by damaging the internal eeprom memory, and these things were known
> to happen even by accident.
> 
> Simply put if you want secure userspace drivers you must be able to
> trust your hardware for security and the only hardware that promises you
> security is a VF in an SRIOV device.

Next I suppose you're going to say assigning a NIC to a guest is
insecure because it could host a malicious OS that infects other systems
on the network.  So to clarify, by secure, I mean that users of VFIO
devices don't have access to the host.  The host still needs to be
suspicious of any data the user might have tainted after a device is
returned.

> > Either single of multifunction PFs
> > can have an option ROM, but since there's no defined mechanism to
> > program the ROM, we can't protect it.  Secure boot actually helps us
> > here since the ROM loaded by the host BIOS or drivers would need to
> > verify the ROM before using it.  Note that secure boot will likely close
> > off the pci-sysfs path uio_pci and KVM device assignment use to get
> > resources since it allows unprotected access to the system.  VFIO
> > provides an interface where we control secure access, so should be
> > compatible with secure boot.  Thanks,
> > 
> > Alex
> 
> IMHO all this means VFIO *works* not just for VFs.
> Not that it's secure.

By your argument above, not even VFs are "secure".  A user could just as
easily taint a disk attached to an HBA VF...

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Sun, Jun 10, 2012 at 11:38:25AM -0600, Alex Williamson wrote:
> On Sun, 2012-06-10 at 19:44 +0300, Michael S. Tsirkin wrote:
> > On Sun, Jun 10, 2012 at 10:09:26AM -0600, Alex Williamson wrote:
> > > On Sun, 2012-06-10 at 17:18 +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> > > > > On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > > > > > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > > > > > Hi Dominic,
> > > > > > > 
> > > > > > > Dominic Eschweiler wrote:
> > > > > > > > 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.  
> > > > > > 
> > > > > > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > > > > > what CPU he's working, so it's not clear if he can use VFIO at all.
> > > > > > 
> > > > > > UIO is intended for general use with devices that have mappable registers
> > > > > > and don't fit into any other subsystem. No more, no less.
> > > > > 
> > > > > VFIO is a secure UIO.
> > > > 
> > > > A secure UIO *for VFs*. I think that's why it's called VFIO :).
> > > > Other stuff sometimes also works but no real guarantees, though
> > > > VFIO tries to make sure you don't burn yourself too badly
> > > > if it breaks.
> > > 
> > > We do a little better than that.  Multifunction devices that don't
> > > explicitly report ACS support are grouped together, so we have security
> > > for multifunction devices as well.
> > 
> > How can you get security with insecure hardware?
> > 
> > So you prevent the device from writing to host memory? Cool.
> > Now guest puts a virus on an on-card flash, the
> > moment device is assigned to another VM it will own that,
> > or host if it's enabled in host.
> > 
> > I can make up more silliness.  Buggy userspace can brick the device,
> > e.g. by damaging the internal eeprom memory, and these things were known
> > to happen even by accident.
> > 
> > Simply put if you want secure userspace drivers you must be able to
> > trust your hardware for security and the only hardware that promises you
> > security is a VF in an SRIOV device.
> 
> Next I suppose you're going to say assigning a NIC to a guest is
> insecure because it could host a malicious OS that infects other systems
> on the network.

*Of course* it is less secure than a firewalled guest
with a virtual NIC. You argue this is not true?

But at least there are ways to contain a NIC on a network.
So it depends on the setup.

Not so for an assigned PF. It depends on the internals of the PF
which you have no idea about.

> So to clarify, by secure, I mean that users of VFIO
> devices don't have access to the host.

Yes. And since you can't guarantee it for PFs, it's insecure.

> The host still needs to be
> suspicious of any data the user might have tainted

That's not the only point. Host data might also leak to guest
when device is assigned.

> after a device is returned.

For a VF you have a way to validate what the VF does.
For a PF there is no way to be suspicious of the device state.

> > > Either single of multifunction PFs
> > > can have an option ROM, but since there's no defined mechanism to
> > > program the ROM, we can't protect it.  Secure boot actually helps us
> > > here since the ROM loaded by the host BIOS or drivers would need to
> > > verify the ROM before using it.  Note that secure boot will likely close
> > > off the pci-sysfs path uio_pci and KVM device assignment use to get
> > > resources since it allows unprotected access to the system.  VFIO
> > > provides an interface where we control secure access, so should be
> > > compatible with secure boot.  Thanks,
> > > 
> > > Alex
> > 
> > IMHO all this means VFIO *works* not just for VFs.
> > Not that it's secure.
> 
> By your argument above, not even VFs are "secure".

VFs can be secure if PF hardware and driver are secure.

There's no sure way to secure a PF.

>  A user could just as
> easily taint a disk attached to an HBA VF...

But if I don't run stuff from this disk I am safe.

What is the way to guarantee security with an assigned PF?

-- 
MST

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Sun, Jun 10, 2012 at 11:38:25AM -0600, Alex Williamson wrote:
> On Sun, 2012-06-10 at 19:44 +0300, Michael S. Tsirkin wrote:
> > On Sun, Jun 10, 2012 at 10:09:26AM -0600, Alex Williamson wrote:
> > > On Sun, 2012-06-10 at 17:18 +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> > > > > On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > > > > > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > > > > > Hi Dominic,
> > > > > > > 
> > > > > > > Dominic Eschweiler wrote:
> > > > > > > > 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.  
> > > > > > 
> > > > > > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > > > > > what CPU he's working, so it's not clear if he can use VFIO at all.
> > > > > > 
> > > > > > UIO is intended for general use with devices that have mappable registers
> > > > > > and don't fit into any other subsystem. No more, no less.
> > > > > 
> > > > > VFIO is a secure UIO.
> > > > 
> > > > A secure UIO *for VFs*. I think that's why it's called VFIO :).
> > > > Other stuff sometimes also works but no real guarantees, though
> > > > VFIO tries to make sure you don't burn yourself too badly
> > > > if it breaks.
> > > 
> > > We do a little better than that.  Multifunction devices that don't
> > > explicitly report ACS support are grouped together, so we have security
> > > for multifunction devices as well.
> > 
> > How can you get security with insecure hardware?
> > 
> > So you prevent the device from writing to host memory? Cool.
> > Now guest puts a virus on an on-card flash, the
> > moment device is assigned to another VM it will own that,
> > or host if it's enabled in host.
> > 
> > I can make up more silliness.  Buggy userspace can brick the device,
> > e.g. by damaging the internal eeprom memory, and these things were known
> > to happen even by accident.
> > 
> > Simply put if you want secure userspace drivers you must be able to
> > trust your hardware for security and the only hardware that promises you
> > security is a VF in an SRIOV device.

One thing I stand corrected on: assigning a PF that does DMA with VFIO
*might* be secure, and sometimes, maybe often, is.
There's just no way to make sure.
This is unlike uio_pci_generic where it would always be insecure.

-- 
MST

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Sun, Jun 10, 2012 at 10:00:36PM +0300, Michael S. Tsirkin wrote:
> 
> One thing I stand corrected on: assigning a PF that does DMA with VFIO
> *might* be secure, and sometimes, maybe often, is.
> There's just no way to make sure.
> This is unlike uio_pci_generic where it would always be insecure.

You need to be root to access a UIO device, and if you're root, you can
compromise a system in many ways. Before UIO, people used /dev/mem for
similar purposes, and UIO is certainly a seccurity improvement over that.

But of course, UIO presents security risks. Like many other things below
/dev, you need to know what you're doing, and who gets access to /dev/uioX.

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Sun, Jun 10, 2012 at 09:11:30PM +0200, Hans J. Koch wrote:
> On Sun, Jun 10, 2012 at 10:00:36PM +0300, Michael S. Tsirkin wrote:
> > 
> > One thing I stand corrected on: assigning a PF that does DMA with VFIO
> > *might* be secure, and sometimes, maybe often, is.
> > There's just no way to make sure.
> > This is unlike uio_pci_generic where it would always be insecure.
> 
> You need to be root to access a UIO device, and if you're root, you can
> compromise a system in many ways. Before UIO, people used /dev/mem for
> similar purposes, and UIO is certainly a seccurity improvement over that.
> 
> But of course, UIO presents security risks. Like many other things below
> /dev, you need to know what you're doing, and who gets access to /dev/uioX.
> 
> Thanks,
> Hans

Sorry I might not have explained myself clearly.  uio_pci_generic would
be insecure if used with a device doing DMA. I am not speaking
about UIO in general at all.

-- 
MST

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Sun, Jun 10, 2012 at 10:16:54PM +0300, Michael S. Tsirkin wrote:
> On Sun, Jun 10, 2012 at 09:11:30PM +0200, Hans J. Koch wrote:
> > On Sun, Jun 10, 2012 at 10:00:36PM +0300, Michael S. Tsirkin wrote:
> > > 
> > > One thing I stand corrected on: assigning a PF that does DMA with VFIO
> > > *might* be secure, and sometimes, maybe often, is.
> > > There's just no way to make sure.
> > > This is unlike uio_pci_generic where it would always be insecure.
> > 
> > You need to be root to access a UIO device, and if you're root, you can
> > compromise a system in many ways. Before UIO, people used /dev/mem for
> > similar purposes, and UIO is certainly a seccurity improvement over that.
> > 
> > But of course, UIO presents security risks. Like many other things below
> > /dev, you need to know what you're doing, and who gets access to /dev/uioX.
> > 
> > Thanks,
> > Hans
> 
> Sorry I might not have explained myself clearly.  uio_pci_generic would
> be insecure if used with a device doing DMA. I am not speaking
> about UIO in general at all.

Oh, I do. There are many more risks than just DMA. I come from the embedded
systems world, and there it is not uncommon that some strange device can
simply turn the power off of some of your chips or even the whole system
if programmed properly. And there are a lot of things that might be fine
from the kernel's point of view, but render the system unusable from a
user's point of view.

UIO is a very thin layer on top of strange hardware. It just fills a gap
for a certain class of devices that don't fit in anywhere else. Although I'm
glad if somebody posts his UIO driver, I'm even more glad if another
subsystem (IIO, VFIO) can be found for the damn chip ;-)

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 11:11:16AM -0600, Alex Williamson wrote:
> On Fri, 2012-06-08 at 18:44 +0200, Hans J. Koch wrote:
> > On Fri, Jun 08, 2012 at 06:16:18PM +0200, Andreas Hartmann wrote:
> > > Hi Dominic,
> > > 
> > > Dominic Eschweiler wrote:
> > > > 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.  
> > 
> > VFIO is not a "better UIO". It *requires* an IOMMU. Dominic didn't say on
> > what CPU he's working, so it's not clear if he can use VFIO at all.
> > 
> > UIO is intended for general use with devices that have mappable registers
> > and don't fit into any other subsystem. No more, no less.
> 
> VFIO is a secure UIO.  An IOMMU is required for any model that allows a
> user to program DMA of a device, whether VFIO or something else.  As
> Michael noted, if we allow UIO PCI to enable bus master, we open
> ourselves up to a whole world of problems with a user suddenly having
> access to and DMA'able memory in the system.  So really, add that the
> device must only use PIO to your list of requirements for UIO, which
> rules out any kind of high speed device.  Thanks,

You're living in a completely different world. If you want to discuss UIO,
it would be best if you forget PCI, DMA, and virtualization first.

UIO was introduced to handle crazy hardware that doesn't fit into an existing
subsystem, e.g. a customer specific FPGA. Before UIO, people wrote kernel
char device drivers with 5000+ lines of code for that. They controlled their
device with 50 new ioctls, the driver was buggy and could never make it to
mainline.

With UIO, they write a very simple kernel driver, typically around 150 LOC.
The rest can be done in userspace. That's the purpose of UIO. And since
strange devices like the FPGA mentioned above are mainly found in
Embedded Devices, it's absolutely neccesary that UIO works on ANY CPU,
including ARM, MIPS, older x86 like LX800, or PowerPC. Not just some
shiny new Core i7 with IOMMU. And it's not only for PCI, most of the drivers
use platform devices and the chips are directly attached to some SoC bus.

And one last word about DMA: The reason the UIO core doesn't support DMA
is simply that so far nobody wanted it. In five years, I never had a device
on my desk where a UIO driver needed DMA support by the core. On the other
hand, I met several people who said "Bad thing that UIO doesn't support DMA".
When I asked what they're about to do, everyone answered "Oh well, I don't
need DMA, but UIO should support it, don't you think so?". So much for that.

One fine day some time ago (2010?), somebody came up with a patch set that
attempted to change half the UIO core. In the resulting discussion, we
quickly found out that he had completly different requirements for a
completely different purpose. The result of that insight was the birth
of VFIO. A new subsystem for new requirements.

If you say "VFIO is a secure UIO", then please post a replacement for
uio_pdrv_genirq.c that runs on an ARM9 and offers the advantages you're
talking about. If that is impossible, please stop comparing VFIO with UIO.

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Dominic Eschweiler]

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/XXXXXresourceX
> ?
> 
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

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 04:28:58PM +0200, Dominic Eschweiler wrote:
> 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/XXXXXresourceX
> > ?
> > 
> 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.

Then there's something fundamentally wrong in your driver. Check the return
value of uio_register_device().

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

Try and fix your driver, then post it. If you absolutely don't get it working,
post a non-working version for review.

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Dominic Eschweiler]

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)

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 05:45:12PM +0200, Dominic Eschweiler wrote:
> 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.

As Michael already said, the intention was to map BAR ressources using
/sys/bus/pci/devices/...

OK, I agree, that's the standard PCI way of doing this, not the standard
UIO way.

What problem do you have with this approach?

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Dominic Eschweiler]

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)

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 06:23:19PM +0200, Dominic Eschweiler wrote:
> 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.

Well, the documentation says "Userspace driver can use pci sysfs interface,
or the libpci libray that wraps it, to talk to the device..."

I agree that this is not very clear, especially since the example code there
doesn't use this.

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

I'm not sure if talking to the device the UIO way can confuse the PCI
subsystem. Are you? Any hints from somebody else?

Unfortunately, I can't remember why we agreed to do it the way it is...

> 
> Anyway, what is wrong when the uio_pci_generic module works like
> described in the related documentation?

It does, the documentation is just not very explicit when it comes to
memory mapping.

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Dominic Eschweiler]

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)

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 07:07:46PM +0200, Dominic Eschweiler wrote:
> 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. 

Ah OK, it's easy to misunderstand that.

> 
> Therefore, would it be OK if I send a patch for the documentation next
> week?

Sure, that would be great!

Thanks,
Hans

Re: [PATCH] uio_pci_generic does not export memory resources

[Michael S. Tsirkin]

On Fri, Jun 08, 2012 at 06:23:19PM +0200, Dominic Eschweiler wrote:
> 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? 

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.

maybe we should make the point that *all* interaction with
the device should be done via sysfs, explicit.

> -- 
> Gruß
>   Dominic
> 
> Frankfurt Institute for Advanced Studies (FIAS)
> Ruth-Moufang-Straße 1
> D-60438 Frankfurt am Main
> Germany
> 
> Phone:  +49 (0)

Re: [PATCH] uio_pci_generic does not export memory resources

[Hans J. Koch]

On Fri, Jun 08, 2012 at 01:56:56PM +0200, Dominic Eschweiler wrote:
> 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 <eschweiler@fias.uni-frankfurt.de>
> ---
> 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.000000000 +0200
> +++ linux-3.4_new/drivers/uio/uio_pci_generic.c	2012-06-08
> 13:01:12.000000000 +0200
> @@ -25,10 +25,12 @@
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
>  
> -#define DRIVER_VERSION	"0.01.0"
> +#define DRIVER_VERSION	"0.02.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>  #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;
> +		}
> +	}
> +

All this DMA stuff doesn't fit into a "uio_pci_generic" driver. There might
be users who need other kinds of DMA handling.

If you need this, please make it a new driver and name it after your device.

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

Do you really have x86 ports on your PCI card?

> +
> +	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;
> +		}
> +	}

As Michael said, why don't you just use the existing PCI sysfs files?

I don't really object to your approach, but I'd like to hear a reason
why you can't live with the existing possibilities.

Thanks,
Hans