[PATCHv2 RFC] virtio-pci: flexible configuration layout

[PATCHv2 RFC] virtio-pci: flexible configuration layout

[Michael S. Tsirkin]

Add a flexible mechanism to specify virtio configuration layout, using
pci vendor-specific capability.  A separate capability is used for each
of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes
also need arch updates for non-x86.
There might also be more spec changes.

Posting here for early feedback, and to allow Sasha to
proceed with his "kvm tool" work.

Changes from v1:
Updated to match v3 of the spec, see:
	Subject: [PATCHv3 RFC] virtio-spec: flexible configuration layout
	Message-ID: <20111110122436.GA13144@redhat.com>
	In-Reply-To: <20111109195901.GA28155@redhat.com>

In particular:
- split ISR out
- reorg capability
- add offset alignment

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci.c |  184 +++++++++++++++++++++++++++++++++++++++---
 include/asm-generic/io.h    |    4 +
 include/asm-generic/iomap.h |   11 +++
 include/linux/pci_regs.h    |    4 +
 include/linux/virtio_pci.h  |   41 ++++++++++
 lib/iomap.c                 |   40 ++++++++-
 6 files changed, 265 insertions(+), 19 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index ecb9254..bdd3876 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -37,8 +37,12 @@ struct virtio_pci_device
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
-	/* the IO mapping for the PCI config space */
+	/* the IO address for the common PCI config space */
 	void __iomem *ioaddr;
+	/* the IO address for device specific config */
+	void __iomem *ioaddr_device;
+	/* the IO address to use for notifications operations */
+	void __iomem *ioaddr_notify;
 
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;
@@ -57,8 +61,158 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+
+	/* Various IO mappings: used for resource tracking only. */
+
+	/* Legacy BAR0: typically PIO. */
+	void __iomem *legacy_map;
+
+	/* Mappings specified by device capabilities: typically in MMIO */
+	void __iomem *isr_map;
+	void __iomem *notify_map;
+	void __iomem *common_map;
+	void __iomem *device_map;
 };
 
+/*
+ * With PIO, device-specific config moves as MSI-X is enabled/disabled.
+ * Use this accessor to keep pointer to that config in sync.
+ */
+static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
+{
+	vp_dev->msix_enabled = enabled;
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+}
+
+static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id,
+					u32 align)
+{
+        u32 size;
+        u32 offset;
+        u8 bir;
+        u8 cap_len;
+	int pos;
+	struct pci_dev *dev = vp_dev->pci_dev;
+	void __iomem *p;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 id;
+		pci_read_config_byte(dev, pos + PCI_VNDR_CAP_LEN, &cap_len);
+		if (cap_len < VIRTIO_PCI_CAP_ID + 1)
+			continue;
+		pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_ID, &id);
+		id >>= VIRTIO_PCI_CAP_ID_SHIFT;
+		id &= VIRTIO_PCI_CAP_ID_MASK;
+		if (id == cap_id)
+			break;
+	}
+
+	if (pos <= 0)
+		return NULL;
+
+	if (cap_len < VIRTIO_PCI_CAP_CFG_BIR + 1)
+		goto err;
+        pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_CFG_BIR, &bir);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_OFF + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_OFF, &offset);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_SIZE + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_SIZE, &size);
+        bir >>= VIRTIO_PCI_CAP_CFG_BIR_SHIFT;
+        bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
+        size >>= VIRTIO_PCI_CAP_CFG_SIZE_SHIFT;
+        size &= VIRTIO_PCI_CAP_CFG_SIZE_MASK;
+        off >>= VIRTIO_PCI_CAP_CFG_OFF_SHIFT;
+        off &= VIRTIO_PCI_CAP_CFG_OFF_MASK;
+	/* Align offset appropriately */
+	off &= ~(align - 1);
+
+	/* It's possible for a device to supply a huge config space,
+	 * but we'll never need to map more than a page ATM. */
+	p = pci_iomap_range(dev, bir, offset, size, PAGE_SIZE);
+	if (!p)
+		dev_err(&vp_dev->vdev.dev, "Unable to map virtio pci memory");
+	return p;
+err:
+	dev_err(&vp_dev->vdev.dev, "Unable to parse virtio pci capability");
+	return NULL;
+}
+
+static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
+{
+	if (vp_dev->legacy_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
+	if (vp_dev->isr_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->isr_map);
+	if (vp_dev->notify_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->notify_map);
+	if (vp_dev->common_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->common_map);
+	if (vp_dev->device_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->device_map);
+}
+
+static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
+{
+	vp_dev->isr_map = virtio_pci_map_cfg(vp_dev,
+					     VIRTIO_PCI_CAP_ISR_CFG,
+					     sizeof(u8));
+	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
+						VIRTIO_PCI_CAP_NOTIFY_CFG,
+						sizeof(u16));
+	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
+						VIRTIO_PCI_CAP_COMMON_CFG,
+						sizeof(u32));
+	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
+						VIRTIO_PCI_CAP_DEVICE_CFG,
+						sizeof(u8));
+
+	if (!vp_dev->notify_map || !vp_dev->common_map ||
+	    !vp_dev->device_map) {
+		/*
+		 * If not all capabilities present, map legacy PIO.
+		 * Legacy access is at BAR 0. We never need to map
+		 * more than 256 bytes there, since legacy config space
+		 * used PIO which has this size limit.
+		 * */
+		vp_dev->legacy_map = pci_iomap(vp_dev->pci_dev, 0, 256);
+		if (!vp_dev->legacy_map) {
+			dev_err(&vp_dev->vdev.dev, "Unable to map legacy PIO");
+			goto err;
+		}
+	}
+
+	/* Prefer MMIO if available. If not, fallback to legacy PIO. */
+	if (vp_dev->common_map)
+		vp_dev->ioaddr = vp_dev->common_map;
+	else
+		vp_dev->ioaddr = vp_dev->legacy_map;
+
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+
+	if (vp_dev->notify_map)
+		vp_dev->ioaddr_notify = vp_dev->notify_map;
+	else
+		vp_dev->ioaddr_notify = vp_dev->legacy_map +
+			VIRTIO_PCI_QUEUE_NOTIFY;
+
+	return 0;
+err:
+	virtio_pci_iounmap(vp_dev);
+	return -EINVAL;
+}
+
 /* Constants for MSI-X */
 /* Use first vector for configuration changes, second and the rest for
  * virtqueues Thus, we need at least 2 vectors for MSI. */
@@ -130,8 +284,7 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 		   void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void __iomem *ioaddr = vp_dev->ioaddr +
-				VIRTIO_PCI_CONFIG(vp_dev) + offset;
+	void __iomem *ioaddr = vp_dev->ioaddr_device + offset;
 	u8 *ptr = buf;
 	int i;
 
@@ -145,8 +298,7 @@ static void vp_set(struct virtio_device *vdev, unsigned offset,
 		   const void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void __iomem *ioaddr = vp_dev->ioaddr +
-				VIRTIO_PCI_CONFIG(vp_dev) + offset;
+	void __iomem *ioaddr = vp_dev->ioaddr_device + offset;
 	const u8 *ptr = buf;
 	int i;
 
@@ -184,7 +336,7 @@ static void vp_notify(struct virtqueue *vq)
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
-	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+	iowrite16(info->queue_index, vp_dev->ioaddr_notify);
 }
 
 /* Handle a configuration change: Tell driver if it wants to know. */
@@ -231,7 +383,8 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
 
 	/* reading the ISR has the effect of also clearing it so it's very
 	 * important to save off the value. */
-	isr = ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
+	isr = ioread8(vp_dev->ioaddr_notify +
+		      VIRTIO_PCI_ISR - VIRTIO_PCI_QUEUE_NOTIFY);
 
 	/* It's definitely not us if the ISR was not high */
 	if (!isr)
