[PATCH v2 0/7] libqos: add VIRTIO PCI 1.0 support

[PATCH v2 0/7] libqos: add VIRTIO PCI 1.0 support

[Stefan Hajnoczi]

v2:
 * Fix checkpatch.pl issues, except MAINTAINERS file warning.  libqos already
   has maintainers and the new virtio-pci-modern.[ch] files don't need extra
   entries since they are already covered by the existing libqos/ entry.

New VIRTIO devices are Non-Transitional.  This means they only expose the
VIRTIO 1.0 PCI register interface.

The libqos virtio-pci.c code only supports Legacy and Transitional devices (in
Legacy mode).  This patch series add VIRTIO PCI 1.0 support so that tests can
run against Non-Transitional devices too.

Note that this does not actually add VIRTIO 1.0 support to our tests.  That
would require extending feature negotiation (VIRTIO_F_VERSION_1).  I will look
at this as a separate step but the most pressing issue is getting libqos to
work with Non-Transitional virtio-pci devices.

Stefan Hajnoczi (7):
  libqos: extract Legacy virtio-pci.c code
  libqos: add iteration support to qpci_find_capability()
  libqos: pass full QVirtQueue to set_queue_address()
  libqos: add MSI-X callbacks to QVirtioPCIDevice
  libqos: expose common virtqueue setup/cleanup functions
  libqos: make the virtio-pci BAR index configurable
  libqos: add VIRTIO PCI 1.0 support

 tests/Makefile.include           |   1 +
 tests/libqos/pci.h               |   2 +-
 tests/libqos/virtio-pci-modern.h |  17 ++
 tests/libqos/virtio-pci.h        |  34 ++-
 tests/libqos/virtio.h            |   2 +-
 tests/libqos/pci.c               |  18 +-
 tests/libqos/virtio-mmio.c       |   6 +-
 tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
 tests/libqos/virtio-pci.c        |  92 ++++---
 9 files changed, 537 insertions(+), 47 deletions(-)
 create mode 100644 tests/libqos/virtio-pci-modern.h
 create mode 100644 tests/libqos/virtio-pci-modern.c

-- 
2.21.0

[PATCH v2 1/7] libqos: extract Legacy virtio-pci.c code

[Stefan Hajnoczi]

The current libqos virtio-pci.c code implements the VIRTIO Legacy
interface.  Extract existing code in preparation for VIRTIO 1.0 support.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio-pci.h |  2 --
 tests/libqos/virtio-pci.c | 25 ++++++++++---------------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index 728b4715f1..0d105d67b3 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -31,8 +31,6 @@ typedef struct QVirtQueuePCI {
     uint32_t msix_data;
 } QVirtQueuePCI;
 
-extern const QVirtioBus qvirtio_pci;
-
 void virtio_pci_init(QVirtioPCIDevice *dev, QPCIBus *bus, QPCIAddress * addr);
 QVirtioPCIDevice *virtio_pci_new(QPCIBus *bus, QPCIAddress * addr);
 
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 50499e75ef..c8d736f4d1 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -35,14 +35,6 @@
  * original qvirtio_pci_destructor and qvirtio_pci_start_hw.
  */
 
-static inline bool qvirtio_pci_is_big_endian(QVirtioPCIDevice *dev)
-{
-    QPCIBus *bus = dev->pdev->bus;
-
-    /* FIXME: virtio 1.0 is always little-endian */
-    return qtest_big_endian(bus->qts);
-}
-
 #define CONFIG_BASE(dev) (VIRTIO_PCI_CONFIG_OFF((dev)->pdev->msix_enabled))
 
 static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t off)
@@ -55,8 +47,7 @@ static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t off)
  * but virtio ( < 1.0) is in guest order
  * so with a big-endian guest the order has been reversed,
  * reverse it again
- * virtio-1.0 is always little-endian, like PCI, but this
- * case will be managed inside qvirtio_pci_is_big_endian()
+ * virtio-1.0 is always little-endian, like PCI
  */
 
 static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t off)
@@ -258,7 +249,7 @@ static void qvirtio_pci_virtqueue_kick(QVirtioDevice *d, QVirtQueue *vq)
     qpci_io_writew(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_NOTIFY, vq->index);
 }
 
-const QVirtioBus qvirtio_pci = {
+static const QVirtioBus qvirtio_pci_legacy = {
     .config_readb = qvirtio_pci_config_readb,
     .config_readw = qvirtio_pci_config_readw,
     .config_readl = qvirtio_pci_config_readl,
@@ -374,15 +365,19 @@ void qvirtio_pci_start_hw(QOSGraphObject *obj)
     qvirtio_start_device(&dev->vdev);
 }
 
+static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
+{
+    dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
+    dev->vdev.bus = &qvirtio_pci_legacy;
+    dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
+}
+
 static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_dev)
 {
     dev->pdev = pci_dev;
-    dev->vdev.device_type = qpci_config_readw(pci_dev, PCI_SUBSYSTEM_ID);
-
     dev->config_msix_entry = -1;
 
-    dev->vdev.bus = &qvirtio_pci;
-    dev->vdev.big_endian = qvirtio_pci_is_big_endian(dev);
+    qvirtio_pci_init_legacy(dev);
 
     /* each virtio-xxx-pci device should override at least this function */
     dev->obj.get_driver = NULL;
-- 
2.21.0

[PATCH v2 2/7] libqos: add iteration support to qpci_find_capability()

[Stefan Hajnoczi]

VIRTIO 1.0 PCI devices have multiple PCI_CAP_ID_VNDR capabilities so we
need a way to iterate over them.  Extend qpci_find_capability() to take
the last address.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/pci.h |  2 +-
 tests/libqos/pci.c | 18 ++++++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index a5389a5845..590c175190 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -86,7 +86,7 @@ bool qpci_has_buggy_msi(QPCIDevice *dev);
 bool qpci_check_buggy_msi(QPCIDevice *dev);
 
 void qpci_device_enable(QPCIDevice *dev);
-uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id);
+uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
 void qpci_msix_enable(QPCIDevice *dev);
 void qpci_msix_disable(QPCIDevice *dev);
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 662ee7a517..b8679dff1d 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -115,10 +115,16 @@ void qpci_device_enable(QPCIDevice *dev)
     g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
 }
 
-uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id)
+uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr)
 {
     uint8_t cap;
-    uint8_t addr = qpci_config_readb(dev, PCI_CAPABILITY_LIST);
+    uint8_t addr;
+
+    if (start_addr) {
+        addr = qpci_config_readb(dev, start_addr + PCI_CAP_LIST_NEXT);
+    } else {
+        addr = qpci_config_readb(dev, PCI_CAPABILITY_LIST);
+    }
 
     do {
         cap = qpci_config_readb(dev, addr);
@@ -138,7 +144,7 @@ void qpci_msix_enable(QPCIDevice *dev)
     uint8_t bir_table;
     uint8_t bir_pba;
 
-    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
+    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
     g_assert_cmphex(addr, !=, 0);
 
     val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
@@ -167,7 +173,7 @@ void qpci_msix_disable(QPCIDevice *dev)
     uint16_t val;
 
     g_assert(dev->msix_enabled);
-    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
+    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
     g_assert_cmphex(addr, !=, 0);
     val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
     qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
@@ -203,7 +209,7 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
     uint64_t vector_off = dev->msix_table_off + entry * PCI_MSIX_ENTRY_SIZE;
 
     g_assert(dev->msix_enabled);
-    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
+    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
     g_assert_cmphex(addr, !=, 0);
     val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
 
@@ -221,7 +227,7 @@ uint16_t qpci_msix_table_size(QPCIDevice *dev)
     uint8_t addr;
     uint16_t control;
 
-    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
+    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
     g_assert_cmphex(addr, !=, 0);
 
     control = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
-- 
2.21.0

[PATCH v2 3/7] libqos: pass full QVirtQueue to set_queue_address()

[Stefan Hajnoczi]

Instead of just passing the vring page frame number, pass the full
QVirtQueue.  This will allow the VIRTIO 1.0 transport to program the
fine-grained vring address registers in the future.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.h      | 2 +-
 tests/libqos/virtio-mmio.c | 6 ++++--
 tests/libqos/virtio-pci.c  | 6 ++++--
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index 2cb2448f46..37f55b6ade 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -79,7 +79,7 @@ struct QVirtioBus {
     uint16_t (*get_queue_size)(QVirtioDevice *d);
 
     /* Set the address of the selected queue */
-    void (*set_queue_address)(QVirtioDevice *d, uint32_t pfn);
+    void (*set_queue_address)(QVirtioDevice *d, QVirtQueue *vq);
 
     /* Setup the virtqueue specified by index */
     QVirtQueue *(*virtqueue_setup)(QVirtioDevice *d, QGuestAllocator *alloc,
diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c
index d0047876a8..43ca4e49c1 100644
--- a/tests/libqos/virtio-mmio.c
+++ b/tests/libqos/virtio-mmio.c
@@ -127,9 +127,11 @@ static uint16_t qvirtio_mmio_get_queue_size(QVirtioDevice *d)
     return (uint16_t)qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_NUM_MAX);
 }
 
-static void qvirtio_mmio_set_queue_address(QVirtioDevice *d, uint32_t pfn)
+static void qvirtio_mmio_set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
 {
     QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
+    uint64_t pfn = vq->desc / dev->page_size;
+
     qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_PFN, pfn);
 }
 
@@ -162,7 +164,7 @@ static QVirtQueue *qvirtio_mmio_virtqueue_setup(QVirtioDevice *d,
 
     addr = guest_alloc(alloc, qvring_size(vq->size, dev->page_size));
     qvring_init(dev->qts, alloc, vq, addr);
-    qvirtio_mmio_set_queue_address(d, vq->desc / dev->page_size);
+    qvirtio_mmio_set_queue_address(d, vq);
 
     return vq;
 }
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index c8d736f4d1..4772239b61 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -190,9 +190,11 @@ static uint16_t qvirtio_pci_get_queue_size(QVirtioDevice *d)
     return qpci_io_readw(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_NUM);
 }
 
-static void qvirtio_pci_set_queue_address(QVirtioDevice *d, uint32_t pfn)
+static void qvirtio_pci_set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    uint64_t pfn = vq->desc / VIRTIO_PCI_VRING_ALIGN;
+
     qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_PFN, pfn);
 }
 
@@ -229,7 +231,7 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
     addr = guest_alloc(alloc, qvring_size(vqpci->vq.size,
                                           VIRTIO_PCI_VRING_ALIGN));
     qvring_init(qvpcidev->pdev->bus->qts, alloc, &vqpci->vq, addr);
-    qvirtio_pci_set_queue_address(d, vqpci->vq.desc / VIRTIO_PCI_VRING_ALIGN);
+    qvirtio_pci_set_queue_address(d, &vqpci->vq);
 
     return &vqpci->vq;
 }
-- 
2.21.0

[PATCH v2 4/7] libqos: add MSI-X callbacks to QVirtioPCIDevice

[Stefan Hajnoczi]

The MSI-X vectors are programmed differently in the VIRTIO 1.0 and
Legacy interfaces.  Introduce callbacks so different implementations can
be used depending on the interface version.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio-pci.h | 12 ++++++++++++
 tests/libqos/virtio-pci.c | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index 0d105d67b3..443e53affc 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -14,16 +14,28 @@
 #include "libqos/pci.h"
 #include "libqos/qgraph.h"
 
