Skip to contentSkip to conversation

uio: add generic driver for PCI 2.3 devices

RFC PATCH Superseded Michael S. Tsirkin 2 participants 6 messages Jul 13, 2009 – Jul 15, 2009

Subsystems (4)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

RFC PATCH v3 1/1 uio: add generic driver for PCI 2.3 devices

This adds a generic uio driver that can bind to any PCI device. First user will be virtualization where a qemu userspace process needs to give guest OS access to the device.

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. Driver detects this support, and won't bind to devices which do not support the Interrupt Disable Bit in the command register.

It's expected that more features of interest to virtualization will be added to this driver in the future. Possibilities are: mmap for device resources, MSI/MSI-X, eventfd (to interface with kvm), iommu.

---

This is intended to solve the problem that shared interrupts do not work with assigned devices. Please take a look before this goes out to lkml.

Changes since v1:
- some naming changes
- do a single read to get both command and status register
- remove irqcontrol: user can enable interrupts by writing command register directly
- don't claim resources as we don't support mmap yet, but note the intent to do so in the commit log

 MAINTAINERS                   |    8 ++
 drivers/uio/Kconfig           |   10 ++
 drivers/uio/Makefile          |    1 +
 drivers/uio/uio_pci_generic.c |  206 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_regs.h      |    1 +
 5 files changed, 226 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pci_generic.c
MAINTAINERS +8 -0
index cf4abdd..da4a3ab 100644
@@ -2425,6 +2425,14 @@ F:	drivers/net/wan/pc300too.c
 F:	drivers/net/wan/pci200syn.c
 F:	drivers/net/wan/wanxl*
 
+GENERIC UIO DRIVER FOR PCI DEVICES
+P:	Michael S. Tsirkin
+M:	[email protected]
+L:	[email protected]
+L:	[email protected]
+S:	Supported
+F:	drivers/uio/uio_pci_generic.c
+
 GFS2 FILE SYSTEM
 P:	Steven Whitehouse
 M:	[email protected]
drivers/uio/Kconfig +10 -0
index 7f86534..0f14c8e 100644
@@ -89,4 +89,14 @@ config UIO_SERCOS3
 
 	  If you compile this as a module, it will be called uio_sercos3.
 
+config UIO_PCI_GENERIC
+	tristate "Generic driver for PCI 2.3 and PCI Express cards"
+	depends on PCI
+	default n
+	help
+	  Generic driver that you can bind, dynamically, to any
+	  PCI 2.3 compliant and PCI Express card. It is useful,
+	  primarily, for virtualization scenarios.
+	  If you compile this as a module, it will be called uio_pci_generic.
+
 endif
drivers/uio/Makefile +1 -0
index 5c2586d..73b2e75 100644
@@ -5,3 +5,4 @@ obj-$(CONFIG_UIO_PDRV_GENIRQ)	+= uio_pdrv_genirq.o
 obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
 obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
 obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
+obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
drivers/uio/uio_pci_generic.c new file +206 -0

206 lines not on this page · raw message

include/linux/pci_regs.h +1 -0
index 616bf8b..bfb9b31 100644
@@ -42,6 +42,7 @@
 #define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
 
 #define PCI_STATUS		0x06	/* 16 bits */
+#define  PCI_STATUS_INTERRUPT	0x08	/* Interrupt status */
 #define  PCI_STATUS_CAP_LIST	0x10	/* Support Capability List */
 #define  PCI_STATUS_66MHZ	0x20	/* Support 66 Mhz PCI 2.1 bus */
 #define  PCI_STATUS_UDF		0x40	/* Support User Definable Features [obsolete] */
Signature · 1 line
1.6.2.5
#1 lore raw
Quoted 2 lines · * Michael S. Tsirkin ([email protected]) wrote:

- remove irqcontrol: user can enable interrupts by writing command register directly

Sorry if I gave you the impression that removing was needed. I actually think the irqcontrol was useful since it's atomic.

drivers/uio/uio_pci_generic.c line 154