@@ -265,7 +418,7 @@ static void vp_free_vectors(struct virtio_device *vdev)
 		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 
 		pci_disable_msix(vp_dev->pci_dev);
-		vp_dev->msix_enabled = 0;
+                virtio_pci_set_msix_enabled(vp_dev, 0);
 		vp_dev->msix_vectors = 0;
 	}
 
@@ -303,7 +456,7 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 	if (err)
 		goto error;
 	vp_dev->msix_vectors = nvectors;
-	vp_dev->msix_enabled = 1;
+        virtio_pci_set_msix_enabled(vp_dev, 1);
 
 	/* Set the vector used for configuration */
 	v = vp_dev->msix_used_vectors;
@@ -447,7 +600,10 @@ static void vp_del_vq(struct virtqueue *vq)
 		iowrite16(VIRTIO_MSI_NO_VECTOR,
 			  vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		/* Flush the write out to device */
-		ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
+		ioread8(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		/* And clear ISR: TODO: really needed? */
+		ioread8(vp_dev->ioaddr_notify +
+		      VIRTIO_PCI_ISR - VIRTIO_PCI_QUEUE_NOTIFY);
 	}
 
 	vring_del_virtqueue(vq);
@@ -638,8 +794,8 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	if (err)
 		goto out_enable_device;
 
-	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
-	if (vp_dev->ioaddr == NULL)
+	err = virtio_pci_iomap(vp_dev);
+	if (err)
 		goto out_req_regions;
 
 	pci_set_drvdata(pci_dev, vp_dev);
@@ -661,7 +817,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 
 out_set_drvdata:
 	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	virtio_pci_iounmap(vp_dev);
 out_req_regions:
 	pci_release_regions(pci_dev);
 out_enable_device:
@@ -679,7 +835,7 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 
 	vp_del_vqs(&vp_dev->vdev);
 	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	virtio_pci_iounmap(vp_dev);
 	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 	kfree(vp_dev);
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 9120887..3cf1787 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -286,6 +286,10 @@ static inline void writesb(const void __iomem *addr, const void *buf, int len)
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 struct pci_dev;
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                                     unsigned offset,
+                                     unsigned long minlen,
+                                     unsigned long maxlen);
 static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
 {
 }
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 98dcd76..6f192d4 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -70,8 +70,19 @@ extern void ioport_unmap(void __iomem *);
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 struct pci_dev;
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                                     unsigned offset,
+                                     unsigned long minlen,
+                                     unsigned long maxlen);
 extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
 #else
+static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                                            unsigned offset,
+                                            unsigned long minlen,
+                                            unsigned long maxlen)
+{
+	return NULL;
+}
 struct pci_dev;
 static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
 {
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index e884096..2ec9f81 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -375,6 +375,10 @@
 #define  PCI_X_STATUS_266MHZ	0x40000000	/* 266 MHz capable */
 #define  PCI_X_STATUS_533MHZ	0x80000000	/* 533 MHz capable */
 
+/* Vendor specific capability */
+#define PCI_VNDR_CAP_LEN	2	/* Capability length (8 bits), including
+					   bytes: ID, NEXT and LEN itself. */
+
 /* PCI Bridge Subsystem ID registers */
 
 #define PCI_SSVID_VENDOR_ID     4	/* PCI-Bridge subsystem vendor id register */
diff --git a/include/linux/virtio_pci.h b/include/linux/virtio_pci.h
index ea66f3f..d6568e7 100644
--- a/include/linux/virtio_pci.h
+++ b/include/linux/virtio_pci.h
@@ -92,4 +92,45 @@
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN		4096
+
+/*
+ * Layout for Virtio PCI vendor specific capability (little-endian):
+ * 5 bit virtio capability id.
+ * 3 bit BAR index register, specifying which BAR to use.
+ * 4 byte cfg offset within the BAR.
+ * 4 byte cfg size.
+ */
+
+/* A single virtio device has multiple vendor specific capabilities, we use the
+ * 5 bit ID field to distinguish between these. */
+#define VIRTIO_PCI_CAP_ID		3
+#define VIRTIO_PCI_CAP_ID_MASK		0xff
+#define VIRTIO_PCI_CAP_ID_SHIFT		0
+
+/* IDs for different capabilities. If a specific configuration
+ * is missing, legacy PIO path is used. */
+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	1
+/* Notifications */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG	2
+/* ISR access */
+#define VIRTIO_PCI_CAP_ISR_CFG		3
+/* Device specific confiuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG	4
+
+/* Index of the BAR including this configuration */
+#define VIRTIO_PCI_CAP_CFG_BIR		4
+#define VIRTIO_PCI_CAP_CFG_BIR_MASK	(0x7)
+#define VIRTIO_PCI_CAP_CFG_BIR_SHIFT	0
+
+/* Size of the configuration space */
+#define VIRTIO_PCI_CAP_CFG_SIZE		4
+#define VIRTIO_PCI_CAP_CFG_SIZE_MASK	0xffffff
+#define VIRTIO_PCI_CAP_CFG_SIZE_SHIFT	8
+
+/* Offset within the BAR */
+#define VIRTIO_PCI_CAP_CFG_OFF		8
+#define VIRTIO_PCI_CAP_CFG_OFF_MASK	0xffffffff
+#define VIRTIO_PCI_CAP_CFG_OFF_SHIFT	0
+
 #endif
diff --git a/lib/iomap.c b/lib/iomap.c
index 5dbcb4b..f28720e 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -243,26 +243,36 @@ EXPORT_SYMBOL(ioport_unmap);
 
 #ifdef CONFIG_PCI
 /**
- * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
  * @bar: BAR number
- * @maxlen: length of the memory to map
+ * @offset: map memory at the given offset in BAR
+ * @minlen: min length of the memory to map
+ * @maxlen: max length of the memory to map
  *
  * Using this function you will get a __iomem address to your device BAR.
  * You can access it using ioread*() and iowrite*(). These functions hide
  * the details if this is a MMIO or PIO address space and will just do what
  * you expect from them in the correct way.
  *
+ * @minlen specifies the minimum length to map. We check that BAR is
+ * large enough.
  * @maxlen specifies the maximum length to map. If you want to get access to
- * the complete BAR without checking for its length first, pass %0 here.
+ * the complete BAR from offset to the end, pass %0 here.
  * */
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                              unsigned offset,
+                              unsigned long minlen,
+                              unsigned long maxlen)
 {
 	resource_size_t start = pci_resource_start(dev, bar);
 	resource_size_t len = pci_resource_len(dev, bar);
 	unsigned long flags = pci_resource_flags(dev, bar);
 
-	if (!len || !start)
+	if (len <= offset || !start)
+		return NULL;
+        len -= offset;
+        if (len < minlen)
 		return NULL;
 	if (maxlen && len > maxlen)
 		len = maxlen;
@@ -277,10 +287,30 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 	return NULL;
 }
 
+/**
+ * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+    return pci_iomap_range(dev, bar, 0, 0, maxlen);
+}
+
 void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
 {
 	IO_COND(addr, /* nothing */, iounmap(addr));
 }
 EXPORT_SYMBOL(pci_iomap);
+EXPORT_SYMBOL(pci_iomap_range);
 EXPORT_SYMBOL(pci_iounmap);
 #endif /* CONFIG_PCI */
-- 
1.7.5.53.gc233e

[RFC 0/5] virtio-pci, kvm tools: Support new virtio-pci spec in kvm tools

[Sasha Levin]

This patch series contains two fixes for the RFC patch send by Michael. These
patches are pretty basic and should probably be merged into the next version
of that patch.

Other two patches add support to the new virtio-pci spec in KVM tools, allowing
for easy testing of any future virtio-pci kernel side patches. The usermode code
isn't complete, but it's working properly and should be enough for functionality
testing.