+typedef struct QVirtioPCIMSIXOps QVirtioPCIMSIXOps;
+
 typedef struct QVirtioPCIDevice {
     QOSGraphObject obj;
     QVirtioDevice vdev;
     QPCIDevice *pdev;
     QPCIBar bar;
+    const QVirtioPCIMSIXOps *msix_ops;
     uint16_t config_msix_entry;
     uint64_t config_msix_addr;
     uint32_t config_msix_data;
 } QVirtioPCIDevice;
 
+struct QVirtioPCIMSIXOps {
+    /* Set the Configuration Vector for MSI-X */
+    void (*set_config_vector)(QVirtioPCIDevice *d, uint16_t entry);
+
+    /* Set the Queue Vector for MSI-X */
+    void (*set_queue_vector)(QVirtioPCIDevice *d, uint16_t vq_idx,
+                             uint16_t entry);
+};
+
 typedef struct QVirtQueuePCI {
     QVirtQueue vq;
     uint16_t msix_entry;
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 4772239b61..651f6dbfc6 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -271,6 +271,31 @@ static const QVirtioBus qvirtio_pci_legacy = {
     .virtqueue_kick = qvirtio_pci_virtqueue_kick,
 };
 
+static void qvirtio_pci_set_config_vector(QVirtioPCIDevice *d, uint16_t entry)
+{
+    uint16_t vector;
+
+    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR, entry);
+    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR);
+    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
+}
+
+static void qvirtio_pci_set_queue_vector(QVirtioPCIDevice *d, uint16_t vq_idx,
+                                         uint16_t entry)
+{
+    uint16_t vector;
+
+    qvirtio_pci_queue_select(&d->vdev, vq_idx);
+    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR, entry);
+    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR);
+    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
+}
+
+static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
+    .set_config_vector = qvirtio_pci_set_config_vector,
+    .set_queue_vector = qvirtio_pci_set_queue_vector,
+};
+
 void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
 {
     qpci_device_enable(d->pdev);
@@ -285,7 +310,6 @@ void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
                                         QGuestAllocator *alloc, uint16_t entry)
 {
-    uint16_t vector;
     uint32_t control;
     uint64_t off;
 
@@ -311,16 +335,12 @@ void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
                    off + PCI_MSIX_ENTRY_VECTOR_CTRL,
                    control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
 
-    qvirtio_pci_queue_select(&d->vdev, vqpci->vq.index);
-    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR, entry);
-    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR);
-    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
+    d->msix_ops->set_queue_vector(d, vqpci->vq.index, entry);
 }
 
 void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
                                         QGuestAllocator *alloc, uint16_t entry)
 {
-    uint16_t vector;
     uint32_t control;
     uint64_t off;
 
@@ -348,9 +368,7 @@ void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
                    off + PCI_MSIX_ENTRY_VECTOR_CTRL,
                    control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
 
-    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR, entry);
-    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR);
-    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
+    d->msix_ops->set_config_vector(d, entry);
 }
 
 void qvirtio_pci_destructor(QOSGraphObject *obj)
@@ -371,6 +389,7 @@ static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
 {
     dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
     dev->vdev.bus = &qvirtio_pci_legacy;
+    dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
     dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
 }
 
-- 
2.21.0

[PATCH v2 5/7] libqos: expose common virtqueue setup/cleanup functions

[Stefan Hajnoczi]

The VIRTIO 1.0 code will need to perform additional steps but it will
reuse the common virtqueue setup/cleanup code.  Make these functions
public.

Make sure to invoke callbacks via QVirtioBus instead of directly calling
the virtio-pci Legacy versions of these functions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio-pci.h |  8 ++++++++
 tests/libqos/virtio-pci.c | 19 ++++++++++---------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index 443e53affc..b620c30451 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -63,4 +63,12 @@ void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
                                         QGuestAllocator *alloc, uint16_t entry);
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
                                         QGuestAllocator *alloc, uint16_t entry);
+
+/* Used by Legacy and Modern virtio-pci code */
+QVirtQueue *qvirtio_pci_virtqueue_setup_common(QVirtioDevice *d,
+                                               QGuestAllocator *alloc,
+                                               uint16_t index);
+void qvirtio_pci_virtqueue_cleanup_common(QVirtQueue *vq,
+                                          QGuestAllocator *alloc);
+
 #endif
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 651f6dbfc6..3fb4af4016 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -198,8 +198,9 @@ static void qvirtio_pci_set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
     qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_PFN, pfn);
 }
 
-static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
-                                        QGuestAllocator *alloc, uint16_t index)
+QVirtQueue *qvirtio_pci_virtqueue_setup_common(QVirtioDevice *d,
+                                               QGuestAllocator *alloc,
+                                               uint16_t index)
 {
     uint32_t feat;
     uint64_t addr;
@@ -207,11 +208,11 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
     QVirtioPCIDevice *qvpcidev = container_of(d, QVirtioPCIDevice, vdev);
 
     vqpci = g_malloc0(sizeof(*vqpci));
-    feat = qvirtio_pci_get_guest_features(d);
+    feat = d->bus->get_guest_features(d);
 
-    qvirtio_pci_queue_select(d, index);
+    d->bus->queue_select(d, index);
     vqpci->vq.index = index;
-    vqpci->vq.size = qvirtio_pci_get_queue_size(d);
+    vqpci->vq.size = d->bus->get_queue_size(d);
     vqpci->vq.free_head = 0;
     vqpci->vq.num_free = vqpci->vq.size;
     vqpci->vq.align = VIRTIO_PCI_VRING_ALIGN;
@@ -231,12 +232,12 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
     addr = guest_alloc(alloc, qvring_size(vqpci->vq.size,
                                           VIRTIO_PCI_VRING_ALIGN));
     qvring_init(qvpcidev->pdev->bus->qts, alloc, &vqpci->vq, addr);
-    qvirtio_pci_set_queue_address(d, &vqpci->vq);
+    d->bus->set_queue_address(d, &vqpci->vq);
 
     return &vqpci->vq;
 }
 
