[RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Guenter Roeck]

Enable support for MSI interrupts if the device supports it.
Since MSI interrupts are edge triggered, it is no longer necessary to
disable interrupts in the kernel and re-enable them from user-space.
Instead, clearing the interrupt condition in the user space application
automatically re-enables the interrupt.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
An open question is if we can just do this unconditionally
or if there should be some flag to enable it. A module parameter, maybe ?

 Documentation/DocBook/uio-howto.tmpl |   23 ++++++++++++++++++++---
 drivers/uio/uio_pci_generic.c        |   15 ++++++++++++---
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
index 9561815..69b54e0 100644
--- a/Documentation/DocBook/uio-howto.tmpl
+++ b/Documentation/DocBook/uio-howto.tmpl
@@ -46,6 +46,12 @@ GPL version 2.
 
 <revhistory>
 	<revision>
+	<revnumber>0.10</revnumber>
+	<date>2013-06-26</date>
+	<authorinitials>gr</authorinitials>
+	<revremark>Added MSI support to uio_pci_generic.</revremark>
+	</revision>
+	<revision>
 	<revnumber>0.9</revnumber>
 	<date>2009-07-16</date>
 	<authorinitials>mst</authorinitials>
@@ -935,15 +941,26 @@ and look in the output for failure reasons
 <sect1 id="uio_pci_generic_internals">
 <title>Things to know about uio_pci_generic</title>
 	<para>
-Interrupts are handled using the Interrupt Disable bit in the PCI command
+Interrupts are handled either as MSI interrupts (if the device supports it) or
+as legacy INTx interrupts.
+	</para>
+	<para>
+uio_pci_generic automatically configures a device to use MSI interrupts
+if the device supports it. If an MSI interrupt is received, the user space
+driver is notified. Since MSI interrupts are edge sensitive, the user space
+driver needs to clear the interrupt condition in the device before blocking
+and waiting for more interrupts.
+	</para>
+	<para>
+Legacy interrupts are handled using the Interrupt Disable bit in the PCI command
 register and Interrupt Status bit in the PCI status register.  All devices
 compliant to PCI 2.3 (circa 2002) and all compliant PCI Express devices should
 support these bits.  uio_pci_generic detects this support, and won't bind to
 devices which do not support the Interrupt Disable Bit in the command register.
 	</para>
 	<para>
-On each interrupt, uio_pci_generic sets the Interrupt Disable bit.
-This prevents the device from generating further interrupts
+If legacy interrupts are used, uio_pci_generic sets the Interrupt Disable bit on
+each interrupt. This prevents the device from generating further interrupts
 until the bit is cleared. The userspace driver should clear this
 bit before blocking and waiting for more interrupts.
 	</para>
diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index 14aa10c..3366fdb 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -32,6 +32,7 @@
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
+	bool have_msi;
 };
 
 static inline struct uio_pci_generic_dev *
@@ -46,7 +47,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
 
-	if (!pci_check_and_mask_intx(gdev->pdev))
+	if (!gdev->have_msi && !pci_check_and_mask_intx(gdev->pdev))
 		return IRQ_NONE;
 
 	/* UIO core will signal the user process. */