Michael S. Tsirkin (1):
  virtio-pci: flexible configuration layout

Sasha Levin (4):
  virtio-pci: Fix compilation issue
  iomap: Don't ignore offset
  kvm tools: Free up the MSI-X PBA BAR
  kvm tools: Support new virtio-pci configuration layout

 drivers/virtio/virtio_pci.c        |  184 ++++++++++++++++++++++--
 include/asm-generic/io.h           |    4 +
 include/asm-generic/iomap.h        |   11 ++
 include/linux/pci_regs.h           |    4 +
 include/linux/virtio_pci.h         |   41 ++++++
 lib/iomap.c                        |   41 +++++-
 tools/kvm/include/kvm/pci.h        |   13 ++-
 tools/kvm/include/kvm/virtio-pci.h |    5 +-
 tools/kvm/virtio/pci.c             |  275 ++++++++++++++++++++++++++++++++----
 9 files changed, 530 insertions(+), 48 deletions(-)

-- 
1.7.8.rc1

[RFC 1/5] virtio-pci: flexible configuration layout

[Sasha Levin]

From: "Michael S. Tsirkin" <mst@redhat.com>

Add a flexible mechanism to specify virtio configuration layout, using
pci vendor-specific capability.  A separate capability is used for each
of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes
also need arch updates for non-x86.
There might also be more spec changes.

Posting here for early feedback, and to allow Sasha to
proceed with his "kvm tool" work.

Changes from v1:
Updated to match v3 of the spec, see:
	Subject: [PATCHv3 RFC] virtio-spec: flexible configuration layout
	Message-ID: <20111110122436.GA13144@redhat.com>
	In-Reply-To: <20111109195901.GA28155@redhat.com>

In particular:
- split ISR out
- reorg capability
- add offset alignment

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci.c |  184 +++++++++++++++++++++++++++++++++++++++---
 include/asm-generic/io.h    |    4 +
 include/asm-generic/iomap.h |   11 +++
 include/linux/pci_regs.h    |    4 +
 include/linux/virtio_pci.h  |   41 ++++++++++
 lib/iomap.c                 |   40 ++++++++-
 6 files changed, 265 insertions(+), 19 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d1bf41..7625434 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -37,8 +37,12 @@ struct virtio_pci_device
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
-	/* the IO mapping for the PCI config space */
+	/* the IO address for the common PCI config space */
 	void __iomem *ioaddr;
+	/* the IO address for device specific config */
+	void __iomem *ioaddr_device;
+	/* the IO address to use for notifications operations */
+	void __iomem *ioaddr_notify;
 
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;
@@ -57,8 +61,158 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+
+	/* Various IO mappings: used for resource tracking only. */
+
+	/* Legacy BAR0: typically PIO. */
+	void __iomem *legacy_map;
+
+	/* Mappings specified by device capabilities: typically in MMIO */
+	void __iomem *isr_map;
+	void __iomem *notify_map;
+	void __iomem *common_map;
+	void __iomem *device_map;
 };
 
+/*
+ * With PIO, device-specific config moves as MSI-X is enabled/disabled.
+ * Use this accessor to keep pointer to that config in sync.
+ */
+static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
+{
+	vp_dev->msix_enabled = enabled;
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+}
+
+static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id,
+					u32 align)
+{
+        u32 size;
+        u32 offset;
+        u8 bir;
+        u8 cap_len;
+	int pos;
+	struct pci_dev *dev = vp_dev->pci_dev;
+	void __iomem *p;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 id;
+		pci_read_config_byte(dev, pos + PCI_VNDR_CAP_LEN, &cap_len);
+		if (cap_len < VIRTIO_PCI_CAP_ID + 1)
+			continue;
+		pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_ID, &id);
+		id >>= VIRTIO_PCI_CAP_ID_SHIFT;
+		id &= VIRTIO_PCI_CAP_ID_MASK;
+		if (id == cap_id)
+			break;
+	}
+
+	if (pos <= 0)
+		return NULL;
+
+	if (cap_len < VIRTIO_PCI_CAP_CFG_BIR + 1)
+		goto err;
+        pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_CFG_BIR, &bir);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_OFF + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_OFF, &offset);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_SIZE + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_SIZE, &size);
+        bir >>= VIRTIO_PCI_CAP_CFG_BIR_SHIFT;
+        bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
+        size >>= VIRTIO_PCI_CAP_CFG_SIZE_SHIFT;
+        size &= VIRTIO_PCI_CAP_CFG_SIZE_MASK;
+        off >>= VIRTIO_PCI_CAP_CFG_OFF_SHIFT;
+        off &= VIRTIO_PCI_CAP_CFG_OFF_MASK;
+	/* Align offset appropriately */
+	off &= ~(align - 1);
+
+	/* It's possible for a device to supply a huge config space,
+	 * but we'll never need to map more than a page ATM. */
+	p = pci_iomap_range(dev, bir, offset, size, PAGE_SIZE);
+	if (!p)
+		dev_err(&vp_dev->vdev.dev, "Unable to map virtio pci memory");
+	return p;
+err:
+	dev_err(&vp_dev->vdev.dev, "Unable to parse virtio pci capability");
+	return NULL;
+}
+
+static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
+{
+	if (vp_dev->legacy_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
+	if (vp_dev->isr_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->isr_map);
+	if (vp_dev->notify_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->notify_map);
+	if (vp_dev->common_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->common_map);
+	if (vp_dev->device_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->device_map);
+}
+
+static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
+{
+	vp_dev->isr_map = virtio_pci_map_cfg(vp_dev,
+					     VIRTIO_PCI_CAP_ISR_CFG,
+					     sizeof(u8));
+	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
+						VIRTIO_PCI_CAP_NOTIFY_CFG,
+						sizeof(u16));
+	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
+						VIRTIO_PCI_CAP_COMMON_CFG,
+						sizeof(u32));
+	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
+						VIRTIO_PCI_CAP_DEVICE_CFG,
+						sizeof(u8));
+
+	if (!vp_dev->notify_map || !vp_dev->common_map ||
+	    !vp_dev->device_map) {
+		/*
+		 * If not all capabilities present, map legacy PIO.
+		 * Legacy access is at BAR 0. We never need to map
+		 * more than 256 bytes there, since legacy config space
+		 * used PIO which has this size limit.
+		 * */
+		vp_dev->legacy_map = pci_iomap(vp_dev->pci_dev, 0, 256);
+		if (!vp_dev->legacy_map) {
+			dev_err(&vp_dev->vdev.dev, "Unable to map legacy PIO");
+			goto err;
+		}
+	}
+
+	/* Prefer MMIO if available. If not, fallback to legacy PIO. */
+	if (vp_dev->common_map)
+		vp_dev->ioaddr = vp_dev->common_map;
+	else
+		vp_dev->ioaddr = vp_dev->legacy_map;
+
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+
+	if (vp_dev->notify_map)
+		vp_dev->ioaddr_notify = vp_dev->notify_map;
+	else
+		vp_dev->ioaddr_notify = vp_dev->legacy_map +
+			VIRTIO_PCI_QUEUE_NOTIFY;
+
+	return 0;
+err:
+	virtio_pci_iounmap(vp_dev);
+	return -EINVAL;
+}
+
 /* Constants for MSI-X */
 /* Use first vector for configuration changes, second and the rest for
  * virtqueues Thus, we need at least 2 vectors for MSI. */
@@ -130,8 +284,7 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 		   void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void __iomem *ioaddr = vp_dev->ioaddr +
-				VIRTIO_PCI_CONFIG(vp_dev) + offset;
+	void __iomem *ioaddr = vp_dev->ioaddr_device + offset;
 	u8 *ptr = buf;
 	int i;
 