-static void qvirtio_pci_virtqueue_cleanup(QVirtQueue *vq,
+void qvirtio_pci_virtqueue_cleanup_common(QVirtQueue *vq,
                                           QGuestAllocator *alloc)
 {
     QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
@@ -266,8 +267,8 @@ static const QVirtioBus qvirtio_pci_legacy = {
     .queue_select = qvirtio_pci_queue_select,
     .get_queue_size = qvirtio_pci_get_queue_size,
     .set_queue_address = qvirtio_pci_set_queue_address,
-    .virtqueue_setup = qvirtio_pci_virtqueue_setup,
-    .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup,
+    .virtqueue_setup = qvirtio_pci_virtqueue_setup_common,
+    .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup_common,
     .virtqueue_kick = qvirtio_pci_virtqueue_kick,
 };
 
-- 
2.21.0

[PATCH v2 6/7] libqos: make the virtio-pci BAR index configurable

[Stefan Hajnoczi]

The Legacy virtio-pci interface always uses BAR 0.  VIRTIO 1.0 may need
to use a different BAR index, so make it configurable.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio-pci.h | 2 ++
 tests/libqos/virtio-pci.c | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index b620c30451..f2d53aa377 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -25,6 +25,8 @@ typedef struct QVirtioPCIDevice {
     uint16_t config_msix_entry;
     uint64_t config_msix_addr;
     uint32_t config_msix_data;
+
+    uint8_t bar_idx;
 } QVirtioPCIDevice;
 
 struct QVirtioPCIMSIXOps {
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 3fb4af4016..efd8caee18 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -300,7 +300,7 @@ static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
 void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
 {
     qpci_device_enable(d->pdev);
-    d->bar = qpci_iomap(d->pdev, 0, NULL);
+    d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
 }
 
 void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
@@ -389,6 +389,7 @@ void qvirtio_pci_start_hw(QOSGraphObject *obj)
 static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
 {
     dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
+    dev->bar_idx = 0;
     dev->vdev.bus = &qvirtio_pci_legacy;
     dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
     dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
-- 
2.21.0

[PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Stefan Hajnoczi]

Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
that the register layout is no longer a fixed layout in BAR 0.  Instead
we have to iterate of PCI Capabilities to find descriptions of where
various registers are located.  The vring registers are also more
fine-grained, allowing for more flexible vring layouts, but we don't
take advantage of that.

Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
therefore not running in VIRTIO 1.0 mode.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/Makefile.include           |   1 +
 tests/libqos/virtio-pci-modern.h |  17 ++
 tests/libqos/virtio-pci.h        |  10 +
 tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
 tests/libqos/virtio-pci.c        |   6 +-
 5 files changed, 445 insertions(+), 1 deletion(-)
 create mode 100644 tests/libqos/virtio-pci-modern.h
 create mode 100644 tests/libqos/virtio-pci-modern.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3543451ed3..3f633c8313 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -715,6 +715,7 @@ qos-test-obj-y += tests/libqos/virtio-blk.o
 qos-test-obj-y += tests/libqos/virtio-mmio.o
 qos-test-obj-y += tests/libqos/virtio-net.o
 qos-test-obj-y += tests/libqos/virtio-pci.o
+qos-test-obj-y += tests/libqos/virtio-pci-modern.o
 qos-test-obj-y += tests/libqos/virtio-rng.o
 qos-test-obj-y += tests/libqos/virtio-scsi.o
 qos-test-obj-y += tests/libqos/virtio-serial.o
diff --git a/tests/libqos/virtio-pci-modern.h b/tests/libqos/virtio-pci-modern.h
new file mode 100644
index 0000000000..6bf2b207c3
--- /dev/null
+++ b/tests/libqos/virtio-pci-modern.h
@@ -0,0 +1,17 @@
+/*
+ * libqos virtio PCI VIRTIO 1.0 definitions
+ *
+ * Copyright (c) 2019 Red Hat, Inc
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef LIBQOS_VIRTIO_PCI_MODERN_H
+#define LIBQOS_VIRTIO_PCI_MODERN_H
+
+#include "virtio-pci.h"
+
+bool qvirtio_pci_init_virtio_1(QVirtioPCIDevice *dev);
+
+#endif /* LIBQOS_VIRTIO_PCI_MODERN_H */
diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index f2d53aa377..29d4e9bf79 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -27,6 +27,13 @@ typedef struct QVirtioPCIDevice {
     uint32_t config_msix_data;
 
     uint8_t bar_idx;
+
+    /* VIRTIO 1.0 */
+    uint32_t common_cfg_offset;
+    uint32_t notify_cfg_offset;
+    uint32_t notify_off_multiplier;
+    uint32_t isr_cfg_offset;
+    uint32_t device_cfg_offset;
 } QVirtioPCIDevice;
 
 struct QVirtioPCIMSIXOps {
@@ -43,6 +50,9 @@ typedef struct QVirtQueuePCI {
     uint16_t msix_entry;
     uint64_t msix_addr;
     uint32_t msix_data;
+
+    /* VIRTIO 1.0 */
+    uint64_t notify_offset;
 } QVirtQueuePCI;
 
 void virtio_pci_init(QVirtioPCIDevice *dev, QPCIBus *bus, QPCIAddress * addr);
diff --git a/tests/libqos/virtio-pci-modern.c b/tests/libqos/virtio-pci-modern.c
new file mode 100644
index 0000000000..f23c876290
--- /dev/null
+++ b/tests/libqos/virtio-pci-modern.c
@@ -0,0 +1,412 @@
+/*
+ * libqos VIRTIO 1.0 PCI driver
+ *
+ * Copyright (c) 2019 Red Hat, Inc
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "standard-headers/linux/pci_regs.h"
+#include "standard-headers/linux/virtio_pci.h"
+#include "virtio-pci-modern.h"
+
+static uint8_t config_readb(QVirtioDevice *d, uint64_t addr)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    return qpci_io_readb(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
+}
+
+static uint16_t config_readw(QVirtioDevice *d, uint64_t addr)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    return qpci_io_readw(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
+}
+
+static uint32_t config_readl(QVirtioDevice *d, uint64_t addr)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    return qpci_io_readl(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
+}
+
+static uint64_t config_readq(QVirtioDevice *d, uint64_t addr)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    return qpci_io_readq(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
+}
+
+static uint32_t get_features(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            device_feature_select),
+                   0);
+    return qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
+                         offsetof(struct virtio_pci_common_cfg,
+                                  device_feature));
+}
+
+static void set_features(QVirtioDevice *d, uint32_t features)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature_select),
+                   0);
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature),
+                   features);
+}
+
+static uint32_t get_guest_features(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature_select),
+                   0);
+    return qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
+                         offsetof(struct virtio_pci_common_cfg,
+                                  guest_feature));
+}
+
+static uint8_t get_status(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    return qpci_io_readb(dev->pdev, dev->bar, dev->common_cfg_offset +
+                         offsetof(struct virtio_pci_common_cfg,
+                                  device_status));
+}
+
+static void set_status(QVirtioDevice *d, uint8_t status)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    return qpci_io_writeb(dev->pdev, dev->bar, dev->common_cfg_offset +
+                          offsetof(struct virtio_pci_common_cfg,
+                                   device_status),
+                          status);
+}
+
+static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
+                            uint32_t msix_addr, uint32_t msix_data)
+{
+    uint32_t data;
+
+    g_assert_cmpint(msix_entry, !=, -1);
+    if (qpci_msix_masked(dev->pdev, msix_entry)) {
+        /* No ISR checking should be done if masked, but read anyway */
+        return qpci_msix_pending(dev->pdev, msix_entry);
+    }
+
+    data = qtest_readl(dev->pdev->bus->qts, msix_addr);
+    if (data == msix_data) {
+        qtest_writel(dev->pdev->bus->qts, msix_addr, 0);
+        return true;
+    } else {
+        return false;
+    }
+}
+
+static bool get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    if (dev->pdev->msix_enabled) {
+        QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
+
+        return get_msix_status(dev, vqpci->msix_entry, vqpci->msix_addr,
+                               vqpci->msix_data);
+    }
+
+    return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 1;
+}
+
+static bool get_config_isr_status(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    if (dev->pdev->msix_enabled) {
+        return get_msix_status(dev, dev->config_msix_entry,
+                               dev->config_msix_addr, dev->config_msix_data);
+    }
+
+    return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 2;
+}
+
+static void wait_config_isr_status(QVirtioDevice *d, gint64 timeout_us)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    gint64 start_time = g_get_monotonic_time();
+
+    do {
+        g_assert(g_get_monotonic_time() - start_time <= timeout_us);
+        qtest_clock_step(dev->pdev->bus->qts, 100);
+    } while (!get_config_isr_status(d));
+}
+
+static void queue_select(QVirtioDevice *d, uint16_t index)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    qpci_io_writew(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_select),
+                   index);
+}
+
+static uint16_t get_queue_size(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    return qpci_io_readw(dev->pdev, dev->bar, dev->common_cfg_offset +
+                         offsetof(struct virtio_pci_common_cfg, queue_size));
+}
+
+static void set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_desc_lo),
+                   vq->desc);
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_desc_hi),
+                   vq->desc >> 32);
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_avail_lo),
+                   vq->avail);
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_avail_hi),
+                   vq->avail >> 32);
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_used_lo),
+                   vq->used);
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_used_hi),
+                   vq->used >> 32);
+}
+
+static QVirtQueue *virtqueue_setup(QVirtioDevice *d, QGuestAllocator *alloc,
+                                   uint16_t index)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    QVirtQueue *vq;
+    QVirtQueuePCI *vqpci;
+    uint16_t notify_off;
+
+    vq = qvirtio_pci_virtqueue_setup_common(d, alloc, index);
+    vqpci = container_of(vq, QVirtQueuePCI, vq);
+
+    notify_off = qpci_io_readw(dev->pdev, dev->bar, dev->common_cfg_offset +
+                               offsetof(struct virtio_pci_common_cfg,
+                                        queue_notify_off));
+
+    vqpci->notify_offset = dev->notify_cfg_offset +
+                           notify_off * dev->notify_off_multiplier;
+
+    qpci_io_writew(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_enable), 1);
+
+    return vq;
+}
+
+static void virtqueue_kick(QVirtioDevice *d, QVirtQueue *vq)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
+
+    qpci_io_writew(dev->pdev, dev->bar, vqpci->notify_offset, vq->index);
+}
+
+static const QVirtioBus qvirtio_pci_virtio_1 = {
+    .config_readb = config_readb,
+    .config_readw = config_readw,
+    .config_readl = config_readl,
+    .config_readq = config_readq,
+    .get_features = get_features,
+    .set_features = set_features,
+    .get_guest_features = get_guest_features,
+    .get_status = get_status,
+    .set_status = set_status,
+    .get_queue_isr_status = get_queue_isr_status,
+    .wait_config_isr_status = wait_config_isr_status,
+    .queue_select = queue_select,
+    .get_queue_size = get_queue_size,
+    .set_queue_address = set_queue_address,
+    .virtqueue_setup = virtqueue_setup,
+    .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup_common,
+    .virtqueue_kick = virtqueue_kick,
+};
+
+static void set_config_vector(QVirtioPCIDevice *d, uint16_t entry)
+{
+    uint16_t vector;
+
+    qpci_io_writew(d->pdev, d->bar, d->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, msix_config), entry);
+    vector = qpci_io_readw(d->pdev, d->bar, d->common_cfg_offset +
+                           offsetof(struct virtio_pci_common_cfg,
+                                    msix_config));
+    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
+}
+
+static void set_queue_vector(QVirtioPCIDevice *d, uint16_t vq_idx,
+                             uint16_t entry)
+{
+    uint16_t vector;
+
+    queue_select(&d->vdev, vq_idx);
+    qpci_io_writew(d->pdev, d->bar, d->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg, queue_msix_vector),
+                   entry);
+    vector = qpci_io_readw(d->pdev, d->bar, d->common_cfg_offset +
+                           offsetof(struct virtio_pci_common_cfg,
+                                    queue_msix_vector));
+    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
+}
+
+static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_virtio_1 = {
+    .set_config_vector = set_config_vector,
+    .set_queue_vector = set_queue_vector,
+};
+
+static bool probe_device_type(QVirtioPCIDevice *dev)
+{
+    uint16_t vendor_id;
+    uint16_t device_id;
+
+    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
+    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
+    if (vendor_id != 0x1af4) {
+        return false;
+    }
+
+    /*
+     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
+     * is a virtio device"
+     */
+    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
+    if (device_id < 0x1000 || device_id > 0x107f) {
+        return false;
+    }
+
+    /*
+     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
+     * 0x103F depending on the device type"
+     */
+    if (device_id < 0x1040) {
+        /*
+         * "Transitional devices MUST have the PCI Subsystem Device ID matching
+         * the Virtio Device ID"
+         */
+        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
+    } else {
+        /*
+         * "The PCI Device ID is calculated by adding 0x1040 to the Virtio
+         * Device ID"
+         */
+        dev->vdev.device_type = device_id - 0x1040;
+    }
+
+    return true;
+}
+
+/* Find the first VIRTIO 1.0 PCI structure for a given type */
+static bool find_structure(QVirtioPCIDevice *dev, uint8_t cfg_type,
+                           uint8_t *bar, uint32_t *offset, uint32_t *length,
+                           uint8_t *cfg_addr)
+{
+    uint8_t addr = 0;
+
+    while ((addr = qpci_find_capability(dev->pdev, PCI_CAP_ID_VNDR,
+                                        addr)) != 0) {
+        uint8_t type;
+
+        type = qpci_config_readb(dev->pdev,
+                addr + offsetof(struct virtio_pci_cap, cfg_type));
+        if (type != cfg_type) {
+            continue;
+        }
+
+        *bar = qpci_config_readb(dev->pdev,
+                addr + offsetof(struct virtio_pci_cap, bar));
+        *offset = qpci_config_readl(dev->pdev,
+                addr + offsetof(struct virtio_pci_cap, offset));
+        *length = qpci_config_readl(dev->pdev,
+                addr + offsetof(struct virtio_pci_cap, length));
+        if (cfg_addr) {
+            *cfg_addr = addr;
+        }
+
+        return true;
+    }
+
+    return false;
+}
+
+static bool probe_device_layout(QVirtioPCIDevice *dev)
+{
+    uint8_t bar;
+    uint8_t cfg_addr;
+    uint32_t length;
+
+    /*
+     * Due to the qpci_iomap() API we only support devices that put all
+     * structures in the same PCI BAR.  Luckily this is true with QEMU.
+     */
+
+    if (!find_structure(dev, VIRTIO_PCI_CAP_COMMON_CFG, &dev->bar_idx,
+                        &dev->common_cfg_offset, &length, NULL)) {
+        return false;
+    }
+
+    if (!find_structure(dev, VIRTIO_PCI_CAP_NOTIFY_CFG, &bar,
+                        &dev->notify_cfg_offset, &length, &cfg_addr)) {
+        return false;
+    }
+    g_assert_cmphex(bar, ==, dev->bar_idx);
+
+    dev->notify_off_multiplier = qpci_config_readl(dev->pdev,
+            cfg_addr + offsetof(struct virtio_pci_notify_cap,
+                                notify_off_multiplier));
+
+    if (!find_structure(dev, VIRTIO_PCI_CAP_ISR_CFG, &bar,
+                        &dev->isr_cfg_offset, &length, NULL)) {
+        return false;
+    }
+    g_assert_cmphex(bar, ==, dev->bar_idx);
+
+    if (!find_structure(dev, VIRTIO_PCI_CAP_DEVICE_CFG, &bar,
+                        &dev->device_cfg_offset, &length, NULL)) {
+        return false;
+    }
+    g_assert_cmphex(bar, ==, dev->bar_idx);
+
+    return true;
+}
+
+/* Probe a VIRTIO 1.0 device */
+bool qvirtio_pci_init_virtio_1(QVirtioPCIDevice *dev)
+{
+    if (!probe_device_type(dev)) {
+        return false;
+    }
+
+    if (!probe_device_layout(dev)) {
+        return false;
+    }
+
+    dev->vdev.bus = &qvirtio_pci_virtio_1;
+    dev->msix_ops = &qvirtio_pci_msix_ops_virtio_1;
+    dev->vdev.big_endian = false;
+    return true;
+}
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index efd8caee18..5bdf403351 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -22,6 +22,8 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_regs.h"
 
+#include "virtio-pci-modern.h"
+
 /* virtio-pci is a superclass of all virtio-xxx-pci devices;
  * the relation between virtio-pci and virtio-xxx-pci is implicit,
  * and therefore virtio-pci does not produce virtio and is not
@@ -400,7 +402,9 @@ static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_
     dev->pdev = pci_dev;
     dev->config_msix_entry = -1;
 
-    qvirtio_pci_init_legacy(dev);
+    if (!qvirtio_pci_init_virtio_1(dev)) {
+        qvirtio_pci_init_legacy(dev);
+    }
 
     /* each virtio-xxx-pci device should override at least this function */
     dev->obj.get_driver = NULL;
-- 
2.21.0

