uio: add irq control support to uio_pci_generic

PATCH In review Stephen Hemminger 2 participants 6 messages Apr 15, 2015 – Apr 20, 2015

Subsystems (3)

KVM Userspace I/O (UIO) Generic UIO driver for PCI devices

lore

Share & tools

lore

Stephen Hemminger

author

2015-04-15 18:05 UTC #0 lore raw

PATCH 1/1 uio: add irq control support to uio_pci_generic

The driver already supported INTX interrupts but had no in kernel function to enable and disable them.

It is possible for userspace to do this by accessing PCI config directly, but this racy and better handled by same mechanism that already exists in kernel.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/uio/uio_pci_generic.c +13 -0

@@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s
 	return IRQ_HANDLED;
 }
 
+static int irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+	struct pci_dev *pdev = gdev->pdev;
+
+	pci_cfg_access_lock(pdev);
+	pci_intx(pdev, irq_on);
+	pci_cfg_access_unlock(pdev);
+
+	return 0;
+}
+
 static int probe(struct pci_dev *pdev,
 			   const struct pci_device_id *id)
 {

@@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev,
 	gdev->info.irq = pdev->irq;
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
+	gdev->info.irqcontrol = irqcontrol;
 	gdev->pdev = pdev;
 
 	err = uio_register_device(&pdev->dev, &gdev->info);

Michael S. Tsirkin

maintainer

2015-04-16 07:43 UTC #1 lore raw

Quoted 5 lines · Stephen Hemminger

The driver already supported INTX interrupts but had no in kernel function to enable and disable them.
It is possible for userspace to do this by accessing PCI config directly, but this racy

How is it racy? We have userspace using this interface, if there's a race I want to fix it.

Quoted 35 lines

and better handled by same mechanism
that already exists in kernel.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/uio/uio_pci_generic.c +13 -0

@@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s
 	return IRQ_HANDLED;
 }
 
+static int irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+	struct pci_dev *pdev = gdev->pdev;
+
+	pci_cfg_access_lock(pdev);
+	pci_intx(pdev, irq_on);
+	pci_cfg_access_unlock(pdev);
+
+	return 0;
+}
+
 static int probe(struct pci_dev *pdev,
 			   const struct pci_device_id *id)
 {

@@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev,
 	gdev->info.irq = pdev->irq;
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
+	gdev->info.irqcontrol = irqcontrol;
 	gdev->pdev = pdev;
 
 	err = uio_register_device(&pdev->dev, &gdev->info);

Stephen Hemminger

author

2015-04-16 21:21 UTC #2 lore raw

Quoted 9 lines · On Thu, 16 Apr 2015 09:43:24 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 5 lines · Stephen Hemminger
The driver already supported INTX interrupts but had no in kernel function to enable and disable them.
It is possible for userspace to do this by accessing PCI config directly, but this racy
How is it racy? We have userspace using this interface, if there's a race I want to fix it.

There is nothing to prevent two threads in user space doing read/modify write at the same time.

The bigger issue is that DPDK needs to support multiple UIO interface types. And with current model there is no abstraction. The way to enable/disable IRQ is different depending on the UIO drivers.

Michael S. Tsirkin

maintainer

2015-04-20 13:59 UTC #3 lore raw

Quoted 7 lines · Stephen Hemminger

Quoted 4 lines · On Thu, 16 Apr 2015 09:43:24 +0200 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 3 lines · Stephen Hemminger
The driver already supported INTX interrupts but had no in kernel function to enable and disable them.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
It is possible for userspace to do this by accessing PCI config directly, but this racy
How is it racy? We have userspace using this interface, if there's a race I want to fix it.
There is nothing to prevent two threads in user space doing read/modify write at the same time.

Well that's a userspace bug then - so let's drop that from commit log lest people think this fixes some kernel bugs. read/modify/write to the same register is at least an easy to grasp problem, creating an extra interface for the same function opens up the possibility that some userspace will do read/modify/write from one thread with irqcontrol from another thread, creating more races.

Quoted 4 lines

The bigger issue is that DPDK needs to support multiple UIO interface types. And with current model there is no abstraction. The way to enable/disable IRQ is different depending on the UIO drivers.

OK compatibility with other devices might be useful, but what are the other UIO drivers DPDK supports? I only found support for igb_uio so far, and that doesn't seem to be upstream.

Signature · 1 line

MST

Stephen Hemminger

author

2015-04-20 15:33 UTC #4 lore raw

Quoted 26 lines · On Mon, 20 Apr 2015 15:59:06 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 7 lines · Stephen Hemminger
Quoted 4 lines · On Thu, 16 Apr 2015 09:43:24 +0200 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 3 lines · Stephen Hemminger
The driver already supported INTX interrupts but had no in kernel function to enable and disable them.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
It is possible for userspace to do this by accessing PCI config directly, but this racy
How is it racy? We have userspace using this interface, if there's a race I want to fix it.
There is nothing to prevent two threads in user space doing read/modify write at the same time.
Well that's a userspace bug then - so let's drop that from commit log lest people think this fixes some kernel bugs. read/modify/write to the same register is at least an easy to grasp problem, creating an extra interface for the same function opens up the possibility that some userspace will do read/modify/write from one thread with irqcontrol from another thread, creating more races.

Quoted 9 lines

Quoted 4 lines
The bigger issue is that DPDK needs to support multiple UIO interface types. And with current model there is no abstraction. The way to enable/disable IRQ is different depending on the UIO drivers.
OK compatibility with other devices might be useful, but what are the other UIO drivers DPDK supports? I only found support for igb_uio so far, and that doesn't seem to be upstream.

Currently, supports:
igb_uio, uio_pci_generic (as well as vfio)

There are additional drivers which been submitted but not accepted for Xen and HyperV both of which require special uio drivers.

Michael S. Tsirkin

maintainer

2015-04-20 15:40 UTC #5 lore raw

Quoted 39 lines · Stephen Hemminger

Quoted 26 lines · On Mon, 20 Apr 2015 15:59:06 +0200 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 7 lines · Stephen Hemminger
Quoted 4 lines · On Thu, 16 Apr 2015 09:43:24 +0200 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 3 lines · Stephen Hemminger
The driver already supported INTX interrupts but had no in kernel function to enable and disable them.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
It is possible for userspace to do this by accessing PCI config directly, but this racy
How is it racy? We have userspace using this interface, if there's a race I want to fix it.
There is nothing to prevent two threads in user space doing read/modify write at the same time.
Well that's a userspace bug then - so let's drop that from commit log lest people think this fixes some kernel bugs. read/modify/write to the same register is at least an easy to grasp problem, creating an extra interface for the same function opens up the possibility that some userspace will do read/modify/write from one thread with irqcontrol from another thread, creating more races.
Quoted 9 lines
Quoted 4 lines
The bigger issue is that DPDK needs to support multiple UIO interface types. And with current model there is no abstraction. The way to enable/disable IRQ is different depending on the UIO drivers.
OK compatibility with other devices might be useful, but what are the other UIO drivers DPDK supports? I only found support for igb_uio so far, and that doesn't seem to be upstream.

Quoted 5 lines

Currently, supports:
igb_uio, uio_pci_generic (as well as vfio)
There are additional drivers which been submitted but not accepted for Xen and HyperV both of which require special uio drivers.

Well vfio doesn't have irq_control, does it? So I'd say it's best to wait and see before we commit to a new ABI then. You probably need to support existing kernels anyway, if igb_uio makes it upstream, then adding an interface that's consistent with it will make sense.

Signature · 1 line

MST