@@ -145,8 +298,7 @@ static void vp_set(struct virtio_device *vdev, unsigned offset,
 		   const void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void __iomem *ioaddr = vp_dev->ioaddr +
-				VIRTIO_PCI_CONFIG(vp_dev) + offset;
+	void __iomem *ioaddr = vp_dev->ioaddr_device + offset;
 	const u8 *ptr = buf;
 	int i;
 
@@ -184,7 +336,7 @@ static void vp_notify(struct virtqueue *vq)
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
-	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+	iowrite16(info->queue_index, vp_dev->ioaddr_notify);
 }
 
 /* Handle a configuration change: Tell driver if it wants to know. */
@@ -231,7 +383,8 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
 
 	/* reading the ISR has the effect of also clearing it so it's very
 	 * important to save off the value. */
-	isr = ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
+	isr = ioread8(vp_dev->ioaddr_notify +
+		      VIRTIO_PCI_ISR - VIRTIO_PCI_QUEUE_NOTIFY);
 
 	/* It's definitely not us if the ISR was not high */
 	if (!isr)
@@ -265,7 +418,7 @@ static void vp_free_vectors(struct virtio_device *vdev)
 		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 
 		pci_disable_msix(vp_dev->pci_dev);
-		vp_dev->msix_enabled = 0;
+                virtio_pci_set_msix_enabled(vp_dev, 0);
 		vp_dev->msix_vectors = 0;
 	}
 
@@ -303,7 +456,7 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 	if (err)
 		goto error;
 	vp_dev->msix_vectors = nvectors;
-	vp_dev->msix_enabled = 1;
+        virtio_pci_set_msix_enabled(vp_dev, 1);
 
 	/* Set the vector used for configuration */
 	v = vp_dev->msix_used_vectors;
@@ -451,7 +604,10 @@ static void vp_del_vq(struct virtqueue *vq)
 		iowrite16(VIRTIO_MSI_NO_VECTOR,
 			  vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		/* Flush the write out to device */
-		ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
+		ioread8(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		/* And clear ISR: TODO: really needed? */
+		ioread8(vp_dev->ioaddr_notify +
+		      VIRTIO_PCI_ISR - VIRTIO_PCI_QUEUE_NOTIFY);
 	}
 
 	vring_del_virtqueue(vq);
@@ -642,8 +798,8 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	if (err)
 		goto out_enable_device;
 
-	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
-	if (vp_dev->ioaddr == NULL)
+	err = virtio_pci_iomap(vp_dev);
+	if (err)
 		goto out_req_regions;
 
 	pci_set_drvdata(pci_dev, vp_dev);
@@ -665,7 +821,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 
 out_set_drvdata:
 	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	virtio_pci_iounmap(vp_dev);
 out_req_regions:
 	pci_release_regions(pci_dev);
 out_enable_device:
@@ -683,7 +839,7 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 
 	vp_del_vqs(&vp_dev->vdev);
 	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	virtio_pci_iounmap(vp_dev);
 	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 	kfree(vp_dev);
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 9120887..3cf1787 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -286,6 +286,10 @@ static inline void writesb(const void __iomem *addr, const void *buf, int len)
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 struct pci_dev;
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                                     unsigned offset,
+                                     unsigned long minlen,
+                                     unsigned long maxlen);
 static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
 {
 }
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 98dcd76..6f192d4 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -70,8 +70,19 @@ extern void ioport_unmap(void __iomem *);
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 struct pci_dev;
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                                     unsigned offset,
+                                     unsigned long minlen,
+                                     unsigned long maxlen);
 extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
 #else
+static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                                            unsigned offset,
+                                            unsigned long minlen,
+                                            unsigned long maxlen)
+{
+	return NULL;
+}
 struct pci_dev;
 static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
 {
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index b5d9657..3e7905c 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -375,6 +375,10 @@
 #define  PCI_X_STATUS_266MHZ	0x40000000	/* 266 MHz capable */
 #define  PCI_X_STATUS_533MHZ	0x80000000	/* 533 MHz capable */
 
+/* Vendor specific capability */
+#define PCI_VNDR_CAP_LEN	2	/* Capability length (8 bits), including
+					   bytes: ID, NEXT and LEN itself. */
+
 /* PCI Bridge Subsystem ID registers */
 
 #define PCI_SSVID_VENDOR_ID     4	/* PCI-Bridge subsystem vendor id register */
diff --git a/include/linux/virtio_pci.h b/include/linux/virtio_pci.h
index ea66f3f..d6568e7 100644
--- a/include/linux/virtio_pci.h
+++ b/include/linux/virtio_pci.h
@@ -92,4 +92,45 @@
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN		4096
+
+/*
+ * Layout for Virtio PCI vendor specific capability (little-endian):
+ * 5 bit virtio capability id.
+ * 3 bit BAR index register, specifying which BAR to use.
+ * 4 byte cfg offset within the BAR.
+ * 4 byte cfg size.
+ */
+
+/* A single virtio device has multiple vendor specific capabilities, we use the
+ * 5 bit ID field to distinguish between these. */
+#define VIRTIO_PCI_CAP_ID		3
+#define VIRTIO_PCI_CAP_ID_MASK		0xff
+#define VIRTIO_PCI_CAP_ID_SHIFT		0
+
+/* IDs for different capabilities. If a specific configuration
+ * is missing, legacy PIO path is used. */
+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	1
+/* Notifications */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG	2
+/* ISR access */
+#define VIRTIO_PCI_CAP_ISR_CFG		3
+/* Device specific confiuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG	4
+
+/* Index of the BAR including this configuration */
+#define VIRTIO_PCI_CAP_CFG_BIR		4
+#define VIRTIO_PCI_CAP_CFG_BIR_MASK	(0x7)
+#define VIRTIO_PCI_CAP_CFG_BIR_SHIFT	0
+
+/* Size of the configuration space */
+#define VIRTIO_PCI_CAP_CFG_SIZE		4
+#define VIRTIO_PCI_CAP_CFG_SIZE_MASK	0xffffff
+#define VIRTIO_PCI_CAP_CFG_SIZE_SHIFT	8
+
+/* Offset within the BAR */
+#define VIRTIO_PCI_CAP_CFG_OFF		8
+#define VIRTIO_PCI_CAP_CFG_OFF_MASK	0xffffffff
+#define VIRTIO_PCI_CAP_CFG_OFF_SHIFT	0
+
 #endif
diff --git a/lib/iomap.c b/lib/iomap.c
index 5dbcb4b..f28720e 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -243,26 +243,36 @@ EXPORT_SYMBOL(ioport_unmap);
 
 #ifdef CONFIG_PCI
 /**
- * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
  * @bar: BAR number
- * @maxlen: length of the memory to map
+ * @offset: map memory at the given offset in BAR
+ * @minlen: min length of the memory to map
+ * @maxlen: max length of the memory to map
  *
  * Using this function you will get a __iomem address to your device BAR.
  * You can access it using ioread*() and iowrite*(). These functions hide
  * the details if this is a MMIO or PIO address space and will just do what
  * you expect from them in the correct way.
  *
+ * @minlen specifies the minimum length to map. We check that BAR is
+ * large enough.
  * @maxlen specifies the maximum length to map. If you want to get access to
- * the complete BAR without checking for its length first, pass %0 here.
+ * the complete BAR from offset to the end, pass %0 here.
  * */
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+                              unsigned offset,
+                              unsigned long minlen,
+                              unsigned long maxlen)
 {
 	resource_size_t start = pci_resource_start(dev, bar);
 	resource_size_t len = pci_resource_len(dev, bar);
 	unsigned long flags = pci_resource_flags(dev, bar);
 
-	if (!len || !start)
+	if (len <= offset || !start)
+		return NULL;
+        len -= offset;
+        if (len < minlen)
 		return NULL;
 	if (maxlen && len > maxlen)
 		len = maxlen;
@@ -277,10 +287,30 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 	return NULL;
 }
 