Re: [PATCH v2 6/7] libqos: make the virtio-pci BAR index configurable

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> The Legacy virtio-pci interface always uses BAR 0.  VIRTIO 1.0 may need
> to use a different BAR index, so make it configurable.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h | 2 ++
>  tests/libqos/virtio-pci.c | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index b620c30451..f2d53aa377 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -25,6 +25,8 @@ typedef struct QVirtioPCIDevice {
>      uint16_t config_msix_entry;
>      uint64_t config_msix_addr;
>      uint32_t config_msix_data;
> +
> +    uint8_t bar_idx;
>  } QVirtioPCIDevice;
>  
>  struct QVirtioPCIMSIXOps {
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 3fb4af4016..efd8caee18 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -300,7 +300,7 @@ static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
>  void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
>  {
>      qpci_device_enable(d->pdev);
> -    d->bar = qpci_iomap(d->pdev, 0, NULL);
> +    d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
>  }
>  
>  void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
> @@ -389,6 +389,7 @@ void qvirtio_pci_start_hw(QOSGraphObject *obj)
>  static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
>  {
>      dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
> +    dev->bar_idx = 0;
>      dev->vdev.bus = &qvirtio_pci_legacy;
>      dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
>      dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);

qpci_iomap() is also called with a hardcoded bar at
virtio-blk-test.c:test_nonexistent_virtqueue(). Should it be fixed here
or in a future patch?

Sergio.

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

Re: [PATCH v2 1/7] libqos: extract Legacy virtio-pci.c code

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> The current libqos virtio-pci.c code implements the VIRTIO Legacy
> interface.  Extract existing code in preparation for VIRTIO 1.0 support.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  2 --
>  tests/libqos/virtio-pci.c | 25 ++++++++++---------------
>  2 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index 728b4715f1..0d105d67b3 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -31,8 +31,6 @@ typedef struct QVirtQueuePCI {
>      uint32_t msix_data;
>  } QVirtQueuePCI;
>  
> -extern const QVirtioBus qvirtio_pci;
> -
>  void virtio_pci_init(QVirtioPCIDevice *dev, QPCIBus *bus, QPCIAddress * addr);
>  QVirtioPCIDevice *virtio_pci_new(QPCIBus *bus, QPCIAddress * addr);
>  
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 50499e75ef..c8d736f4d1 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -35,14 +35,6 @@
>   * original qvirtio_pci_destructor and qvirtio_pci_start_hw.
>   */
>  
> -static inline bool qvirtio_pci_is_big_endian(QVirtioPCIDevice *dev)
> -{
> -    QPCIBus *bus = dev->pdev->bus;
> -
> -    /* FIXME: virtio 1.0 is always little-endian */
> -    return qtest_big_endian(bus->qts);
> -}
> -
>  #define CONFIG_BASE(dev) (VIRTIO_PCI_CONFIG_OFF((dev)->pdev->msix_enabled))
>  
>  static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t off)
> @@ -55,8 +47,7 @@ static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t off)
>   * but virtio ( < 1.0) is in guest order
>   * so with a big-endian guest the order has been reversed,
>   * reverse it again
> - * virtio-1.0 is always little-endian, like PCI, but this
> - * case will be managed inside qvirtio_pci_is_big_endian()
> + * virtio-1.0 is always little-endian, like PCI
>   */
>  
>  static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t off)
> @@ -258,7 +249,7 @@ static void qvirtio_pci_virtqueue_kick(QVirtioDevice *d, QVirtQueue *vq)
>      qpci_io_writew(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_NOTIFY, vq->index);
>  }
>  
> -const QVirtioBus qvirtio_pci = {
> +static const QVirtioBus qvirtio_pci_legacy = {
>      .config_readb = qvirtio_pci_config_readb,
>      .config_readw = qvirtio_pci_config_readw,
>      .config_readl = qvirtio_pci_config_readl,
> @@ -374,15 +365,19 @@ void qvirtio_pci_start_hw(QOSGraphObject *obj)
>      qvirtio_start_device(&dev->vdev);
>  }
>  
> +static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
> +{
> +    dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
> +    dev->vdev.bus = &qvirtio_pci_legacy;
> +    dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
> +}
> +
>  static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_dev)
>  {
>      dev->pdev = pci_dev;
> -    dev->vdev.device_type = qpci_config_readw(pci_dev, PCI_SUBSYSTEM_ID);
> -
>      dev->config_msix_entry = -1;
>  
> -    dev->vdev.bus = &qvirtio_pci;
> -    dev->vdev.big_endian = qvirtio_pci_is_big_endian(dev);
> +    qvirtio_pci_init_legacy(dev);
>  
>      /* each virtio-xxx-pci device should override at least this function */
>      dev->obj.get_driver = NULL;

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

Re: [PATCH v2 2/7] libqos: add iteration support to qpci_find_capability()

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> VIRTIO 1.0 PCI devices have multiple PCI_CAP_ID_VNDR capabilities so we
> need a way to iterate over them.  Extend qpci_find_capability() to take
> the last address.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/pci.h |  2 +-
>  tests/libqos/pci.c | 18 ++++++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index a5389a5845..590c175190 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -86,7 +86,7 @@ bool qpci_has_buggy_msi(QPCIDevice *dev);
>  bool qpci_check_buggy_msi(QPCIDevice *dev);
>  
>  void qpci_device_enable(QPCIDevice *dev);
> -uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id);
> +uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
>  void qpci_msix_enable(QPCIDevice *dev);
>  void qpci_msix_disable(QPCIDevice *dev);
>  bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 662ee7a517..b8679dff1d 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -115,10 +115,16 @@ void qpci_device_enable(QPCIDevice *dev)
>      g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
>  }
>  
> -uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id)
> +uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr)
>  {
>      uint8_t cap;
> -    uint8_t addr = qpci_config_readb(dev, PCI_CAPABILITY_LIST);
> +    uint8_t addr;
> +
> +    if (start_addr) {
> +        addr = qpci_config_readb(dev, start_addr + PCI_CAP_LIST_NEXT);
> +    } else {
> +        addr = qpci_config_readb(dev, PCI_CAPABILITY_LIST);
> +    }
>  
>      do {
>          cap = qpci_config_readb(dev, addr);
> @@ -138,7 +144,7 @@ void qpci_msix_enable(QPCIDevice *dev)
>      uint8_t bir_table;
>      uint8_t bir_pba;
>  
> -    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
> +    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
>      g_assert_cmphex(addr, !=, 0);
>  
>      val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
> @@ -167,7 +173,7 @@ void qpci_msix_disable(QPCIDevice *dev)
>      uint16_t val;
>  
>      g_assert(dev->msix_enabled);
> -    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
> +    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
>      g_assert_cmphex(addr, !=, 0);
>      val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
>      qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
> @@ -203,7 +209,7 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
>      uint64_t vector_off = dev->msix_table_off + entry * PCI_MSIX_ENTRY_SIZE;
>  
>      g_assert(dev->msix_enabled);
> -    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
> +    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
>      g_assert_cmphex(addr, !=, 0);
>      val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
>  
> @@ -221,7 +227,7 @@ uint16_t qpci_msix_table_size(QPCIDevice *dev)
>      uint8_t addr;
>      uint16_t control;
>  
> -    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
> +    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
>      g_assert_cmphex(addr, !=, 0);
>  
>      control = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

Re: [PATCH v2 3/7] libqos: pass full QVirtQueue to set_queue_address()

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> Instead of just passing the vring page frame number, pass the full
> QVirtQueue.  This will allow the VIRTIO 1.0 transport to program the
> fine-grained vring address registers in the future.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.h      | 2 +-
>  tests/libqos/virtio-mmio.c | 6 ++++--
>  tests/libqos/virtio-pci.c  | 6 ++++--
>  3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> index 2cb2448f46..37f55b6ade 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -79,7 +79,7 @@ struct QVirtioBus {
>      uint16_t (*get_queue_size)(QVirtioDevice *d);
>  
>      /* Set the address of the selected queue */
> -    void (*set_queue_address)(QVirtioDevice *d, uint32_t pfn);
> +    void (*set_queue_address)(QVirtioDevice *d, QVirtQueue *vq);
>  
>      /* Setup the virtqueue specified by index */
>      QVirtQueue *(*virtqueue_setup)(QVirtioDevice *d, QGuestAllocator *alloc,
> diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c
> index d0047876a8..43ca4e49c1 100644
> --- a/tests/libqos/virtio-mmio.c
> +++ b/tests/libqos/virtio-mmio.c
> @@ -127,9 +127,11 @@ static uint16_t qvirtio_mmio_get_queue_size(QVirtioDevice *d)
>      return (uint16_t)qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_NUM_MAX);
>  }
>  
> -static void qvirtio_mmio_set_queue_address(QVirtioDevice *d, uint32_t pfn)
> +static void qvirtio_mmio_set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
>  {
>      QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
> +    uint64_t pfn = vq->desc / dev->page_size;
> +
>      qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_PFN, pfn);
>  }
>  
> @@ -162,7 +164,7 @@ static QVirtQueue *qvirtio_mmio_virtqueue_setup(QVirtioDevice *d,
>  
>      addr = guest_alloc(alloc, qvring_size(vq->size, dev->page_size));
>      qvring_init(dev->qts, alloc, vq, addr);
> -    qvirtio_mmio_set_queue_address(d, vq->desc / dev->page_size);
> +    qvirtio_mmio_set_queue_address(d, vq);
>  
>      return vq;
>  }
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index c8d736f4d1..4772239b61 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -190,9 +190,11 @@ static uint16_t qvirtio_pci_get_queue_size(QVirtioDevice *d)
>      return qpci_io_readw(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_NUM);
>  }
>  
> -static void qvirtio_pci_set_queue_address(QVirtioDevice *d, uint32_t pfn)
> +static void qvirtio_pci_set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
>  {
>      QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    uint64_t pfn = vq->desc / VIRTIO_PCI_VRING_ALIGN;
> +
>      qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_PFN, pfn);
>  }
>  
> @@ -229,7 +231,7 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
>      addr = guest_alloc(alloc, qvring_size(vqpci->vq.size,
>                                            VIRTIO_PCI_VRING_ALIGN));
>      qvring_init(qvpcidev->pdev->bus->qts, alloc, &vqpci->vq, addr);
> -    qvirtio_pci_set_queue_address(d, vqpci->vq.desc / VIRTIO_PCI_VRING_ALIGN);
> +    qvirtio_pci_set_queue_address(d, &vqpci->vq);
>  
>      return &vqpci->vq;
>  }

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