@@ -58,6 +59,7 @@ static int probe(struct pci_dev *pdev,
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
+	bool have_msi = false;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -73,7 +75,9 @@ static int probe(struct pci_dev *pdev,
 		return -ENODEV;
 	}
 
-	if (!pci_intx_mask_supported(pdev)) {
+	if (!pci_enable_msi(pdev)) {
+		have_msi = true;
+	} else if (!pci_intx_mask_supported(pdev)) {
 		err = -ENODEV;
 		goto err_verify;
 	}
@@ -84,10 +88,11 @@ static int probe(struct pci_dev *pdev,
 		goto err_alloc;
 	}
 
+	gdev->have_msi = have_msi;
 	gdev->info.name = "uio_pci_generic";
 	gdev->info.version = DRIVER_VERSION;
 	gdev->info.irq = pdev->irq;
-	gdev->info.irq_flags = IRQF_SHARED;
+	gdev->info.irq_flags = have_msi ? 0 : IRQF_SHARED;
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
 
@@ -99,6 +104,8 @@ static int probe(struct pci_dev *pdev,
 err_register:
 	kfree(gdev);
 err_alloc:
+	if (have_msi)
+		pci_disable_msi(pdev);
 err_verify:
 	pci_disable_device(pdev);
 	return err;
@@ -109,6 +116,8 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	if (gdev->have_msi)
+		pci_disable_msi(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
 }
-- 
1.7.9.5

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Michael S. Tsirkin]

On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> Enable support for MSI interrupts if the device supports it.
> Since MSI interrupts are edge triggered, it is no longer necessary to
> disable interrupts in the kernel and re-enable them from user-space.
> Instead, clearing the interrupt condition in the user space application
> automatically re-enables the interrupt.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> An open question is if we can just do this unconditionally
> or if there should be some flag to enable it. A module parameter, maybe ?

NACK

UIO is for devices that don't do memory writes.
Anything that can do writes must be protected by an IOMMU
and/or have a secure kernel driver, not a UIO stub.

MSI is done by memory writes so if userspace
controls the device it can trick it to write
anywhere in memory.


>  Documentation/DocBook/uio-howto.tmpl |   23 ++++++++++++++++++++---
>  drivers/uio/uio_pci_generic.c        |   15 ++++++++++++---
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
> index 9561815..69b54e0 100644
> --- a/Documentation/DocBook/uio-howto.tmpl
> +++ b/Documentation/DocBook/uio-howto.tmpl
> @@ -46,6 +46,12 @@ GPL version 2.
>  
>  <revhistory>
>  	<revision>
> +	<revnumber>0.10</revnumber>
> +	<date>2013-06-26</date>
> +	<authorinitials>gr</authorinitials>
> +	<revremark>Added MSI support to uio_pci_generic.</revremark>
> +	</revision>
> +	<revision>
>  	<revnumber>0.9</revnumber>
>  	<date>2009-07-16</date>
>  	<authorinitials>mst</authorinitials>
> @@ -935,15 +941,26 @@ and look in the output for failure reasons
>  <sect1 id="uio_pci_generic_internals">
>  <title>Things to know about uio_pci_generic</title>
>  	<para>
> -Interrupts are handled using the Interrupt Disable bit in the PCI command
> +Interrupts are handled either as MSI interrupts (if the device supports it) or
> +as legacy INTx interrupts.
> +	</para>
> +	<para>
> +uio_pci_generic automatically configures a device to use MSI interrupts
> +if the device supports it. If an MSI interrupt is received, the user space
> +driver is notified. Since MSI interrupts are edge sensitive, the user space
> +driver needs to clear the interrupt condition in the device before blocking
> +and waiting for more interrupts.
> +	</para>
> +	<para>
> +Legacy interrupts are handled using the Interrupt Disable bit in the PCI command
>  register and Interrupt Status bit in the PCI status register.  All devices
>  compliant to PCI 2.3 (circa 2002) and all compliant PCI Express devices should
>  support these bits.  uio_pci_generic detects this support, and won't bind to
>  devices which do not support the Interrupt Disable Bit in the command register.
>  	</para>
>  	<para>
> -On each interrupt, uio_pci_generic sets the Interrupt Disable bit.
> -This prevents the device from generating further interrupts
> +If legacy interrupts are used, uio_pci_generic sets the Interrupt Disable bit on
> +each interrupt. This prevents the device from generating further interrupts
>  until the bit is cleared. The userspace driver should clear this
>  bit before blocking and waiting for more interrupts.
>  	</para>
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index 14aa10c..3366fdb 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -32,6 +32,7 @@
>  struct uio_pci_generic_dev {
>  	struct uio_info info;
>  	struct pci_dev *pdev;
> +	bool have_msi;
>  };
>  
>  static inline struct uio_pci_generic_dev *
> @@ -46,7 +47,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  {
>  	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
>  
> -	if (!pci_check_and_mask_intx(gdev->pdev))
> +	if (!gdev->have_msi && !pci_check_and_mask_intx(gdev->pdev))
>  		return IRQ_NONE;
>  
>  	/* UIO core will signal the user process. */
> @@ -58,6 +59,7 @@ static int probe(struct pci_dev *pdev,
>  {
>  	struct uio_pci_generic_dev *gdev;
>  	int err;
> +	bool have_msi = false;
>  
>  	err = pci_enable_device(pdev);
>  	if (err) {
> @@ -73,7 +75,9 @@ static int probe(struct pci_dev *pdev,
>  		return -ENODEV;
>  	}
>  
> -	if (!pci_intx_mask_supported(pdev)) {
> +	if (!pci_enable_msi(pdev)) {
> +		have_msi = true;
> +	} else if (!pci_intx_mask_supported(pdev)) {
>  		err = -ENODEV;
>  		goto err_verify;
>  	}
> @@ -84,10 +88,11 @@ static int probe(struct pci_dev *pdev,
>  		goto err_alloc;
>  	}
>  
> +	gdev->have_msi = have_msi;
>  	gdev->info.name = "uio_pci_generic";
>  	gdev->info.version = DRIVER_VERSION;
>  	gdev->info.irq = pdev->irq;
> -	gdev->info.irq_flags = IRQF_SHARED;
> +	gdev->info.irq_flags = have_msi ? 0 : IRQF_SHARED;
>  	gdev->info.handler = irqhandler;
>  	gdev->pdev = pdev;
>  
> @@ -99,6 +104,8 @@ static int probe(struct pci_dev *pdev,
>  err_register:
>  	kfree(gdev);
>  err_alloc:
> +	if (have_msi)
> +		pci_disable_msi(pdev);
>  err_verify:
>  	pci_disable_device(pdev);
>  	return err;
> @@ -109,6 +116,8 @@ static void remove(struct pci_dev *pdev)
>  	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
>  
>  	uio_unregister_device(&gdev->info);
> +	if (gdev->have_msi)
> +		pci_disable_msi(pdev);
>  	pci_disable_device(pdev);
>  	kfree(gdev);
>  }
> -- 
> 1.7.9.5

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Guenter Roeck]

On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > Enable support for MSI interrupts if the device supports it.
> > Since MSI interrupts are edge triggered, it is no longer necessary to
> > disable interrupts in the kernel and re-enable them from user-space.
> > Instead, clearing the interrupt condition in the user space application
> > automatically re-enables the interrupt.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > An open question is if we can just do this unconditionally
> > or if there should be some flag to enable it. A module parameter, maybe ?
> 
> NACK
> 
> UIO is for devices that don't do memory writes.
> Anything that can do writes must be protected by an IOMMU
> and/or have a secure kernel driver, not a UIO stub.
> 
> MSI is done by memory writes so if userspace
> controls the device it can trick it to write
> anywhere in memory.
> 
Interesting. Thanks for letting me know.

Guenter

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Guenter Roeck]

On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > Enable support for MSI interrupts if the device supports it.
> > Since MSI interrupts are edge triggered, it is no longer necessary to
> > disable interrupts in the kernel and re-enable them from user-space.
> > Instead, clearing the interrupt condition in the user space application
> > automatically re-enables the interrupt.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > An open question is if we can just do this unconditionally
> > or if there should be some flag to enable it. A module parameter, maybe ?
> 
> NACK
> 
> UIO is for devices that don't do memory writes.
> Anything that can do writes must be protected by an IOMMU
> and/or have a secure kernel driver, not a UIO stub.
> 
> MSI is done by memory writes so if userspace
> controls the device it can trick it to write
> anywhere in memory.
> 
Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
isn't that possible anyway, even if MSI is not enabled by the kernel ?
All one would need to do is to enable MSI from user space; after all,
the chip configuration space is writable.

Thanks,
Guenter

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Michael S. Tsirkin]

On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > Enable support for MSI interrupts if the device supports it.
> > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > disable interrupts in the kernel and re-enable them from user-space.
> > > Instead, clearing the interrupt condition in the user space application
> > > automatically re-enables the interrupt.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > An open question is if we can just do this unconditionally
> > > or if there should be some flag to enable it. A module parameter, maybe ?
> > 
> > NACK
> > 
> > UIO is for devices that don't do memory writes.
> > Anything that can do writes must be protected by an IOMMU
> > and/or have a secure kernel driver, not a UIO stub.
> > 
> > MSI is done by memory writes so if userspace
> > controls the device it can trick it to write
> > anywhere in memory.
> > 
> Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> isn't that possible anyway, even if MSI is not enabled by the kernel ?
> All one would need to do is to enable MSI from user space; after all,
> the chip configuration space is writable.
> 
> Thanks,
> Guenter

If a device has capability to do writes, sure. So don't do this then :)

-- 
MST

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Guenter Roeck]

On Thu, Jul 04, 2013 at 10:20:23AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> > On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > > Enable support for MSI interrupts if the device supports it.
> > > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > > disable interrupts in the kernel and re-enable them from user-space.
> > > > Instead, clearing the interrupt condition in the user space application
> > > > automatically re-enables the interrupt.
> > > > 
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > ---
> > > > An open question is if we can just do this unconditionally
> > > > or if there should be some flag to enable it. A module parameter, maybe ?
> > > 
> > > NACK
> > > 
> > > UIO is for devices that don't do memory writes.
> > > Anything that can do writes must be protected by an IOMMU
> > > and/or have a secure kernel driver, not a UIO stub.
> > > 
> > > MSI is done by memory writes so if userspace
> > > controls the device it can trick it to write
> > > anywhere in memory.
> > > 
> > Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> > isn't that possible anyway, even if MSI is not enabled by the kernel ?
> > All one would need to do is to enable MSI from user space; after all,
> > the chip configuration space is writable.
> > 
> > Thanks,
> > Guenter
> 
> If a device has capability to do writes, sure. So don't do this then :)
> 
Not an option. I need to use MSI.

Not that it matters anymore - turns out it was better writing a specific driver
for my devices anyway; I needed to be able to disable chip interrupts before
unloading the driver. But why is it then a reason to NACK this patch ?
Besides, doesn't one have to be root anyway to perform such activities,
which could then be more easily accomplished by writing into /dev/mem ?

Thanks,
Guenter

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Michael S. Tsirkin]

On Thu, Jul 04, 2013 at 07:25:23AM -0700, Guenter Roeck wrote:
> On Thu, Jul 04, 2013 at 10:20:23AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> > > On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > > > Enable support for MSI interrupts if the device supports it.
> > > > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > > > disable interrupts in the kernel and re-enable them from user-space.
> > > > > Instead, clearing the interrupt condition in the user space application
> > > > > automatically re-enables the interrupt.
> > > > > 
> > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > ---
> > > > > An open question is if we can just do this unconditionally
> > > > > or if there should be some flag to enable it. A module parameter, maybe ?
> > > > 
> > > > NACK
> > > > 
> > > > UIO is for devices that don't do memory writes.
> > > > Anything that can do writes must be protected by an IOMMU
> > > > and/or have a secure kernel driver, not a UIO stub.
> > > > 
> > > > MSI is done by memory writes so if userspace
> > > > controls the device it can trick it to write
> > > > anywhere in memory.
> > > > 
> > > Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> > > isn't that possible anyway, even if MSI is not enabled by the kernel ?
> > > All one would need to do is to enable MSI from user space; after all,
> > > the chip configuration space is writable.
> > > 
> > > Thanks,
> > > Guenter
> > 
> > If a device has capability to do writes, sure. So don't do this then :)
> > 
> Not an option. I need to use MSI.
> 
> Not that it matters anymore - turns out it was better writing a specific driver
> for my devices anyway; I needed to be able to disable chip interrupts before
> unloading the driver. But why is it then a reason to NACK this patch ?

There seem to be two cases - either you can't access the device -
and the uio driver is not useful - or you can, and it's not safe.
In both cases the patch does not seem to bring about anything
except user confusion ...

> Besides, doesn't one have to be root anyway to perform such activities,
> which could then be more easily accomplished by writing into /dev/mem ?
> 
> Thanks,
> Guenter

root might not be able to write into /dev/mem.


-- 
MST

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Guenter Roeck]

On Thu, Jul 04, 2013 at 05:34:12PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 04, 2013 at 07:25:23AM -0700, Guenter Roeck wrote:
> > On Thu, Jul 04, 2013 at 10:20:23AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> > > > On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > > > > Enable support for MSI interrupts if the device supports it.
> > > > > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > > > > disable interrupts in the kernel and re-enable them from user-space.
> > > > > > Instead, clearing the interrupt condition in the user space application
> > > > > > automatically re-enables the interrupt.
> > > > > > 
> > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > > ---
> > > > > > An open question is if we can just do this unconditionally
> > > > > > or if there should be some flag to enable it. A module parameter, maybe ?
> > > > > 
> > > > > NACK
> > > > > 
> > > > > UIO is for devices that don't do memory writes.
> > > > > Anything that can do writes must be protected by an IOMMU
> > > > > and/or have a secure kernel driver, not a UIO stub.
> > > > > 
> > > > > MSI is done by memory writes so if userspace
> > > > > controls the device it can trick it to write
> > > > > anywhere in memory.
> > > > > 
> > > > Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> > > > isn't that possible anyway, even if MSI is not enabled by the kernel ?
> > > > All one would need to do is to enable MSI from user space; after all,
> > > > the chip configuration space is writable.
> > > > 
> > > > Thanks,
> > > > Guenter
> > > 
> > > If a device has capability to do writes, sure. So don't do this then :)
> > > 
> > Not an option. I need to use MSI.
> > 
> > Not that it matters anymore - turns out it was better writing a specific driver
> > for my devices anyway; I needed to be able to disable chip interrupts before
> > unloading the driver. But why is it then a reason to NACK this patch ?
> 
> There seem to be two cases - either you can't access the device -
> and the uio driver is not useful - or you can, and it's not safe.
> In both cases the patch does not seem to bring about anything
> except user confusion ...
> 
Sounds like claiming that supporting MSI would cause some kind of confusion.
Not sure if I can follow that logic.

Actually, it simplifies the user space code a lot. Since MSI interrupts are edge
triggered and queued, it is not necessary to disable the interrupts, and all
user space has to do is to remove the interrupt reason. Works quite nicely for
our devices.

As I said, I don't really care too much anymore if this patch is rejected, but
the reasons for the rejection are kind of weak.

> > Besides, doesn't one have to be root anyway to perform such activities,
> > which could then be more easily accomplished by writing into /dev/mem ?
> > 
> > Thanks,
> > Guenter
> 
> root might not be able to write into /dev/mem.
> 
I don't really want to try, but at least it is marked rw for root.

Guenter

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Michael S. Tsirkin]