+/**
+ * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+    return pci_iomap_range(dev, bar, 0, 0, maxlen);
+}
+
 void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
 {
 	IO_COND(addr, /* nothing */, iounmap(addr));
 }
 EXPORT_SYMBOL(pci_iomap);
+EXPORT_SYMBOL(pci_iomap_range);
 EXPORT_SYMBOL(pci_iounmap);
 #endif /* CONFIG_PCI */
-- 
1.7.8.rc1

[RFC 2/5] virtio-pci: Fix compilation issue

[Sasha Levin]

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 drivers/virtio/virtio_pci.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 7625434..d242fcc 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -129,10 +129,10 @@ static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap
         bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
         size >>= VIRTIO_PCI_CAP_CFG_SIZE_SHIFT;
         size &= VIRTIO_PCI_CAP_CFG_SIZE_MASK;
-        off >>= VIRTIO_PCI_CAP_CFG_OFF_SHIFT;
-        off &= VIRTIO_PCI_CAP_CFG_OFF_MASK;
+        offset >>= VIRTIO_PCI_CAP_CFG_OFF_SHIFT;
+        offset &= VIRTIO_PCI_CAP_CFG_OFF_MASK;
 	/* Align offset appropriately */
-	off &= ~(align - 1);
+	offset &= ~(align - 1);
 
 	/* It's possible for a device to supply a huge config space,
 	 * but we'll never need to map more than a page ATM. */
-- 
1.7.8.rc1

[RFC 3/5] iomap: Don't ignore offset

[Sasha Levin]

Offset was ignored for start calulcations, making all mappings start at
offset 0 always.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 lib/iomap.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/iomap.c b/lib/iomap.c
index f28720e..93ae915 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -272,6 +272,7 @@ void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
 	if (len <= offset || !start)
 		return NULL;
         len -= offset;
+	start += offset;
         if (len < minlen)
 		return NULL;
 	if (maxlen && len > maxlen)
-- 
1.7.8.rc1

[RFC 4/5] kvm tools: Free up the MSI-X PBA BAR

[Sasha Levin]