Re: [PATCH v2 4/7] libqos: add MSI-X callbacks to QVirtioPCIDevice

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> The MSI-X vectors are programmed differently in the VIRTIO 1.0 and
> Legacy interfaces.  Introduce callbacks so different implementations can
> be used depending on the interface version.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h | 12 ++++++++++++
>  tests/libqos/virtio-pci.c | 37 ++++++++++++++++++++++++++++---------
>  2 files changed, 40 insertions(+), 9 deletions(-)
>
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index 0d105d67b3..443e53affc 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -14,16 +14,28 @@
>  #include "libqos/pci.h"
>  #include "libqos/qgraph.h"
>  
> +typedef struct QVirtioPCIMSIXOps QVirtioPCIMSIXOps;
> +
>  typedef struct QVirtioPCIDevice {
>      QOSGraphObject obj;
>      QVirtioDevice vdev;
>      QPCIDevice *pdev;
>      QPCIBar bar;
> +    const QVirtioPCIMSIXOps *msix_ops;
>      uint16_t config_msix_entry;
>      uint64_t config_msix_addr;
>      uint32_t config_msix_data;
>  } QVirtioPCIDevice;
>  
> +struct QVirtioPCIMSIXOps {
> +    /* Set the Configuration Vector for MSI-X */
> +    void (*set_config_vector)(QVirtioPCIDevice *d, uint16_t entry);
> +
> +    /* Set the Queue Vector for MSI-X */
> +    void (*set_queue_vector)(QVirtioPCIDevice *d, uint16_t vq_idx,
> +                             uint16_t entry);
> +};
> +
>  typedef struct QVirtQueuePCI {
>      QVirtQueue vq;
>      uint16_t msix_entry;
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 4772239b61..651f6dbfc6 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -271,6 +271,31 @@ static const QVirtioBus qvirtio_pci_legacy = {
>      .virtqueue_kick = qvirtio_pci_virtqueue_kick,
>  };
>  
> +static void qvirtio_pci_set_config_vector(QVirtioPCIDevice *d, uint16_t entry)
> +{
> +    uint16_t vector;
> +
> +    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR, entry);
> +    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR);
> +    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
> +}
> +
> +static void qvirtio_pci_set_queue_vector(QVirtioPCIDevice *d, uint16_t vq_idx,
> +                                         uint16_t entry)
> +{
> +    uint16_t vector;
> +
> +    qvirtio_pci_queue_select(&d->vdev, vq_idx);
> +    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR, entry);
> +    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR);
> +    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
> +}
> +
> +static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
> +    .set_config_vector = qvirtio_pci_set_config_vector,
> +    .set_queue_vector = qvirtio_pci_set_queue_vector,
> +};
> +
>  void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
>  {
>      qpci_device_enable(d->pdev);
> @@ -285,7 +310,6 @@ void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
>  void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
>                                          QGuestAllocator *alloc, uint16_t entry)
>  {
> -    uint16_t vector;
>      uint32_t control;
>      uint64_t off;
>  
> @@ -311,16 +335,12 @@ void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
>                     off + PCI_MSIX_ENTRY_VECTOR_CTRL,
>                     control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
>  
> -    qvirtio_pci_queue_select(&d->vdev, vqpci->vq.index);
> -    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR, entry);
> -    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_QUEUE_VECTOR);
> -    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
> +    d->msix_ops->set_queue_vector(d, vqpci->vq.index, entry);
>  }
>  
>  void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
>                                          QGuestAllocator *alloc, uint16_t entry)
>  {
> -    uint16_t vector;
>      uint32_t control;
>      uint64_t off;
>  
> @@ -348,9 +368,7 @@ void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
>                     off + PCI_MSIX_ENTRY_VECTOR_CTRL,
>                     control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
>  
> -    qpci_io_writew(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR, entry);
> -    vector = qpci_io_readw(d->pdev, d->bar, VIRTIO_MSI_CONFIG_VECTOR);
> -    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
> +    d->msix_ops->set_config_vector(d, entry);
>  }
>  
>  void qvirtio_pci_destructor(QOSGraphObject *obj)
> @@ -371,6 +389,7 @@ static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
>  {
>      dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
>      dev->vdev.bus = &qvirtio_pci_legacy;
> +    dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
>      dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
>  }

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