On Thu, Jul 04, 2013 at 09:35:00AM -0700, Guenter Roeck wrote:
> On Thu, Jul 04, 2013 at 05:34:12PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 04, 2013 at 07:25:23AM -0700, Guenter Roeck wrote:
> > > On Thu, Jul 04, 2013 at 10:20:23AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> > > > > On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > > > > > Enable support for MSI interrupts if the device supports it.
> > > > > > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > > > > > disable interrupts in the kernel and re-enable them from user-space.
> > > > > > > Instead, clearing the interrupt condition in the user space application
> > > > > > > automatically re-enables the interrupt.
> > > > > > > 
> > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > > > ---
> > > > > > > An open question is if we can just do this unconditionally
> > > > > > > or if there should be some flag to enable it. A module parameter, maybe ?
> > > > > > 
> > > > > > NACK
> > > > > > 
> > > > > > UIO is for devices that don't do memory writes.
> > > > > > Anything that can do writes must be protected by an IOMMU
> > > > > > and/or have a secure kernel driver, not a UIO stub.
> > > > > > 
> > > > > > MSI is done by memory writes so if userspace
> > > > > > controls the device it can trick it to write
> > > > > > anywhere in memory.
> > > > > > 
> > > > > Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> > > > > isn't that possible anyway, even if MSI is not enabled by the kernel ?
> > > > > All one would need to do is to enable MSI from user space; after all,
> > > > > the chip configuration space is writable.
> > > > > 
> > > > > Thanks,
> > > > > Guenter
> > > > 
> > > > If a device has capability to do writes, sure. So don't do this then :)
> > > > 
> > > Not an option. I need to use MSI.
> > > 
> > > Not that it matters anymore - turns out it was better writing a specific driver
> > > for my devices anyway; I needed to be able to disable chip interrupts before
> > > unloading the driver. But why is it then a reason to NACK this patch ?
> > 
> > There seem to be two cases - either you can't access the device -
> > and the uio driver is not useful - or you can, and it's not safe.
> > In both cases the patch does not seem to bring about anything
> > except user confusion ...
> > 
> Sounds like claiming that supporting MSI would cause some kind of confusion.
> Not sure if I can follow that logic.
> 
> Actually, it simplifies the user space code a lot. Since MSI interrupts are edge
> triggered and queued, it is not necessary to disable the interrupts, and all
> user space has to do is to remove the interrupt reason. Works quite nicely for
> our devices.
> 
> As I said, I don't really care too much anymore if this patch is rejected, but
> the reasons for the rejection are kind of weak.
> 
> > > Besides, doesn't one have to be root anyway to perform such activities,
> > > which could then be more easily accomplished by writing into /dev/mem ?
> > > 
> > > Thanks,
> > > Guenter
> > 
> > root might not be able to write into /dev/mem.
> > 
> I don't really want to try, but at least it is marked rw for root.
> 
> Guenter