Free up the BAR to make space for the new virtio BARs. It isn't required
to have the PBA and the table in the separate BARs, and uniting them will
just give us extra BARs to play with.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/virtio-pci.h |    1 -
 tools/kvm/virtio/pci.c             |   35 ++++++++++++++---------------------
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/tools/kvm/include/kvm/virtio-pci.h b/tools/kvm/include/kvm/virtio-pci.h
index 2bbb271..73f7486 100644
--- a/tools/kvm/include/kvm/virtio-pci.h
+++ b/tools/kvm/include/kvm/virtio-pci.h
@@ -30,7 +30,6 @@ struct virtio_pci {
 	u32			vq_vector[VIRTIO_PCI_MAX_VQ];
 	u32			gsis[VIRTIO_PCI_MAX_VQ];
 	u32			msix_io_block;
-	u32			msix_pba_block;
 	u64			msix_pba;
 	struct msix_table	msix_table[VIRTIO_PCI_MAX_VQ + VIRTIO_PCI_MAX_CONFIG];
 
diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
index 1660f06..da38ba5 100644
--- a/tools/kvm/virtio/pci.c
+++ b/tools/kvm/virtio/pci.c
@@ -214,23 +214,21 @@ static struct ioport_operations virtio_pci__io_ops = {
 static void callback_mmio_table(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr)
 {
 	struct virtio_pci *vpci = ptr;
-	void *table = &vpci->msix_table;
+	void *table;
+	u32 offset;
 
-	if (is_write)
-		memcpy(table + addr - vpci->msix_io_block, data, len);
-	else
-		memcpy(data, table + addr - vpci->msix_io_block, len);
-}
-
-static void callback_mmio_pba(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr)
-{
-	struct virtio_pci *vpci = ptr;
-	void *pba = &vpci->msix_pba;
+	if (addr > vpci->msix_io_block + PCI_IO_SIZE) {
+		table	= &vpci->msix_pba;
+		offset	= vpci->msix_io_block + PCI_IO_SIZE;
+	} else {
+		table	= &vpci->msix_table;
+		offset	= vpci->msix_io_block;
+	}
 
 	if (is_write)
-		memcpy(pba + addr - vpci->msix_pba_block, data, len);
+		memcpy(table + addr - offset, data, len);
 	else
-		memcpy(data, pba + addr - vpci->msix_pba_block, len);
+		memcpy(data, table + addr - offset, len);
 }
 
 int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_trans *vtrans, u32 vq)
@@ -283,12 +281,10 @@ int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
 	u8 pin, line, ndev;
 
 	vpci->dev = dev;
-	vpci->msix_io_block = pci_get_io_space_block(PCI_IO_SIZE);
-	vpci->msix_pba_block = pci_get_io_space_block(PCI_IO_SIZE);
+	vpci->msix_io_block = pci_get_io_space_block(PCI_IO_SIZE * 2);
 
 	vpci->base_addr = ioport__register(IOPORT_EMPTY, &virtio_pci__io_ops, IOPORT_SIZE, vtrans);
 	kvm__register_mmio(kvm, vpci->msix_io_block, 0x100, callback_mmio_table, vpci);
-	kvm__register_mmio(kvm, vpci->msix_pba_block, 0x100, callback_mmio_pba, vpci);
 
 	vpci->pci_hdr = (struct pci_device_header) {
 		.vendor_id		= PCI_VENDOR_ID_REDHAT_QUMRANET,
@@ -299,10 +295,7 @@ int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
 		.subsys_vendor_id	= PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
 		.subsys_id		= subsys_id,
 		.bar[0]			= vpci->base_addr | PCI_BASE_ADDRESS_SPACE_IO,
-		.bar[1]			= vpci->msix_io_block | PCI_BASE_ADDRESS_SPACE_MEMORY
-					| PCI_BASE_ADDRESS_MEM_TYPE_64,
-		.bar[3]			= vpci->msix_pba_block | PCI_BASE_ADDRESS_SPACE_MEMORY
-					| PCI_BASE_ADDRESS_MEM_TYPE_64,
+		.bar[1]			= vpci->msix_io_block | PCI_BASE_ADDRESS_SPACE_MEMORY,
 		.status			= PCI_STATUS_CAP_LIST,
 		.capabilities		= (void *)&vpci->pci_hdr.msix - (void *)&vpci->pci_hdr,
 	};
@@ -327,7 +320,7 @@ int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
 	 * we're not in short of BARs
 	 */
 	vpci->pci_hdr.msix.table_offset = 1; /* Use BAR 1 */
-	vpci->pci_hdr.msix.pba_offset = 3; /* Use BAR 3 */
+	vpci->pci_hdr.msix.pba_offset = 1 | PCI_IO_SIZE; /* Use BAR 1 with offset */
 	vpci->config_vector = 0;
 
 	if (irq__register_device(subsys_id, &ndev, &pin, &line) < 0)
-- 
1.7.8.rc1

[RFC 5/5] kvm tools: Support new virtio-pci configuration layout

[Sasha Levin]

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/pci.h        |   13 ++-
 tools/kvm/include/kvm/virtio-pci.h |    4 +
 tools/kvm/virtio/pci.c             |  248 ++++++++++++++++++++++++++++++++++--
 3 files changed, 254 insertions(+), 11 deletions(-)

diff --git a/tools/kvm/include/kvm/pci.h b/tools/kvm/include/kvm/pci.h
index f71af0b..4f5a09f 100644
--- a/tools/kvm/include/kvm/pci.h
+++ b/tools/kvm/include/kvm/pci.h
@@ -39,6 +39,16 @@ struct msix_cap {
 	u32 pba_offset;
 };
 
+struct virtio_cap {
+	u8 cap;
+	u8 next;
+	u8 cap_len;
+	u8 structure_id;
+	u8 bir;
+	u32 size:24;
+	u32 offset;
+};
+
 struct pci_device_header {
 	u16		vendor_id;
 	u16		device_id;
@@ -63,7 +73,8 @@ struct pci_device_header {
 	u8		min_gnt;
 	u8		max_lat;
 	struct msix_cap msix;
-	u8		empty[136]; /* Rest of PCI config space */
+	struct virtio_cap virtio[4];
+	u8		empty[90]; /* Rest of PCI config space */
 	u32		bar_size[6];
 };
 
diff --git a/tools/kvm/include/kvm/virtio-pci.h b/tools/kvm/include/kvm/virtio-pci.h
index 73f7486..fc3c03f 100644
--- a/tools/kvm/include/kvm/virtio-pci.h
+++ b/tools/kvm/include/kvm/virtio-pci.h
@@ -19,6 +19,7 @@ struct virtio_pci_ioevent_param {
 struct virtio_pci {
 	struct pci_device_header pci_hdr;
 	void			*dev;
+	struct kvm		*kvm;
 
 	u16			base_addr;
 	u8			status;
@@ -36,6 +37,9 @@ struct virtio_pci {
 	/* virtio queue */
 	u16			queue_selector;
 	struct virtio_pci_ioevent_param ioeventfds[VIRTIO_PCI_MAX_VQ];
+
+	u32			virtio_mmio;
+	u16			virtio_pio;
 };
 
 int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
index da38ba5..8606d87 100644
--- a/tools/kvm/virtio/pci.c
+++ b/tools/kvm/virtio/pci.c
@@ -40,7 +40,7 @@ static int virtio_pci__init_ioeventfd(struct kvm *kvm, struct virtio_trans *vtra
 	};
 
 	ioevent = (struct ioevent) {
-		.io_addr	= vpci->base_addr + VIRTIO_PCI_QUEUE_NOTIFY,
+		.io_addr	= vpci->virtio_pio,
 		.io_len		= sizeof(u16),
 		.fn		= virtio_pci__ioevent_callback,
 		.fn_ptr		= &vpci->ioeventfds[vq],
@@ -59,7 +59,7 @@ static inline bool virtio_pci__msix_enabled(struct virtio_pci *vpci)
 	return vpci->pci_hdr.msix.ctrl & PCI_MSIX_FLAGS_ENABLE;
 }
 
-static bool virtio_pci__specific_io_in(struct kvm *kvm, struct virtio_trans *vtrans, u16 port,
+static bool virtio_pci_legacy__specific_io_in(struct kvm *kvm, struct virtio_trans *vtrans, u16 port,
 					void *data, int size, int offset)
 {
 	u32 config_offset;
@@ -89,7 +89,8 @@ static bool virtio_pci__specific_io_in(struct kvm *kvm, struct virtio_trans *vtr
 	return false;
 }
 
-static bool virtio_pci__io_in(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size)
+static bool virtio_pci_legacy__io_in(struct ioport *ioport, struct kvm *kvm,
+					u16 port, void *data, int size)
 {
 	unsigned long offset;
 	bool ret = true;
@@ -124,15 +125,15 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm *kvm, u16 port,
 		vpci->isr = VIRTIO_IRQ_LOW;
 		break;
 	default:
-		ret = virtio_pci__specific_io_in(kvm, vtrans, port, data, size, offset);
+		ret = virtio_pci_legacy__specific_io_in(kvm, vtrans, port, data, size, offset);
 		break;
 	};
 
 	return ret;
 }
 
-static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_trans *vtrans, u16 port,
-					void *data, int size, int offset)
+static bool virtio_pci_legacy__specific_io_out(struct kvm *kvm, struct virtio_trans *vtrans,
+					u16 port, void *data, int size, int offset)
 {
 	struct virtio_pci *vpci = vtrans->virtio;
 	u32 config_offset, gsi, vec;
@@ -166,7 +167,8 @@ static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_trans *vt
 	return false;
 }
 
-static bool virtio_pci__io_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size)
+static bool virtio_pci_legacy__io_out(struct ioport *ioport,
+				struct kvm *kvm, u16 port, void *data, int size)
 {
 	unsigned long offset;
 	bool ret = true;
@@ -199,7 +201,76 @@ static bool virtio_pci__io_out(struct ioport *ioport, struct kvm *kvm, u16 port,
 		vpci->status		= ioport__read8(data);
 		break;
 	default:
-		ret = virtio_pci__specific_io_out(kvm, vtrans, port, data, size, offset);
+		ret = virtio_pci_legacy__specific_io_out(kvm, vtrans, port,
+							data, size, offset);
+		break;
+	};
+
+	return ret;
+}
+
+static struct ioport_operations virtio_pci_legacy__io_ops = {
+	.io_in	= virtio_pci_legacy__io_in,
+	.io_out	= virtio_pci_legacy__io_out,
+};
+
+static bool virtio_pci__io_out(struct ioport *ioport,
+				struct kvm *kvm, u16 port, void *data, int size)
+{
+	unsigned long offset;
+	bool ret = true;
+	u16 val;
+	struct virtio_pci *vpci;
+	struct virtio_trans *vtrans;
+
+	vtrans = ioport->priv;
+	vpci = vtrans->virtio;
+	offset = port - vpci->virtio_pio;
+
+	/*
+	 * 0-1: Notifications
+	 * 2: ISR
+	 */
+	switch (offset) {
+	case 0:
+		val		= ioport__read16(data);
+		vtrans->virtio_ops->notify_vq(kvm, vpci->dev, val);
+		break;
+	case 2:
+		vpci->isr	= ioport__read8(data);
+		break;
+	default:
+		ret = false;
+		break;
+	}
+
+	return ret;
+}
+
+static bool virtio_pci__io_in(struct ioport *ioport, struct kvm *kvm,
+					u16 port, void *data, int size)
+{
+	unsigned long offset;
+	bool ret = true;
+	struct virtio_pci *vpci;
+
+	vpci = ioport->priv;
+	offset = port - vpci->virtio_pio;
+
+	/*
+	 * 0-1: Notifications
+	 * 2: ISR
+	 */
+	switch (offset) {
+	case 0:
+		/* Shouldn't happen */
+	case 2:
+		ioport__write8(data, vpci->isr);
+		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
+		vpci->isr = VIRTIO_IRQ_LOW;
+		break;
+	default:
+		ret = false;
 		break;
 	};
 
@@ -231,6 +302,113 @@ static void callback_mmio_table(u64 addr, u8 *data, u32 len, u8 is_write, void *
 		memcpy(data, table + addr - offset, len);
 }
 
+static void virtio_mmio_dev_specific(u64 addr, u8 *data, u32 len, u8 is_write,
+					struct virtio_trans *vtrans)
+{
+	struct virtio_pci *vpci = vtrans->virtio;
+	u32 i;
+
+	for (i = 0; i < len; i++) {
+		if (is_write)
+			vtrans->virtio_ops->set_config(vpci->kvm, vpci->dev,
+							*(u8 *)data + i, addr + i);
+		else
+			data[i] =
+				vtrans->virtio_ops->get_config(vpci->kvm, vpci->dev, addr + i);
+	}
+}
+
+static void virtio_mmio_in(u64 addr, u8 *data, u32 len, u8 is_write,
+					struct virtio_trans *vtrans)
+{
+	struct virtio_pci *vpci = vtrans->virtio;
+
+	switch (addr) {
+	case VIRTIO_PCI_HOST_FEATURES:
+		*(u32 *)data  = vtrans->virtio_ops->get_host_features(vpci->kvm, vpci->dev);
+		break;
+	case VIRTIO_PCI_QUEUE_PFN:
+		*(u32 *)data  = vtrans->virtio_ops->get_pfn_vq(vpci->kvm,
+							vpci->dev, vpci->queue_selector);
+		break;
+	case VIRTIO_PCI_QUEUE_NUM:
+		*(u32 *)data  = vtrans->virtio_ops->get_size_vq(vpci->kvm,
+							vpci->dev, vpci->queue_selector);
+		break;
+	case VIRTIO_PCI_STATUS:
+		*(u8 *)data  = vpci->status;
+		break;
+	case VIRTIO_MSI_CONFIG_VECTOR:
+		*(u8 *)data  = vpci->config_vector;
+		break;
+	case VIRTIO_MSI_QUEUE_VECTOR:
+		*(u8 *)data = vpci->vq_vector[vpci->queue_selector];
+		break;
+	};
+}
+
+static void virtio_mmio_out(u64 addr, u8 *data, u32 len, u8 is_write,
+					struct virtio_trans *vtrans)
+{
+	struct virtio_pci *vpci = vtrans->virtio;
+	u32 val;
+
+	switch (addr) {
+	case VIRTIO_PCI_GUEST_FEATURES:
+		val = *(u32 *)data;
+		vtrans->virtio_ops->set_guest_features(vpci->kvm, vpci, val);
+		break;
+	case VIRTIO_PCI_QUEUE_PFN:
+		val = *(u32 *)data;
+		virtio_pci__init_ioeventfd(vpci->kvm, vtrans, vpci->queue_selector);
+		vtrans->virtio_ops->init_vq(vpci->kvm, vpci->dev, vpci->queue_selector, val);
+		break;
+	case VIRTIO_PCI_QUEUE_SEL:
+		vpci->queue_selector	= *(u16 *)data;
+		break;
+	case VIRTIO_PCI_QUEUE_NOTIFY:
+		val			= *(u16 *)data;
+		vtrans->virtio_ops->notify_vq(vpci->kvm, vpci->dev, val);
+		break;
+	case VIRTIO_PCI_STATUS:
+		vpci->status		= *(u8 *)data;
+		break;
+	case VIRTIO_MSI_CONFIG_VECTOR: {
+		u16 vec, gsi;
+
+		vec = *(u16 *)data;
+		gsi = irq__add_msix_route(vpci->kvm, &vpci->msix_table[vec].msg);
+		vpci->config_gsi = gsi;
+		break;
+	}
+	case VIRTIO_MSI_QUEUE_VECTOR: {
+		u16 vec, gsi;
+
+		vec = vpci->vq_vector[vpci->queue_selector] = *(u16 *)data;
+		gsi = irq__add_msix_route(vpci->kvm, &vpci->msix_table[vec].msg);
+		vpci->gsis[vpci->queue_selector] = gsi;
+		break;
+	}
+	};
+}
+
+static void callback_virtio_mmio(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr)
+{
+	struct virtio_trans *vtrans = ptr;
+	struct virtio_pci *vpci = vtrans->virtio;
+	u32 offset = addr - vpci->virtio_mmio;
+
+	if (offset >= 0x100) {
+		virtio_mmio_dev_specific(offset - 0x100, data, len, is_write, vtrans);
+		return;
+	}
+
+	if (is_write == 0)
+		virtio_mmio_in(offset, data, len, is_write, vtrans);
+	else
+		virtio_mmio_out(offset, data, len, is_write, vtrans);
+}
+
 int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_trans *vtrans, u32 vq)
 {
 	struct virtio_pci *vpci = vtrans->virtio;
@@ -282,10 +460,16 @@ int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
 
 	vpci->dev = dev;
 	vpci->msix_io_block = pci_get_io_space_block(PCI_IO_SIZE * 2);
+	vpci->virtio_mmio = pci_get_io_space_block(PCI_IO_SIZE * 2);
 
-	vpci->base_addr = ioport__register(IOPORT_EMPTY, &virtio_pci__io_ops, IOPORT_SIZE, vtrans);
+	vpci->base_addr = ioport__register(IOPORT_EMPTY,
+				&virtio_pci_legacy__io_ops, IOPORT_SIZE, vtrans);
+	vpci->virtio_pio = ioport__register(IOPORT_EMPTY,
+				&virtio_pci__io_ops, IOPORT_SIZE, vtrans);
 	kvm__register_mmio(kvm, vpci->msix_io_block, 0x100, callback_mmio_table, vpci);
+	kvm__register_mmio(kvm, vpci->virtio_mmio, 0x200, callback_virtio_mmio, vtrans);
 
+	vpci->kvm = kvm;
 	vpci->pci_hdr = (struct pci_device_header) {
 		.vendor_id		= PCI_VENDOR_ID_REDHAT_QUMRANET,
 		.device_id		= device_id,
@@ -296,12 +480,16 @@ int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
 		.subsys_id		= subsys_id,
 		.bar[0]			= vpci->base_addr | PCI_BASE_ADDRESS_SPACE_IO,
 		.bar[1]			= vpci->msix_io_block | PCI_BASE_ADDRESS_SPACE_MEMORY,
+		.bar[2]			= vpci->virtio_mmio | PCI_BASE_ADDRESS_SPACE_MEMORY,
+		.bar_size[2]		= PCI_IO_SIZE * 2,
+		.bar[3]			= vpci->virtio_pio | PCI_BASE_ADDRESS_SPACE_IO,
+		.bar_size[3]		= PCI_IO_SIZE * 2,
 		.status			= PCI_STATUS_CAP_LIST,
 		.capabilities		= (void *)&vpci->pci_hdr.msix - (void *)&vpci->pci_hdr,
 	};
 
 	vpci->pci_hdr.msix.cap = PCI_CAP_ID_MSIX;
-	vpci->pci_hdr.msix.next = 0;
+	vpci->pci_hdr.msix.next = (void *)&vpci->pci_hdr.virtio[0] - (void *)&vpci->pci_hdr;
 	/*
 	 * We at most have VIRTIO_PCI_MAX_VQ entries for virt queue,
 	 * VIRTIO_PCI_MAX_CONFIG entries for config.
@@ -321,6 +509,46 @@ int virtio_pci__init(struct kvm *kvm, struct virtio_trans *vtrans, void *dev,
 	 */
 	vpci->pci_hdr.msix.table_offset = 1; /* Use BAR 1 */
 	vpci->pci_hdr.msix.pba_offset = 1 | PCI_IO_SIZE; /* Use BAR 1 with offset */
+
+	vpci->pci_hdr.virtio[0] = (struct virtio_cap) {
+		.cap = PCI_CAP_ID_VNDR,
+		.next = (void *)&vpci->pci_hdr.virtio[1] - (void *)&vpci->pci_hdr,
+		.cap_len = sizeof(struct virtio_cap),
+		.structure_id = VIRTIO_PCI_CAP_COMMON_CFG,
+		.bir = 2,
+		.size = PCI_IO_SIZE,
+		.offset = 0,
+	};
+
+	vpci->pci_hdr.virtio[1] = (struct virtio_cap) {
+		.cap = PCI_CAP_ID_VNDR,
+		.next = (void *)&vpci->pci_hdr.virtio[2] - (void *)&vpci->pci_hdr,
+		.cap_len = sizeof(struct virtio_cap),
+		.structure_id = VIRTIO_PCI_CAP_ISR_CFG,
+		.bir = 3,
+		.size = 1,
+		.offset = 2,
+	};
+
+	vpci->pci_hdr.virtio[2] = (struct virtio_cap) {
+		.cap = PCI_CAP_ID_VNDR,
+		.next = (void *)&vpci->pci_hdr.virtio[3] - (void *)&vpci->pci_hdr,
+		.cap_len = sizeof(struct virtio_cap),
+		.structure_id = VIRTIO_PCI_CAP_NOTIFY_CFG,
+		.bir = 3,
+		.size = 2,
+		.offset = 0,
+	};
+
+	vpci->pci_hdr.virtio[3] = (struct virtio_cap) {
+		.cap = PCI_CAP_ID_VNDR,
+		.next = 0,
+		.cap_len = sizeof(struct virtio_cap),
+		.structure_id = VIRTIO_PCI_CAP_DEVICE_CFG,
+		.bir = 2,
+		.size = PCI_IO_SIZE,
+		.offset = PCI_IO_SIZE,
+	};
 	vpci->config_vector = 0;
 
 	if (irq__register_device(subsys_id, &ndev, &pin, &line) < 0)
-- 
1.7.8.rc1

Re: [RFC 0/5] virtio-pci,kvm tools: Support new virtio-pci spec in kvm tools

[Michael S. Tsirkin]

On Tue, Nov 15, 2011 at 01:43:12AM +0200, Sasha Levin wrote:
> This patch series contains two fixes for the RFC patch send by Michael. These
> patches are pretty basic and should probably be merged into the next version
> of that patch.

Yes, will do, thanks!

> Other two patches add support to the new virtio-pci spec in KVM tools, allowing
> for easy testing of any future virtio-pci kernel side patches. The usermode code
> isn't complete, but it's working properly and should be enough for functionality
> testing.
> 
> Michael S. Tsirkin (1):
>   virtio-pci: flexible configuration layout
> 
> Sasha Levin (4):
>   virtio-pci: Fix compilation issue
>   iomap: Don't ignore offset
>   kvm tools: Free up the MSI-X PBA BAR
>   kvm tools: Support new virtio-pci configuration layout
> 
>  drivers/virtio/virtio_pci.c        |  184 ++++++++++++++++++++++--
>  include/asm-generic/io.h           |    4 +
>  include/asm-generic/iomap.h        |   11 ++
>  include/linux/pci_regs.h           |    4 +
>  include/linux/virtio_pci.h         |   41 ++++++
>  lib/iomap.c                        |   41 +++++-
>  tools/kvm/include/kvm/pci.h        |   13 ++-
>  tools/kvm/include/kvm/virtio-pci.h |    5 +-
>  tools/kvm/virtio/pci.c             |  275 ++++++++++++++++++++++++++++++++----
>  9 files changed, 530 insertions(+), 48 deletions(-)
> 
> -- 
> 1.7.8.rc1

Re: [RFC 1/5] virtio-pci: flexible configuration layout

[Rusty Russell]

On Tue, 15 Nov 2011 01:43:13 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> Add a flexible mechanism to specify virtio configuration layout, using
> pci vendor-specific capability.  A separate capability is used for each
> of common, device specific and data-path accesses.

OK, a couple of minor suggestions:

> +static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
> +{
> +	vp_dev->isr_map = virtio_pci_map_cfg(vp_dev,
> +					     VIRTIO_PCI_CAP_ISR_CFG,
> +					     sizeof(u8));
> +	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
> +						VIRTIO_PCI_CAP_NOTIFY_CFG,
> +						sizeof(u16));
> +	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
> +						VIRTIO_PCI_CAP_COMMON_CFG,
> +						sizeof(u32));
> +	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
> +						VIRTIO_PCI_CAP_DEVICE_CFG,
> +						sizeof(u8));
> +
> +	if (!vp_dev->notify_map || !vp_dev->common_map ||
> +	    !vp_dev->device_map) {
> +		/*
> +		 * If not all capabilities present, map legacy PIO.
> +		 * Legacy access is at BAR 0. We never need to map
> +		 * more than 256 bytes there, since legacy config space
> +		 * used PIO which has this size limit.
> +		 * */
> +		vp_dev->legacy_map = pci_iomap(vp_dev->pci_dev, 0, 256);
> +		if (!vp_dev->legacy_map) {
> +			dev_err(&vp_dev->vdev.dev, "Unable to map legacy PIO");
> +			goto err;
> +		}
> +	}

Please don't mix the two.  Ever, under any circumstances.  If it helps,
put CONFIG_VIRTIO_PCI_LEGACY #ifdefs in there:

	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
						VIRTIO_PCI_CAP_COMMON_CFG,
						sizeof(u32));
        /* Something horribly wrong? */
        if (IS_ERR(vp_dev->common_map)) {
                err = PTR_ERR(vp_dev->common_map);
                goto fail;
        }

        /* Not found? */
        if (!vp_dev->common_map)
                return legacy_setup(vp_dev, ...);

        ...

Practice has shown that if we allow something, it'll be done.  We should
break horribly on malformed devices, and really really only fall back
when we have a well-formed, but old, device.

And various other legacy paths can happily use:

#ifdef CONFIG_VIRTIO_PCI_LEGACY
#define is_legacy(vp_dev) ((vp_dev)->legacy_map != NULL)
#else
#define is_legacy(vp_dev) false
#endif

Here's suggested Kconfig entry:

config VIRTIO_PCI_LEGACY
	bool
        default y
	depends on VIRTIO_PCI
	---help---
	  Look out into your driveway.  Do you have a flying car?  If
          so, you can happily disable this option and virtio will not
          break.  Otherwise, leave it set.  Unless you're testing what
          life will be like in The Future.

Cheers,
Rusty.

Re: [PATCHv2 RFC] virtio-pci: flexible configuration layout

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

On Mon, 14 Nov 2011 20:18:55 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Add a flexible mechanism to specify virtio configuration layout, using
> pci vendor-specific capability.  A separate capability is used for each
> of common, device specific and data-path accesses.
> 
> Warning: compiled only.
> This patch also needs to be split up, pci_iomap changes
> also need arch updates for non-x86.
> There might also be more spec changes.
> 
> Posting here for early feedback, and to allow Sasha to
> proceed with his "kvm tool" work.
> 
> Changes from v1:
> Updated to match v3 of the spec, see:
> 	Subject: [PATCHv3 RFC] virtio-spec: flexible configuration layout
> 	Message-ID: <20111110122436.GA13144@redhat.com>
> 	In-Reply-To: <20111109195901.GA28155@redhat.com>

Looks like this conflicts with your other iomap changes... I didn't
check your latest tree; do you just add another patch on top for the
virtio changes now?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv2 RFC] virtio-pci: flexible configuration layout

[Michael S. Tsirkin]

On Mon, Dec 05, 2011 at 11:16:05AM -0800, Jesse Barnes wrote:
> On Mon, 14 Nov 2011 20:18:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Add a flexible mechanism to specify virtio configuration layout, using
> > pci vendor-specific capability.  A separate capability is used for each
> > of common, device specific and data-path accesses.
> > 
> > Warning: compiled only.
> > This patch also needs to be split up, pci_iomap changes
> > also need arch updates for non-x86.
> > There might also be more spec changes.
> > 
> > Posting here for early feedback, and to allow Sasha to
> > proceed with his "kvm tool" work.
> > 
> > Changes from v1:
> > Updated to match v3 of the spec, see:
> > 	Subject: [PATCHv3 RFC] virtio-spec: flexible configuration layout
> > 	Message-ID: <20111110122436.GA13144@redhat.com>
> > 	In-Reply-To: <20111109195901.GA28155@redhat.com>
> 
> Looks like this conflicts with your other iomap changes... I didn't
> check your latest tree; do you just add another patch on top for the
> virtio changes now?
> 
> Thanks,

Yes. Rusty asked for more changes so that isn't yet pushed.

> -- 
> Jesse Barnes, Intel Open Source Technology Center