Re: [PATCH v2 5/7] libqos: expose common virtqueue setup/cleanup functions

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> The VIRTIO 1.0 code will need to perform additional steps but it will
> reuse the common virtqueue setup/cleanup code.  Make these functions
> public.
>
> Make sure to invoke callbacks via QVirtioBus instead of directly calling
> the virtio-pci Legacy versions of these functions.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  8 ++++++++
>  tests/libqos/virtio-pci.c | 19 ++++++++++---------
>  2 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index 443e53affc..b620c30451 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -63,4 +63,12 @@ void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
>                                          QGuestAllocator *alloc, uint16_t entry);
>  void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
>                                          QGuestAllocator *alloc, uint16_t entry);
> +
> +/* Used by Legacy and Modern virtio-pci code */
> +QVirtQueue *qvirtio_pci_virtqueue_setup_common(QVirtioDevice *d,
> +                                               QGuestAllocator *alloc,
> +                                               uint16_t index);
> +void qvirtio_pci_virtqueue_cleanup_common(QVirtQueue *vq,
> +                                          QGuestAllocator *alloc);
> +
>  #endif
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 651f6dbfc6..3fb4af4016 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -198,8 +198,9 @@ static void qvirtio_pci_set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
>      qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_PFN, pfn);
>  }
>  
> -static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
> -                                        QGuestAllocator *alloc, uint16_t index)
> +QVirtQueue *qvirtio_pci_virtqueue_setup_common(QVirtioDevice *d,
> +                                               QGuestAllocator *alloc,
> +                                               uint16_t index)
>  {
>      uint32_t feat;
>      uint64_t addr;
> @@ -207,11 +208,11 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
>      QVirtioPCIDevice *qvpcidev = container_of(d, QVirtioPCIDevice, vdev);
>  
>      vqpci = g_malloc0(sizeof(*vqpci));
> -    feat = qvirtio_pci_get_guest_features(d);
> +    feat = d->bus->get_guest_features(d);
>  
> -    qvirtio_pci_queue_select(d, index);
> +    d->bus->queue_select(d, index);
>      vqpci->vq.index = index;
> -    vqpci->vq.size = qvirtio_pci_get_queue_size(d);
> +    vqpci->vq.size = d->bus->get_queue_size(d);
>      vqpci->vq.free_head = 0;
>      vqpci->vq.num_free = vqpci->vq.size;
>      vqpci->vq.align = VIRTIO_PCI_VRING_ALIGN;
> @@ -231,12 +232,12 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
>      addr = guest_alloc(alloc, qvring_size(vqpci->vq.size,
>                                            VIRTIO_PCI_VRING_ALIGN));
>      qvring_init(qvpcidev->pdev->bus->qts, alloc, &vqpci->vq, addr);
> -    qvirtio_pci_set_queue_address(d, &vqpci->vq);
> +    d->bus->set_queue_address(d, &vqpci->vq);
>  
>      return &vqpci->vq;
>  }
>  
> -static void qvirtio_pci_virtqueue_cleanup(QVirtQueue *vq,
> +void qvirtio_pci_virtqueue_cleanup_common(QVirtQueue *vq,
>                                            QGuestAllocator *alloc)
>  {
>      QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
> @@ -266,8 +267,8 @@ static const QVirtioBus qvirtio_pci_legacy = {
>      .queue_select = qvirtio_pci_queue_select,
>      .get_queue_size = qvirtio_pci_get_queue_size,
>      .set_queue_address = qvirtio_pci_set_queue_address,
> -    .virtqueue_setup = qvirtio_pci_virtqueue_setup,
> -    .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup,
> +    .virtqueue_setup = qvirtio_pci_virtqueue_setup_common,
> +    .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup_common,
>      .virtqueue_kick = qvirtio_pci_virtqueue_kick,
>  };

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
> that the register layout is no longer a fixed layout in BAR 0.  Instead
> we have to iterate of PCI Capabilities to find descriptions of where
> various registers are located.  The vring registers are also more
> fine-grained, allowing for more flexible vring layouts, but we don't
> take advantage of that.
>
> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
> therefore not running in VIRTIO 1.0 mode.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/Makefile.include           |   1 +
>  tests/libqos/virtio-pci-modern.h |  17 ++
>  tests/libqos/virtio-pci.h        |  10 +
>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
>  tests/libqos/virtio-pci.c        |   6 +-
>  5 files changed, 445 insertions(+), 1 deletion(-)
>  create mode 100644 tests/libqos/virtio-pci-modern.h
>  create mode 100644 tests/libqos/virtio-pci-modern.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 3543451ed3..3f633c8313 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -715,6 +715,7 @@ qos-test-obj-y += tests/libqos/virtio-blk.o
>  qos-test-obj-y += tests/libqos/virtio-mmio.o
>  qos-test-obj-y += tests/libqos/virtio-net.o
>  qos-test-obj-y += tests/libqos/virtio-pci.o
> +qos-test-obj-y += tests/libqos/virtio-pci-modern.o
>  qos-test-obj-y += tests/libqos/virtio-rng.o
>  qos-test-obj-y += tests/libqos/virtio-scsi.o
>  qos-test-obj-y += tests/libqos/virtio-serial.o
> diff --git a/tests/libqos/virtio-pci-modern.h b/tests/libqos/virtio-pci-modern.h
> new file mode 100644
> index 0000000000..6bf2b207c3
> --- /dev/null
> +++ b/tests/libqos/virtio-pci-modern.h
> @@ -0,0 +1,17 @@
> +/*
> + * libqos virtio PCI VIRTIO 1.0 definitions
> + *
> + * Copyright (c) 2019 Red Hat, Inc
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_VIRTIO_PCI_MODERN_H
> +#define LIBQOS_VIRTIO_PCI_MODERN_H
> +
> +#include "virtio-pci.h"
> +
> +bool qvirtio_pci_init_virtio_1(QVirtioPCIDevice *dev);
> +
> +#endif /* LIBQOS_VIRTIO_PCI_MODERN_H */
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index f2d53aa377..29d4e9bf79 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -27,6 +27,13 @@ typedef struct QVirtioPCIDevice {
>      uint32_t config_msix_data;
>  
>      uint8_t bar_idx;
> +
> +    /* VIRTIO 1.0 */
> +    uint32_t common_cfg_offset;
> +    uint32_t notify_cfg_offset;
> +    uint32_t notify_off_multiplier;
> +    uint32_t isr_cfg_offset;
> +    uint32_t device_cfg_offset;
>  } QVirtioPCIDevice;
>  
>  struct QVirtioPCIMSIXOps {
> @@ -43,6 +50,9 @@ typedef struct QVirtQueuePCI {
>      uint16_t msix_entry;
>      uint64_t msix_addr;
>      uint32_t msix_data;
> +
> +    /* VIRTIO 1.0 */
> +    uint64_t notify_offset;
>  } QVirtQueuePCI;
>  
>  void virtio_pci_init(QVirtioPCIDevice *dev, QPCIBus *bus, QPCIAddress * addr);
> diff --git a/tests/libqos/virtio-pci-modern.c b/tests/libqos/virtio-pci-modern.c
> new file mode 100644
> index 0000000000..f23c876290
> --- /dev/null
> +++ b/tests/libqos/virtio-pci-modern.c
> @@ -0,0 +1,412 @@
> +/*
> + * libqos VIRTIO 1.0 PCI driver
> + *
> + * Copyright (c) 2019 Red Hat, Inc
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "standard-headers/linux/pci_regs.h"
> +#include "standard-headers/linux/virtio_pci.h"
> +#include "virtio-pci-modern.h"
> +
> +static uint8_t config_readb(QVirtioDevice *d, uint64_t addr)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    return qpci_io_readb(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
> +}
> +
> +static uint16_t config_readw(QVirtioDevice *d, uint64_t addr)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    return qpci_io_readw(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
> +}
> +
> +static uint32_t config_readl(QVirtioDevice *d, uint64_t addr)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    return qpci_io_readl(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
> +}
> +
> +static uint64_t config_readq(QVirtioDevice *d, uint64_t addr)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    return qpci_io_readq(dev->pdev, dev->bar, dev->device_cfg_offset + addr);
> +}
> +
> +static uint32_t get_features(QVirtioDevice *d)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg,
> +                            device_feature_select),
> +                   0);
> +    return qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                         offsetof(struct virtio_pci_common_cfg,
> +                                  device_feature));
> +}
> +
> +static void set_features(QVirtioDevice *d, uint32_t features)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg,
> +                            guest_feature_select),
> +                   0);
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg,
> +                            guest_feature),
> +                   features);
> +}
> +
> +static uint32_t get_guest_features(QVirtioDevice *d)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg,
> +                            guest_feature_select),
> +                   0);
> +    return qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                         offsetof(struct virtio_pci_common_cfg,
> +                                  guest_feature));
> +}
> +
> +static uint8_t get_status(QVirtioDevice *d)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    return qpci_io_readb(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                         offsetof(struct virtio_pci_common_cfg,
> +                                  device_status));
> +}
> +
> +static void set_status(QVirtioDevice *d, uint8_t status)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    return qpci_io_writeb(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                          offsetof(struct virtio_pci_common_cfg,
> +                                   device_status),
> +                          status);
> +}
> +
> +static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
> +                            uint32_t msix_addr, uint32_t msix_data)
> +{
> +    uint32_t data;
> +
> +    g_assert_cmpint(msix_entry, !=, -1);
> +    if (qpci_msix_masked(dev->pdev, msix_entry)) {
> +        /* No ISR checking should be done if masked, but read anyway */
> +        return qpci_msix_pending(dev->pdev, msix_entry);
> +    }
> +
> +    data = qtest_readl(dev->pdev->bus->qts, msix_addr);
> +    if (data == msix_data) {
> +        qtest_writel(dev->pdev->bus->qts, msix_addr, 0);
> +        return true;
> +    } else {
> +        return false;
> +    }
> +}
> +
> +static bool get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    if (dev->pdev->msix_enabled) {
> +        QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
> +
> +        return get_msix_status(dev, vqpci->msix_entry, vqpci->msix_addr,
> +                               vqpci->msix_data);
> +    }
> +
> +    return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 1;
> +}
> +
> +static bool get_config_isr_status(QVirtioDevice *d)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    if (dev->pdev->msix_enabled) {
> +        return get_msix_status(dev, dev->config_msix_entry,
> +                               dev->config_msix_addr, dev->config_msix_data);
> +    }
> +
> +    return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 2;
> +}
> +
> +static void wait_config_isr_status(QVirtioDevice *d, gint64 timeout_us)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    gint64 start_time = g_get_monotonic_time();
> +
> +    do {
> +        g_assert(g_get_monotonic_time() - start_time <= timeout_us);
> +        qtest_clock_step(dev->pdev->bus->qts, 100);
> +    } while (!get_config_isr_status(d));
> +}
> +
> +static void queue_select(QVirtioDevice *d, uint16_t index)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    qpci_io_writew(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_select),
> +                   index);
> +}
> +
> +static uint16_t get_queue_size(QVirtioDevice *d)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    return qpci_io_readw(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                         offsetof(struct virtio_pci_common_cfg, queue_size));
> +}
> +
> +static void set_queue_address(QVirtioDevice *d, QVirtQueue *vq)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_desc_lo),
> +                   vq->desc);
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_desc_hi),
> +                   vq->desc >> 32);
> +
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_avail_lo),
> +                   vq->avail);
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_avail_hi),
> +                   vq->avail >> 32);
> +
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_used_lo),
> +                   vq->used);
> +    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_used_hi),
> +                   vq->used >> 32);
> +}
> +
> +static QVirtQueue *virtqueue_setup(QVirtioDevice *d, QGuestAllocator *alloc,
> +                                   uint16_t index)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    QVirtQueue *vq;
> +    QVirtQueuePCI *vqpci;
> +    uint16_t notify_off;
> +
> +    vq = qvirtio_pci_virtqueue_setup_common(d, alloc, index);
> +    vqpci = container_of(vq, QVirtQueuePCI, vq);
> +
> +    notify_off = qpci_io_readw(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                               offsetof(struct virtio_pci_common_cfg,
> +                                        queue_notify_off));
> +
> +    vqpci->notify_offset = dev->notify_cfg_offset +
> +                           notify_off * dev->notify_off_multiplier;
> +
> +    qpci_io_writew(dev->pdev, dev->bar, dev->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_enable), 1);
> +
> +    return vq;
> +}
> +
> +static void virtqueue_kick(QVirtioDevice *d, QVirtQueue *vq)
> +{
> +    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> +    QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
> +
> +    qpci_io_writew(dev->pdev, dev->bar, vqpci->notify_offset, vq->index);
> +}
> +
> +static const QVirtioBus qvirtio_pci_virtio_1 = {
> +    .config_readb = config_readb,
> +    .config_readw = config_readw,
> +    .config_readl = config_readl,
> +    .config_readq = config_readq,
> +    .get_features = get_features,
> +    .set_features = set_features,
> +    .get_guest_features = get_guest_features,
> +    .get_status = get_status,
> +    .set_status = set_status,
> +    .get_queue_isr_status = get_queue_isr_status,
> +    .wait_config_isr_status = wait_config_isr_status,
> +    .queue_select = queue_select,
> +    .get_queue_size = get_queue_size,
> +    .set_queue_address = set_queue_address,
> +    .virtqueue_setup = virtqueue_setup,
> +    .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup_common,
> +    .virtqueue_kick = virtqueue_kick,
> +};
> +
> +static void set_config_vector(QVirtioPCIDevice *d, uint16_t entry)
> +{
> +    uint16_t vector;
> +
> +    qpci_io_writew(d->pdev, d->bar, d->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, msix_config), entry);
> +    vector = qpci_io_readw(d->pdev, d->bar, d->common_cfg_offset +
> +                           offsetof(struct virtio_pci_common_cfg,
> +                                    msix_config));
> +    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
> +}
> +
> +static void set_queue_vector(QVirtioPCIDevice *d, uint16_t vq_idx,
> +                             uint16_t entry)
> +{
> +    uint16_t vector;
> +
> +    queue_select(&d->vdev, vq_idx);
> +    qpci_io_writew(d->pdev, d->bar, d->common_cfg_offset +
> +                   offsetof(struct virtio_pci_common_cfg, queue_msix_vector),
> +                   entry);
> +    vector = qpci_io_readw(d->pdev, d->bar, d->common_cfg_offset +
> +                           offsetof(struct virtio_pci_common_cfg,
> +                                    queue_msix_vector));
> +    g_assert_cmphex(vector, !=, VIRTIO_MSI_NO_VECTOR);
> +}
> +
> +static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_virtio_1 = {
> +    .set_config_vector = set_config_vector,
> +    .set_queue_vector = set_queue_vector,
> +};
> +
> +static bool probe_device_type(QVirtioPCIDevice *dev)
> +{
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +
> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
> +    if (vendor_id != 0x1af4) {
> +        return false;
> +    }
> +
> +    /*
> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
> +     * is a virtio device"
> +     */
> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
> +    if (device_id < 0x1000 || device_id > 0x107f) {
> +        return false;
> +    }
> +
> +    /*
> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
> +     * 0x103F depending on the device type"
> +     */
> +    if (device_id < 0x1040) {
> +        /*
> +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
> +         * the Virtio Device ID"
> +         */
> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
> +    } else {
> +        /*
> +         * "The PCI Device ID is calculated by adding 0x1040 to the Virtio
> +         * Device ID"
> +         */
> +        dev->vdev.device_type = device_id - 0x1040;
> +    }
> +
> +    return true;
> +}
> +
> +/* Find the first VIRTIO 1.0 PCI structure for a given type */
> +static bool find_structure(QVirtioPCIDevice *dev, uint8_t cfg_type,
> +                           uint8_t *bar, uint32_t *offset, uint32_t *length,
> +                           uint8_t *cfg_addr)
> +{
> +    uint8_t addr = 0;
> +
> +    while ((addr = qpci_find_capability(dev->pdev, PCI_CAP_ID_VNDR,
> +                                        addr)) != 0) {
> +        uint8_t type;
> +
> +        type = qpci_config_readb(dev->pdev,
> +                addr + offsetof(struct virtio_pci_cap, cfg_type));
> +        if (type != cfg_type) {
> +            continue;
> +        }
> +
> +        *bar = qpci_config_readb(dev->pdev,
> +                addr + offsetof(struct virtio_pci_cap, bar));
> +        *offset = qpci_config_readl(dev->pdev,
> +                addr + offsetof(struct virtio_pci_cap, offset));
> +        *length = qpci_config_readl(dev->pdev,
> +                addr + offsetof(struct virtio_pci_cap, length));
> +        if (cfg_addr) {
> +            *cfg_addr = addr;
> +        }
> +
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static bool probe_device_layout(QVirtioPCIDevice *dev)
> +{
> +    uint8_t bar;
> +    uint8_t cfg_addr;
> +    uint32_t length;
> +
> +    /*
> +     * Due to the qpci_iomap() API we only support devices that put all
> +     * structures in the same PCI BAR.  Luckily this is true with QEMU.
> +     */
> +
> +    if (!find_structure(dev, VIRTIO_PCI_CAP_COMMON_CFG, &dev->bar_idx,
> +                        &dev->common_cfg_offset, &length, NULL)) {
> +        return false;
> +    }
> +
> +    if (!find_structure(dev, VIRTIO_PCI_CAP_NOTIFY_CFG, &bar,
> +                        &dev->notify_cfg_offset, &length, &cfg_addr)) {
> +        return false;
> +    }
> +    g_assert_cmphex(bar, ==, dev->bar_idx);
> +
> +    dev->notify_off_multiplier = qpci_config_readl(dev->pdev,
> +            cfg_addr + offsetof(struct virtio_pci_notify_cap,
> +                                notify_off_multiplier));
> +
> +    if (!find_structure(dev, VIRTIO_PCI_CAP_ISR_CFG, &bar,
> +                        &dev->isr_cfg_offset, &length, NULL)) {
> +        return false;
> +    }
> +    g_assert_cmphex(bar, ==, dev->bar_idx);
> +
> +    if (!find_structure(dev, VIRTIO_PCI_CAP_DEVICE_CFG, &bar,
> +                        &dev->device_cfg_offset, &length, NULL)) {
> +        return false;
> +    }
> +    g_assert_cmphex(bar, ==, dev->bar_idx);
> +
> +    return true;
> +}
> +
> +/* Probe a VIRTIO 1.0 device */
> +bool qvirtio_pci_init_virtio_1(QVirtioPCIDevice *dev)
> +{
> +    if (!probe_device_type(dev)) {
> +        return false;
> +    }
> +
> +    if (!probe_device_layout(dev)) {
> +        return false;
> +    }
> +
> +    dev->vdev.bus = &qvirtio_pci_virtio_1;
> +    dev->msix_ops = &qvirtio_pci_msix_ops_virtio_1;
> +    dev->vdev.big_endian = false;
> +    return true;
> +}
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index efd8caee18..5bdf403351 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -22,6 +22,8 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_regs.h"
>  
> +#include "virtio-pci-modern.h"
> +
>  /* virtio-pci is a superclass of all virtio-xxx-pci devices;
>   * the relation between virtio-pci and virtio-xxx-pci is implicit,
>   * and therefore virtio-pci does not produce virtio and is not
> @@ -400,7 +402,9 @@ static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_
>      dev->pdev = pci_dev;
>      dev->config_msix_entry = -1;
>  
> -    qvirtio_pci_init_legacy(dev);
> +    if (!qvirtio_pci_init_virtio_1(dev)) {
> +        qvirtio_pci_init_legacy(dev);
> +    }
>  
>      /* each virtio-xxx-pci device should override at least this function */
>      dev->obj.get_driver = NULL;

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

Re: [PATCH v2 6/7] libqos: make the virtio-pci BAR index configurable

[Stefan Hajnoczi]

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

On Fri, Oct 11, 2019 at 02:06:01PM +0200, Sergio Lopez wrote:
> 
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > The Legacy virtio-pci interface always uses BAR 0.  VIRTIO 1.0 may need
> > to use a different BAR index, so make it configurable.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/libqos/virtio-pci.h | 2 ++
> >  tests/libqos/virtio-pci.c | 3 ++-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> > index b620c30451..f2d53aa377 100644
> > --- a/tests/libqos/virtio-pci.h
> > +++ b/tests/libqos/virtio-pci.h
> > @@ -25,6 +25,8 @@ typedef struct QVirtioPCIDevice {
> >      uint16_t config_msix_entry;
> >      uint64_t config_msix_addr;
> >      uint32_t config_msix_data;
> > +
> > +    uint8_t bar_idx;
> >  } QVirtioPCIDevice;
> >  
> >  struct QVirtioPCIMSIXOps {
> > diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> > index 3fb4af4016..efd8caee18 100644
> > --- a/tests/libqos/virtio-pci.c
> > +++ b/tests/libqos/virtio-pci.c
> > @@ -300,7 +300,7 @@ static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
> >  void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
> >  {
> >      qpci_device_enable(d->pdev);
> > -    d->bar = qpci_iomap(d->pdev, 0, NULL);
> > +    d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
> >  }
> >  
> >  void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
> > @@ -389,6 +389,7 @@ void qvirtio_pci_start_hw(QOSGraphObject *obj)
> >  static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
> >  {
> >      dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
> > +    dev->bar_idx = 0;
> >      dev->vdev.bus = &qvirtio_pci_legacy;
> >      dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
> >      dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
> 
> qpci_iomap() is also called with a hardcoded bar at
> virtio-blk-test.c:test_nonexistent_virtqueue(). Should it be fixed here
> or in a future patch?

That virtio-blk-pci-specific continues to work because the
virtio-blk-pci device is a Transitional device that still supports
Legacy mode in BAR 0.

Also, that test doesn't use the libqos virtio API so there is no
conflict between it and this patch series.

Stefan

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

Re: [PATCH v2 6/7] libqos: make the virtio-pci BAR index configurable

[Sergio Lopez]

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


Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Oct 11, 2019 at 02:06:01PM +0200, Sergio Lopez wrote:
>> 
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > The Legacy virtio-pci interface always uses BAR 0.  VIRTIO 1.0 may need
>> > to use a different BAR index, so make it configurable.
>> >
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  tests/libqos/virtio-pci.h | 2 ++
>> >  tests/libqos/virtio-pci.c | 3 ++-
>> >  2 files changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
>> > index b620c30451..f2d53aa377 100644
>> > --- a/tests/libqos/virtio-pci.h
>> > +++ b/tests/libqos/virtio-pci.h
>> > @@ -25,6 +25,8 @@ typedef struct QVirtioPCIDevice {
>> >      uint16_t config_msix_entry;
>> >      uint64_t config_msix_addr;
>> >      uint32_t config_msix_data;
>> > +
>> > +    uint8_t bar_idx;
>> >  } QVirtioPCIDevice;
>> >  
>> >  struct QVirtioPCIMSIXOps {
>> > diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
>> > index 3fb4af4016..efd8caee18 100644
>> > --- a/tests/libqos/virtio-pci.c
>> > +++ b/tests/libqos/virtio-pci.c
>> > @@ -300,7 +300,7 @@ static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
>> >  void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
>> >  {
>> >      qpci_device_enable(d->pdev);
>> > -    d->bar = qpci_iomap(d->pdev, 0, NULL);
>> > +    d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
>> >  }
>> >  
>> >  void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
>> > @@ -389,6 +389,7 @@ void qvirtio_pci_start_hw(QOSGraphObject *obj)
>> >  static void qvirtio_pci_init_legacy(QVirtioPCIDevice *dev)
>> >  {
>> >      dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
>> > +    dev->bar_idx = 0;
>> >      dev->vdev.bus = &qvirtio_pci_legacy;
>> >      dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
>> >      dev->vdev.big_endian = qtest_big_endian(dev->pdev->bus->qts);
>> 
>> qpci_iomap() is also called with a hardcoded bar at
>> virtio-blk-test.c:test_nonexistent_virtqueue(). Should it be fixed here
>> or in a future patch?
>
> That virtio-blk-pci-specific continues to work because the
> virtio-blk-pci device is a Transitional device that still supports
> Legacy mode in BAR 0.
>
> Also, that test doesn't use the libqos virtio API so there is no
> conflict between it and this patch series.

OK, in that case:

Reviewed-by: Sergio Lopez <slp@redhat.com>

> Stefan


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

Re: [PATCH v2 1/7] libqos: extract Legacy virtio-pci.c code

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> The current libqos virtio-pci.c code implements the VIRTIO Legacy
> interface.  Extract existing code in preparation for VIRTIO 1.0 support.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  2 --
>  tests/libqos/virtio-pci.c | 25 ++++++++++---------------
>  2 files changed, 10 insertions(+), 17 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 2/7] libqos: add iteration support to qpci_find_capability()

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> VIRTIO 1.0 PCI devices have multiple PCI_CAP_ID_VNDR capabilities so we
> need a way to iterate over them.  Extend qpci_find_capability() to take
> the last address.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/pci.h |  2 +-
>  tests/libqos/pci.c | 18 ++++++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index a5389a5845..590c175190 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -86,7 +86,7 @@ bool qpci_has_buggy_msi(QPCIDevice *dev);
>  bool qpci_check_buggy_msi(QPCIDevice *dev);
>  
>  void qpci_device_enable(QPCIDevice *dev);
> -uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id);
> +uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);

In case you respin, I'd like to suggest to add a comment in front of
that prototype, explaining how the start_addr is meant to work (but the
source code is trivial enough, so there's no need to respin just because
of this, IMHO).

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 3/7] libqos: pass full QVirtQueue to set_queue_address()

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> Instead of just passing the vring page frame number, pass the full
> QVirtQueue.  This will allow the VIRTIO 1.0 transport to program the
> fine-grained vring address registers in the future.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.h      | 2 +-
>  tests/libqos/virtio-mmio.c | 6 ++++--
>  tests/libqos/virtio-pci.c  | 6 ++++--
>  3 files changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 4/7] libqos: add MSI-X callbacks to QVirtioPCIDevice

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> The MSI-X vectors are programmed differently in the VIRTIO 1.0 and
> Legacy interfaces.  Introduce callbacks so different implementations can
> be used depending on the interface version.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h | 12 ++++++++++++
>  tests/libqos/virtio-pci.c | 37 ++++++++++++++++++++++++++++---------
>  2 files changed, 40 insertions(+), 9 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 5/7] libqos: expose common virtqueue setup/cleanup functions

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> The VIRTIO 1.0 code will need to perform additional steps but it will
> reuse the common virtqueue setup/cleanup code.  Make these functions
> public.
> 
> Make sure to invoke callbacks via QVirtioBus instead of directly calling
> the virtio-pci Legacy versions of these functions.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  8 ++++++++
>  tests/libqos/virtio-pci.c | 19 ++++++++++---------
>  2 files changed, 18 insertions(+), 9 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 6/7] libqos: make the virtio-pci BAR index configurable

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> The Legacy virtio-pci interface always uses BAR 0.  VIRTIO 1.0 may need
> to use a different BAR index, so make it configurable.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-pci.h | 2 ++
>  tests/libqos/virtio-pci.c | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index b620c30451..f2d53aa377 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -25,6 +25,8 @@ typedef struct QVirtioPCIDevice {
>      uint16_t config_msix_entry;
>      uint64_t config_msix_addr;
>      uint32_t config_msix_data;
> +
> +    uint8_t bar_idx;

I think I'd rather make that an "int" instead of "uint8_t" ... but
that's just my personal taste, so anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Thomas Huth]

On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
> that the register layout is no longer a fixed layout in BAR 0.  Instead
> we have to iterate of PCI Capabilities to find descriptions of where
> various registers are located.  The vring registers are also more
> fine-grained, allowing for more flexible vring layouts, but we don't
> take advantage of that.
> 
> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
> therefore not running in VIRTIO 1.0 mode.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/Makefile.include           |   1 +
>  tests/libqos/virtio-pci-modern.h |  17 ++
>  tests/libqos/virtio-pci.h        |  10 +
>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
>  tests/libqos/virtio-pci.c        |   6 +-
>  5 files changed, 445 insertions(+), 1 deletion(-)
>  create mode 100644 tests/libqos/virtio-pci-modern.h
>  create mode 100644 tests/libqos/virtio-pci-modern.c
[...]
> +static bool probe_device_type(QVirtioPCIDevice *dev)
> +{
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +
> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
> +    if (vendor_id != 0x1af4) {
> +        return false;
> +    }
> +
> +    /*
> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
> +     * is a virtio device"
> +     */
> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
> +    if (device_id < 0x1000 || device_id > 0x107f) {
> +        return false;
> +    }
> +
> +    /*
> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
> +     * 0x103F depending on the device type"
> +     */
> +    if (device_id < 0x1040) {
> +        /*
> +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
> +         * the Virtio Device ID"
> +         */
> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);

Shouldn't you return "false" here in case the device_type is 0 ? Which
likely means that it is a legacy or broken device ...?

> +    } else {
> +        /*
> +         * "The PCI Device ID is calculated by adding 0x1040 to the Virtio
> +         * Device ID"
> +         */
> +        dev->vdev.device_type = device_id - 0x1040;
> +    }
> +
> +    return true;
> +}

 Thomas

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Stefan Hajnoczi]

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