Yes but mechanisms such as selinux can still block it.

-- 
MST

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Guenter Roeck]

On Sun, Jul 07, 2013 at 11:15:11AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 04, 2013 at 09:35:00AM -0700, Guenter Roeck wrote:
> > On Thu, Jul 04, 2013 at 05:34:12PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 04, 2013 at 07:25:23AM -0700, Guenter Roeck wrote:
> > > > On Thu, Jul 04, 2013 at 10:20:23AM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> > > > > > On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > > > > > > Enable support for MSI interrupts if the device supports it.
> > > > > > > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > > > > > > disable interrupts in the kernel and re-enable them from user-space.
> > > > > > > > Instead, clearing the interrupt condition in the user space application
> > > > > > > > automatically re-enables the interrupt.
> > > > > > > > 
> > > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > > > > ---
> > > > > > > > An open question is if we can just do this unconditionally
> > > > > > > > or if there should be some flag to enable it. A module parameter, maybe ?
> > > > > > > 
> > > > > > > NACK
> > > > > > > 
> > > > > > > UIO is for devices that don't do memory writes.
> > > > > > > Anything that can do writes must be protected by an IOMMU
> > > > > > > and/or have a secure kernel driver, not a UIO stub.
> > > > > > > 
> > > > > > > MSI is done by memory writes so if userspace
> > > > > > > controls the device it can trick it to write
> > > > > > > anywhere in memory.
> > > > > > > 
> > > > > > Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> > > > > > isn't that possible anyway, even if MSI is not enabled by the kernel ?
> > > > > > All one would need to do is to enable MSI from user space; after all,
> > > > > > the chip configuration space is writable.
> > > > > > 
> > > > > > Thanks,
> > > > > > Guenter
> > > > > 
> > > > > If a device has capability to do writes, sure. So don't do this then :)
> > > > > 
> > > > Not an option. I need to use MSI.
> > > > 
> > > > Not that it matters anymore - turns out it was better writing a specific driver
> > > > for my devices anyway; I needed to be able to disable chip interrupts before
> > > > unloading the driver. But why is it then a reason to NACK this patch ?
> > > 
> > > There seem to be two cases - either you can't access the device -
> > > and the uio driver is not useful - or you can, and it's not safe.
> > > In both cases the patch does not seem to bring about anything
> > > except user confusion ...
> > > 
> > Sounds like claiming that supporting MSI would cause some kind of confusion.
> > Not sure if I can follow that logic.
> > 
> > Actually, it simplifies the user space code a lot. Since MSI interrupts are edge
> > triggered and queued, it is not necessary to disable the interrupts, and all
> > user space has to do is to remove the interrupt reason. Works quite nicely for
> > our devices.
> > 
> > As I said, I don't really care too much anymore if this patch is rejected, but
> > the reasons for the rejection are kind of weak.
> > 
> > > > Besides, doesn't one have to be root anyway to perform such activities,
> > > > which could then be more easily accomplished by writing into /dev/mem ?
> > > > 
> > > > Thanks,
> > > > Guenter
> > > 
> > > root might not be able to write into /dev/mem.
> > > 
> > I don't really want to try, but at least it is marked rw for root.
> > 
> > Guenter
> 
> Yes but mechanisms such as selinux can still block it.
> 
I hope it can also prevent the use of the the MSI trick on PCIe devices.
After all, the user space mechanisms to write into configuration space
are available for all devices, not just for those with uio drivers.