Quoted 2 lines
drivers/uio/uio_pci_generic.c new file +2 -0
+	gdev->info.name = "uio_pci_generic";
+	gdev->info.version = "0.01";

Minor nit: DRIVER_VERSION

Otherwise, it looks good.

thanks,

-chris

author maintainer
#2 lore raw
Quoted 6 lines · Chris Wright
Quoted 2 lines · * Michael S. Tsirkin ([email protected]) wrote:

- remove irqcontrol: user can enable interrupts by writing command register directly

Sorry if I gave you the impression that removing was needed. I actually think the irqcontrol was useful since it's atomic.

What I'm saying, it is not strictly needed (user can safely write 0 in that register and worst case you just get an extra interrupt). So let's make the decision on what does irqcontrol do when we have a pressing need for an extra kernel/user interface.

Quoted 4 lines

drivers/uio/uio_pci_generic.c line 154

Quoted 2 lines
drivers/uio/uio_pci_generic.c new file +2 -0
+	gdev->info.name = "uio_pci_generic";
+	gdev->info.version = "0.01";

Minor nit: DRIVER_VERSION

Ack.

Quoted 5 lines

Otherwise, it looks good.

thanks,

-chris

Thanks!

#3 lore raw
Quoted 5 lines · * Michael S. Tsirkin ([email protected]) wrote:
Quoted 4 lines · Chris Wright
Quoted 2 lines · * Michael S. Tsirkin ([email protected]) wrote:

- remove irqcontrol: user can enable interrupts by writing command register directly

Quoted 7 lines
Quoted 2 lines

Sorry if I gave you the impression that removing was needed. I actually think the irqcontrol was useful since it's atomic.

What I'm saying, it is not strictly needed (user can safely write 0 in that register and worst case you just get an extra interrupt). So let's make the decision on what does irqcontrol do when we have a pressing need for an extra kernel/user interface.

OK. My concern is theoretical (as in the current design wouldn't trigger an issue):

cmd = pread()

             ------+
                    \
cmd &= ~INTX_DISABLE +----+
                    /     |
            -------+      |
pwrite(cmd)               |
                          -

During this window Command Reg can change[1] and cmd is stale

[1] due to irqhandler (doesn't matter in this case since it touches same bit). or due to some other thread updating Command Reg (also doesn't matter since this does not happen right now).

author maintainer
#4 lore raw
Quoted 25 lines · Chris Wright
Quoted 5 lines · * Michael S. Tsirkin ([email protected]) wrote:
Quoted 4 lines · Chris Wright
Quoted 2 lines · * Michael S. Tsirkin ([email protected]) wrote:

- remove irqcontrol: user can enable interrupts by writing command register directly

Quoted 7 lines
Quoted 2 lines

Sorry if I gave you the impression that removing was needed. I actually think the irqcontrol was useful since it's atomic.

What I'm saying, it is not strictly needed (user can safely write 0 in that register and worst case you just get an extra interrupt). So let's make the decision on what does irqcontrol do when we have a pressing need for an extra kernel/user interface.

OK. My concern is theoretical (as in the current design wouldn't trigger an issue):

cmd = pread()

             ------+
                    \
cmd &= ~INTX_DISABLE +----+
                    /     |
            -------+      |
pwrite(cmd)               |
                          -
Quoted 5 lines

During this window Command Reg can change[1] and cmd is stale

[1] due to irqhandler (doesn't matter in this case since it touches same bit). or due to some other thread updating Command Reg (also doesn't matter since this does not happen right now).

Right. Note that the limitation that only a single thread should touch config space through sysfs applies to any sub-byte field. If we wanted to make such ops atomic, pci devices would need some kind of compare and swap ioctl.

Signature · 1 line
MST
Acked-by
#5 lore raw
Quoted 5 lines · * Michael S. Tsirkin ([email protected]) wrote:

Right. Note that the limitation that only a single thread should touch config space through sysfs applies to any sub-byte field. If we wanted to make such ops atomic, pci devices would need some kind of compare and swap ioctl.

Looks good.