On Thu, Oct 17, 2019 at 04:52:54PM +0200, Thomas Huth wrote:
> On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> > Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
> > that the register layout is no longer a fixed layout in BAR 0.  Instead
> > we have to iterate of PCI Capabilities to find descriptions of where
> > various registers are located.  The vring registers are also more
> > fine-grained, allowing for more flexible vring layouts, but we don't
> > take advantage of that.
> > 
> > Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
> > therefore not running in VIRTIO 1.0 mode.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/Makefile.include           |   1 +
> >  tests/libqos/virtio-pci-modern.h |  17 ++
> >  tests/libqos/virtio-pci.h        |  10 +
> >  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
> >  tests/libqos/virtio-pci.c        |   6 +-
> >  5 files changed, 445 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/libqos/virtio-pci-modern.h
> >  create mode 100644 tests/libqos/virtio-pci-modern.c
> [...]
> > +static bool probe_device_type(QVirtioPCIDevice *dev)
> > +{
> > +    uint16_t vendor_id;
> > +    uint16_t device_id;
> > +
> > +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
> > +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
> > +    if (vendor_id != 0x1af4) {
> > +        return false;
> > +    }
> > +
> > +    /*
> > +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
> > +     * is a virtio device"
> > +     */
> > +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
> > +    if (device_id < 0x1000 || device_id > 0x107f) {
> > +        return false;
> > +    }
> > +
> > +    /*
> > +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
> > +     * 0x103F depending on the device type"
> > +     */
> > +    if (device_id < 0x1040) {
> > +        /*
> > +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
> > +         * the Virtio Device ID"
> > +         */
> > +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
> 
> Shouldn't you return "false" here in case the device_type is 0 ? Which
> likely means that it is a legacy or broken device ...?

The real decision whether to use this PCI device or not happens in
probe_device_layout().  If it's broken or a legacy device then that
function will fail.

Stefan

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

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Thomas Huth]

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

On 17/10/2019 18.07, Stefan Hajnoczi wrote:
> On Thu, Oct 17, 2019 at 04:52:54PM +0200, Thomas Huth wrote:
>> On 11/10/2019 10.56, Stefan Hajnoczi wrote:
>>> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
>>> that the register layout is no longer a fixed layout in BAR 0.  Instead
>>> we have to iterate of PCI Capabilities to find descriptions of where
>>> various registers are located.  The vring registers are also more
>>> fine-grained, allowing for more flexible vring layouts, but we don't
>>> take advantage of that.
>>>
>>> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
>>> therefore not running in VIRTIO 1.0 mode.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  tests/Makefile.include           |   1 +
>>>  tests/libqos/virtio-pci-modern.h |  17 ++
>>>  tests/libqos/virtio-pci.h        |  10 +
>>>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
>>>  tests/libqos/virtio-pci.c        |   6 +-
>>>  5 files changed, 445 insertions(+), 1 deletion(-)
>>>  create mode 100644 tests/libqos/virtio-pci-modern.h
>>>  create mode 100644 tests/libqos/virtio-pci-modern.c
>> [...]
>>> +static bool probe_device_type(QVirtioPCIDevice *dev)
>>> +{
>>> +    uint16_t vendor_id;
>>> +    uint16_t device_id;
>>> +
>>> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
>>> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
>>> +    if (vendor_id != 0x1af4) {
>>> +        return false;
>>> +    }
>>> +
>>> +    /*
>>> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
>>> +     * is a virtio device"
>>> +     */
>>> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
>>> +    if (device_id < 0x1000 || device_id > 0x107f) {
>>> +        return false;
>>> +    }
>>> +
>>> +    /*
>>> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
>>> +     * 0x103F depending on the device type"
>>> +     */
>>> +    if (device_id < 0x1040) {
>>> +        /*
>>> +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
>>> +         * the Virtio Device ID"
>>> +         */
>>> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
>>
>> Shouldn't you return "false" here in case the device_type is 0 ? Which
>> likely means that it is a legacy or broken device ...?
> 
> The real decision whether to use this PCI device or not happens in
> probe_device_layout().  If it's broken or a legacy device then that
> function will fail.

Ok, fair.

I've added the patches to my qtest-next branch:

https://gitlab.com/huth/qemu/tree/qtest-next

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Thomas Huth]

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

On 17/10/2019 18.18, Thomas Huth wrote:
> On 17/10/2019 18.07, Stefan Hajnoczi wrote:
>> On Thu, Oct 17, 2019 at 04:52:54PM +0200, Thomas Huth wrote:
>>> On 11/10/2019 10.56, Stefan Hajnoczi wrote:
>>>> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
>>>> that the register layout is no longer a fixed layout in BAR 0.  Instead
>>>> we have to iterate of PCI Capabilities to find descriptions of where
>>>> various registers are located.  The vring registers are also more
>>>> fine-grained, allowing for more flexible vring layouts, but we don't
>>>> take advantage of that.
>>>>
>>>> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
>>>> therefore not running in VIRTIO 1.0 mode.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>  tests/Makefile.include           |   1 +
>>>>  tests/libqos/virtio-pci-modern.h |  17 ++
>>>>  tests/libqos/virtio-pci.h        |  10 +
>>>>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
>>>>  tests/libqos/virtio-pci.c        |   6 +-
>>>>  5 files changed, 445 insertions(+), 1 deletion(-)
>>>>  create mode 100644 tests/libqos/virtio-pci-modern.h
>>>>  create mode 100644 tests/libqos/virtio-pci-modern.c
>>> [...]
>>>> +static bool probe_device_type(QVirtioPCIDevice *dev)
>>>> +{
>>>> +    uint16_t vendor_id;
>>>> +    uint16_t device_id;
>>>> +
>>>> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
>>>> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
>>>> +    if (vendor_id != 0x1af4) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
>>>> +     * is a virtio device"
>>>> +     */
>>>> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
>>>> +    if (device_id < 0x1000 || device_id > 0x107f) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
>>>> +     * 0x103F depending on the device type"
>>>> +     */
>>>> +    if (device_id < 0x1040) {
>>>> +        /*
>>>> +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
>>>> +         * the Virtio Device ID"
>>>> +         */
>>>> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
>>>
>>> Shouldn't you return "false" here in case the device_type is 0 ? Which
>>> likely means that it is a legacy or broken device ...?
>>
>> The real decision whether to use this PCI device or not happens in
>> probe_device_layout().  If it's broken or a legacy device then that
>> function will fail.
> 
> Ok, fair.
> 
> I've added the patches to my qtest-next branch:
> 
> https://gitlab.com/huth/qemu/tree/qtest-next

 Hi Stephan,

looks like this is breaking the virtio-blk-test in certain configurations:

 https://gitlab.com/huth/qemu/-/jobs/324085741

and:

 https://cirrus-ci.com/task/4511314474434560

Could you please have a look?

 Thanks,
  Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Thomas Huth]

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

On 18/10/2019 08.48, Thomas Huth wrote:
> On 17/10/2019 18.18, Thomas Huth wrote:
>> On 17/10/2019 18.07, Stefan Hajnoczi wrote:
>>> On Thu, Oct 17, 2019 at 04:52:54PM +0200, Thomas Huth wrote:
>>>> On 11/10/2019 10.56, Stefan Hajnoczi wrote:
>>>>> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
>>>>> that the register layout is no longer a fixed layout in BAR 0.  Instead
>>>>> we have to iterate of PCI Capabilities to find descriptions of where
>>>>> various registers are located.  The vring registers are also more
>>>>> fine-grained, allowing for more flexible vring layouts, but we don't
>>>>> take advantage of that.
>>>>>
>>>>> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
>>>>> therefore not running in VIRTIO 1.0 mode.
>>>>>
>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> ---
>>>>>  tests/Makefile.include           |   1 +
>>>>>  tests/libqos/virtio-pci-modern.h |  17 ++
>>>>>  tests/libqos/virtio-pci.h        |  10 +
>>>>>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
>>>>>  tests/libqos/virtio-pci.c        |   6 +-
>>>>>  5 files changed, 445 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 tests/libqos/virtio-pci-modern.h
>>>>>  create mode 100644 tests/libqos/virtio-pci-modern.c
>>>> [...]
>>>>> +static bool probe_device_type(QVirtioPCIDevice *dev)
>>>>> +{
>>>>> +    uint16_t vendor_id;
>>>>> +    uint16_t device_id;
>>>>> +
>>>>> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
>>>>> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
>>>>> +    if (vendor_id != 0x1af4) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
>>>>> +     * is a virtio device"
>>>>> +     */
>>>>> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
>>>>> +    if (device_id < 0x1000 || device_id > 0x107f) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
>>>>> +     * 0x103F depending on the device type"
>>>>> +     */
>>>>> +    if (device_id < 0x1040) {
>>>>> +        /*
>>>>> +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
>>>>> +         * the Virtio Device ID"
>>>>> +         */
>>>>> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
>>>>
>>>> Shouldn't you return "false" here in case the device_type is 0 ? Which
>>>> likely means that it is a legacy or broken device ...?
>>>
>>> The real decision whether to use this PCI device or not happens in
>>> probe_device_layout().  If it's broken or a legacy device then that
>>> function will fail.
>>
>> Ok, fair.
>>
>> I've added the patches to my qtest-next branch:
>>
>> https://gitlab.com/huth/qemu/tree/qtest-next
> 
>  Hi Stephan,
> 
> looks like this is breaking the virtio-blk-test in certain configurations:
> 
>  https://gitlab.com/huth/qemu/-/jobs/324085741
> 
> and:
> 
>  https://cirrus-ci.com/task/4511314474434560
> 
> Could you please have a look?

FWIW, the
 assertion failed (capacity == TEST_IMAGE_SIZE / 512): (2199023255552 ==
131072)
looks like an endianess bug to me. 2199023255552 is 131072 byte-swapped.

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Stefan Hajnoczi]

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

On Fri, Oct 18, 2019 at 08:48:23AM +0200, Thomas Huth wrote:
> On 17/10/2019 18.18, Thomas Huth wrote:
> > On 17/10/2019 18.07, Stefan Hajnoczi wrote:
> >> On Thu, Oct 17, 2019 at 04:52:54PM +0200, Thomas Huth wrote:
> >>> On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> >>>> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
> >>>> that the register layout is no longer a fixed layout in BAR 0.  Instead
> >>>> we have to iterate of PCI Capabilities to find descriptions of where
> >>>> various registers are located.  The vring registers are also more
> >>>> fine-grained, allowing for more flexible vring layouts, but we don't
> >>>> take advantage of that.
> >>>>
> >>>> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
> >>>> therefore not running in VIRTIO 1.0 mode.
> >>>>
> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>>> ---
> >>>>  tests/Makefile.include           |   1 +
> >>>>  tests/libqos/virtio-pci-modern.h |  17 ++
> >>>>  tests/libqos/virtio-pci.h        |  10 +
> >>>>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
> >>>>  tests/libqos/virtio-pci.c        |   6 +-
> >>>>  5 files changed, 445 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 tests/libqos/virtio-pci-modern.h
> >>>>  create mode 100644 tests/libqos/virtio-pci-modern.c
> >>> [...]
> >>>> +static bool probe_device_type(QVirtioPCIDevice *dev)
> >>>> +{
> >>>> +    uint16_t vendor_id;
> >>>> +    uint16_t device_id;
> >>>> +
> >>>> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
> >>>> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
> >>>> +    if (vendor_id != 0x1af4) {
> >>>> +        return false;
> >>>> +    }
> >>>> +
> >>>> +    /*
> >>>> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F inclusive
> >>>> +     * is a virtio device"
> >>>> +     */
> >>>> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
> >>>> +    if (device_id < 0x1000 || device_id > 0x107f) {
> >>>> +        return false;
> >>>> +    }
> >>>> +
> >>>> +    /*
> >>>> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 to
> >>>> +     * 0x103F depending on the device type"
> >>>> +     */
> >>>> +    if (device_id < 0x1040) {
> >>>> +        /*
> >>>> +         * "Transitional devices MUST have the PCI Subsystem Device ID matching
> >>>> +         * the Virtio Device ID"
> >>>> +         */
> >>>> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, PCI_SUBSYSTEM_ID);
> >>>
> >>> Shouldn't you return "false" here in case the device_type is 0 ? Which
> >>> likely means that it is a legacy or broken device ...?
> >>
> >> The real decision whether to use this PCI device or not happens in
> >> probe_device_layout().  If it's broken or a legacy device then that
> >> function will fail.
> > 
> > Ok, fair.
> > 
> > I've added the patches to my qtest-next branch:
> > 
> > https://gitlab.com/huth/qemu/tree/qtest-next
> 
>  Hi Stephan,
> 
> looks like this is breaking the virtio-blk-test in certain configurations:
> 
>  https://gitlab.com/huth/qemu/-/jobs/324085741
> 
> and:
> 
>  https://cirrus-ci.com/task/4511314474434560
> 
> Could you please have a look?

On reading the VIRTIO specification again, I think my idea of supporting
the VIRTIO 1.0 PCI interface without actually negotiating the
VIRTIO_F_VERSION_1 feature bit is non-compliant:

  2.2.3 Legacy Interface: A Note on Feature Bits

  Transitional Drivers MUST detect Legacy Devices by detecting that the feature bit VIRTIO_F_VERSION_1 is not offered. [...]

  In this case device is used through the legacy interface.

Please drop this patch series for now.  Additional patches are required
to implement VIRTIO_F_VERSION_1 and then the endianness issue will go
away.  I will send a v2.

Stefan

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