Guenter

Re: [RFC PATCH] uio: uio_pci_generic: Add support for MSI interrupts

[Michael S. Tsirkin]

On Sun, Jul 07, 2013 at 07:39:01AM -0700, Guenter Roeck wrote:
> On Sun, Jul 07, 2013 at 11:15:11AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 04, 2013 at 09:35:00AM -0700, Guenter Roeck wrote:
> > > On Thu, Jul 04, 2013 at 05:34:12PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 04, 2013 at 07:25:23AM -0700, Guenter Roeck wrote:
> > > > > On Thu, Jul 04, 2013 at 10:20:23AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jun 27, 2013 at 10:00:52AM -0700, Guenter Roeck wrote:
> > > > > > > On Thu, Jun 27, 2013 at 10:45:01AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > On Wed, Jun 26, 2013 at 03:30:23PM -0700, Guenter Roeck wrote:
> > > > > > > > > Enable support for MSI interrupts if the device supports it.
> > > > > > > > > Since MSI interrupts are edge triggered, it is no longer necessary to
> > > > > > > > > disable interrupts in the kernel and re-enable them from user-space.
> > > > > > > > > Instead, clearing the interrupt condition in the user space application
> > > > > > > > > automatically re-enables the interrupt.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > > > > > ---
> > > > > > > > > An open question is if we can just do this unconditionally
> > > > > > > > > or if there should be some flag to enable it. A module parameter, maybe ?
> > > > > > > > 
> > > > > > > > NACK
> > > > > > > > 
> > > > > > > > UIO is for devices that don't do memory writes.
> > > > > > > > Anything that can do writes must be protected by an IOMMU
> > > > > > > > and/or have a secure kernel driver, not a UIO stub.
> > > > > > > > 
> > > > > > > > MSI is done by memory writes so if userspace
> > > > > > > > controls the device it can trick it to write
> > > > > > > > anywhere in memory.
> > > > > > > > 
> > > > > > > Just out of curiosity: Since MSI support is mandatory for all PCIE devices,
> > > > > > > isn't that possible anyway, even if MSI is not enabled by the kernel ?
> > > > > > > All one would need to do is to enable MSI from user space; after all,
> > > > > > > the chip configuration space is writable.
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > Guenter
> > > > > > 
> > > > > > If a device has capability to do writes, sure. So don't do this then :)
> > > > > > 
> > > > > Not an option. I need to use MSI.
> > > > > 
> > > > > Not that it matters anymore - turns out it was better writing a specific driver
> > > > > for my devices anyway; I needed to be able to disable chip interrupts before
> > > > > unloading the driver. But why is it then a reason to NACK this patch ?
> > > > 
> > > > There seem to be two cases - either you can't access the device -
> > > > and the uio driver is not useful - or you can, and it's not safe.
> > > > In both cases the patch does not seem to bring about anything
> > > > except user confusion ...
> > > > 
> > > Sounds like claiming that supporting MSI would cause some kind of confusion.
> > > Not sure if I can follow that logic.
> > > 
> > > Actually, it simplifies the user space code a lot. Since MSI interrupts are edge
> > > triggered and queued, it is not necessary to disable the interrupts, and all
> > > user space has to do is to remove the interrupt reason. Works quite nicely for
> > > our devices.
> > > 
> > > As I said, I don't really care too much anymore if this patch is rejected, but
> > > the reasons for the rejection are kind of weak.
> > > 
> > > > > Besides, doesn't one have to be root anyway to perform such activities,
> > > > > which could then be more easily accomplished by writing into /dev/mem ?
> > > > > 
> > > > > Thanks,
> > > > > Guenter
> > > > 
> > > > root might not be able to write into /dev/mem.
> > > > 
> > > I don't really want to try, but at least it is marked rw for root.
> > > 
> > > Guenter
> > 
> > Yes but mechanisms such as selinux can still block it.
> > 
> I hope it can also prevent the use of the the MSI trick on PCIe devices.
> After all, the user space mechanisms to write into configuration space
> are available for all devices, not just for those with uio drivers.
> 
> Guenter

Yes, selinux can also prevent access to sysfs and procfs for any process.

-- 
MST