[PATCH v3 00/16] libqos: add VIRTIO PCI 1.0 support

[PATCH v3 00/16] libqos: add VIRTIO PCI 1.0 support

[Stefan Hajnoczi]

v3:
 * Now implements VIRTIO 1.0 fully (vring, device initialization).
   This required several new patches to address the following issues:
   1. Tests that do not negotiate features (non-compliant!)
   2. Tests that access configuration space before feature negotiation
      (non-compliant!)
   3. Tests that set up virtqueues before feature negotiation (non-compliant!)
   4. vring accesses require byte-swapping when VIRTIO 1.0 is used with a
      big-endian guest because the qtest_readX/writeX() API automatically
      converts to guest-endian
   5. VIRTIO 1.0 has an additional FEATURES_OK step during Device
      Initialization
 * Change uint8_t bar_idx to int [Thomas]
 * Document qpci_find_capability() [Thomas]
 * Every commit tested with arm, ppc64, and x86_64 targets using:
   git rebase -i origin/master -x 'make tests/qos-test &&
   QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 tests/qos-test &&
   QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/qos-test'
   QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/qos-test'
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 interface, not the Legacy interface.

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

The virtio-fs device is Non-Transitional, so this is a prerequisite for
virtio-fs qos tests.

Stefan Hajnoczi (16):
  tests/virtio-blk-test: read config space after feature negotiation
  libqos: read QVIRTIO_MMIO_VERSION register
  libqos: extend feature bits to 64-bit
  virtio-scsi-test: add missing feature negotiation
  tests/virtio-blk-test: set up virtqueue after feature negotiation
  libqos: add missing virtio-9p feature negotiation
  libqos: enforce Device Initialization order
  libqos: implement VIRTIO 1.0 FEATURES_OK step
  libqos: access VIRTIO 1.0 vring in little-endian
  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: extract Legacy virtio-pci.c code
  libqos: add VIRTIO PCI 1.0 support

 tests/Makefile.include           |   1 +
 tests/libqos/pci.h               |   2 +-
 tests/libqos/virtio-mmio.h       |   1 +
 tests/libqos/virtio-pci-modern.h |  17 ++
 tests/libqos/virtio-pci.h        |  34 ++-
 tests/libqos/virtio.h            |  18 +-
 tests/libqos/pci.c               |  30 ++-
 tests/libqos/virtio-9p.c         |   6 +
 tests/libqos/virtio-mmio.c       |  38 ++-
 tests/libqos/virtio-net.c        |   6 +-
 tests/libqos/virtio-pci-modern.c | 443 +++++++++++++++++++++++++++++++
 tests/libqos/virtio-pci.c        | 105 +++++---
 tests/libqos/virtio.c            | 166 +++++++++---
 tests/virtio-blk-test.c          |  66 +++--
 tests/virtio-scsi-test.c         |   8 +
 15 files changed, 805 insertions(+), 136 deletions(-)
 create mode 100644 tests/libqos/virtio-pci-modern.h
 create mode 100644 tests/libqos/virtio-pci-modern.c

-- 
2.21.0

[PATCH v3 01/16] tests/virtio-blk-test: read config space after feature negotiation

[Stefan Hajnoczi]

The VIRTIO Configuration Space cannot be accessed before device feature
bits have been read because a driver doesn't know the endianness until
it has checked VIRTIO_F_VERSION_1.

Fix this problem in preparation for VIRTIO 1.0 support.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-blk-test.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index ed13167392..f6674fb233 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -125,10 +125,6 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
     char *data;
     QTestState *qts = global_qtest;
 
-    capacity = qvirtio_config_readq(dev, 0);
-
-    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
-
     features = qvirtio_get_features(dev);
     features = features & ~(QVIRTIO_F_BAD_FEATURE |
                     (1u << VIRTIO_RING_F_INDIRECT_DESC) |
@@ -136,6 +132,9 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
                     (1u << VIRTIO_BLK_F_SCSI));
     qvirtio_set_features(dev, features);
 
+    capacity = qvirtio_config_readq(dev, 0);
+    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
+
     qvirtio_set_driver_ok(dev);
 
     /* Write and read with 3 descriptor layout */
@@ -359,9 +358,6 @@ static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc)
     char *data;
     QTestState *qts = global_qtest;
 
-    capacity = qvirtio_config_readq(dev, 0);
-    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
-
     features = qvirtio_get_features(dev);
     g_assert_cmphex(features & (1u << VIRTIO_RING_F_INDIRECT_DESC), !=, 0);
     features = features & ~(QVIRTIO_F_BAD_FEATURE |
@@ -369,6 +365,9 @@ static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc)
                             (1u << VIRTIO_BLK_F_SCSI));
     qvirtio_set_features(dev, features);
 
+    capacity = qvirtio_config_readq(dev, 0);
+    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
+
     vq = qvirtqueue_setup(dev, t_alloc, 0);
     qvirtio_set_driver_ok(dev);
 
@@ -434,8 +433,16 @@ static void config(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtioBlk *blk_if = obj;
     QVirtioDevice *dev = blk_if->vdev;
     int n_size = TEST_IMAGE_SIZE / 2;
+    uint64_t features;
     uint64_t capacity;
 
+    features = qvirtio_get_features(dev);
+    features = features & ~(QVIRTIO_F_BAD_FEATURE |
+                            (1u << VIRTIO_RING_F_INDIRECT_DESC) |
+                            (1u << VIRTIO_RING_F_EVENT_IDX) |
+                            (1u << VIRTIO_BLK_F_SCSI));
+    qvirtio_set_features(dev, features);
+
     capacity = qvirtio_config_readq(dev, 0);
     g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
 
@@ -475,9 +482,6 @@ static void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
     qpci_msix_enable(pdev->pdev);
     qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
 
-    capacity = qvirtio_config_readq(dev, 0);
-    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
-
     features = qvirtio_get_features(dev);
     features = features & ~(QVIRTIO_F_BAD_FEATURE |
                             (1u << VIRTIO_RING_F_INDIRECT_DESC) |
@@ -485,6 +489,9 @@ static void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
                             (1u << VIRTIO_BLK_F_SCSI));
     qvirtio_set_features(dev, features);
 
+    capacity = qvirtio_config_readq(dev, 0);
+    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
+
     vq = qvirtqueue_setup(dev, t_alloc, 0);
     qvirtqueue_pci_msix_setup(pdev, (QVirtQueuePCI *)vq, t_alloc, 1);
 
@@ -584,9 +591,6 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
     qpci_msix_enable(pdev->pdev);
     qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
 
-    capacity = qvirtio_config_readq(dev, 0);
-    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
-
     features = qvirtio_get_features(dev);
     features = features & ~(QVIRTIO_F_BAD_FEATURE |
                             (1u << VIRTIO_RING_F_INDIRECT_DESC) |
@@ -594,6 +598,9 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
                             (1u << VIRTIO_BLK_F_SCSI));
     qvirtio_set_features(dev, features);
 
+    capacity = qvirtio_config_readq(dev, 0);
+    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
+
     vq = qvirtqueue_setup(dev, t_alloc, 0);
     qvirtqueue_pci_msix_setup(pdev, (QVirtQueuePCI *)vq, t_alloc, 1);
 
-- 
2.21.0

Re: [PATCH v3 01/16] tests/virtio-blk-test: read config space after feature negotiation

[Thomas Huth]

On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> The VIRTIO Configuration Space cannot be accessed before device feature
> bits have been read because a driver doesn't know the endianness until
> it has checked VIRTIO_F_VERSION_1.
> 
> Fix this problem in preparation for VIRTIO 1.0 support.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/virtio-blk-test.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)

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

[PATCH v3 02/16] libqos: read QVIRTIO_MMIO_VERSION register

[Stefan Hajnoczi]

There was no real virtio-mmio ABI change between Legacy and VIRTIO 1.0
except that the Version field was incremented from 1 to 2.

However, QEMU does not allow Legacy drivers to perform VIRTIO 1.0
operations like accessing 64-bit feature bits.  Since we will introduce
64-bit feature bit support we need a way to differentiate between
virtio-mmio Version 1 and 2 to avoid upsetting QEMU when we operate in
Legacy mode.

Stash away the Version field so later patches can change behavior
depending on the version.

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

diff --git a/tests/libqos/virtio-mmio.h b/tests/libqos/virtio-mmio.h
index 17a17141c3..0e45778b07 100644
--- a/tests/libqos/virtio-mmio.h
+++ b/tests/libqos/virtio-mmio.h
@@ -40,6 +40,7 @@ typedef struct QVirtioMMIODevice {
     uint64_t addr;
     uint32_t page_size;
     uint32_t features; /* As it cannot be read later, save it */
+    uint32_t version;
 } QVirtioMMIODevice;
 
 extern const QVirtioBus qvirtio_mmio;
diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c
index d0047876a8..7154b03c1d 100644
--- a/tests/libqos/virtio-mmio.c
+++ b/tests/libqos/virtio-mmio.c
@@ -223,6 +223,9 @@ void qvirtio_mmio_init_device(QVirtioMMIODevice *dev, QTestState *qts,
     magic = qtest_readl(qts, addr + QVIRTIO_MMIO_MAGIC_VALUE);
     g_assert(magic == ('v' | 'i' << 8 | 'r' << 16 | 't' << 24));
 
+    dev->version = qtest_readl(qts, addr + QVIRTIO_MMIO_VERSION);
+    g_assert(dev->version == 1 || dev->version == 2);
+
     dev->qts = qts;
     dev->addr = addr;
     dev->page_size = page_size;
-- 
2.21.0

Re: [PATCH v3 02/16] libqos: read QVIRTIO_MMIO_VERSION register

[Thomas Huth]

On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> There was no real virtio-mmio ABI change between Legacy and VIRTIO 1.0
> except that the Version field was incremented from 1 to 2.
> 
> However, QEMU does not allow Legacy drivers to perform VIRTIO 1.0
> operations like accessing 64-bit feature bits.  Since we will introduce
> 64-bit feature bit support we need a way to differentiate between
> virtio-mmio Version 1 and 2 to avoid upsetting QEMU when we operate in
> Legacy mode.
> 
> Stash away the Version field so later patches can change behavior
> depending on the version.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-mmio.h | 1 +
>  tests/libqos/virtio-mmio.c | 3 +++
>  2 files changed, 4 insertions(+)

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

[PATCH v3 03/16] libqos: extend feature bits to 64-bit

[Stefan Hajnoczi]

In VIRTIO 1.0 feature bits changed from 32-bit to 64-bit.  (In fact, the
transports allow even more feature bits but nothing uses more than 64
bits today.)

Add 64-bit feature bit support to virtio-mmio and virtio-pci.  This will
be necessary for VIRTIO 1.0 support.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.h      | 12 ++++++------
 tests/libqos/virtio-mmio.c | 28 ++++++++++++++++++++++------
 tests/libqos/virtio-net.c  |  6 +++---
 tests/libqos/virtio-pci.c  | 12 ++++++------
 tests/libqos/virtio.c      |  4 ++--
 tests/virtio-blk-test.c    |  8 ++++----
 6 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index 2cb2448f46..a5c99fb3c9 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -13,7 +13,7 @@
 #include "libqos/malloc.h"
 #include "standard-headers/linux/virtio_ring.h"
 
-#define QVIRTIO_F_BAD_FEATURE           0x40000000
+#define QVIRTIO_F_BAD_FEATURE           0x40000000ull
 
 typedef struct QVirtioBus QVirtioBus;
 
@@ -52,13 +52,13 @@ struct QVirtioBus {
     uint64_t (*config_readq)(QVirtioDevice *d, uint64_t addr);
 
     /* Get features of the device */
-    uint32_t (*get_features)(QVirtioDevice *d);
+    uint64_t (*get_features)(QVirtioDevice *d);
 
     /* Set features of the device */
-    void (*set_features)(QVirtioDevice *d, uint32_t features);
+    void (*set_features)(QVirtioDevice *d, uint64_t features);
 
     /* Get features of the guest */
-    uint32_t (*get_guest_features)(QVirtioDevice *d);
+    uint64_t (*get_guest_features)(QVirtioDevice *d);
 
     /* Get status of the device */
     uint8_t (*get_status)(QVirtioDevice *d);
@@ -103,8 +103,8 @@ uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr);
 uint16_t qvirtio_config_readw(QVirtioDevice *d, uint64_t addr);
 uint32_t qvirtio_config_readl(QVirtioDevice *d, uint64_t addr);
 uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr);
-uint32_t qvirtio_get_features(QVirtioDevice *d);
-void qvirtio_set_features(QVirtioDevice *d, uint32_t features);
+uint64_t qvirtio_get_features(QVirtioDevice *d);
+void qvirtio_set_features(QVirtioDevice *d, uint64_t features);
 bool qvirtio_is_big_endian(QVirtioDevice *d);
 
 void qvirtio_reset(QVirtioDevice *d);
diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c
index 7154b03c1d..78066e6e05 100644
--- a/tests/libqos/virtio-mmio.c
+++ b/tests/libqos/virtio-mmio.c
@@ -40,22 +40,38 @@ static uint64_t qvirtio_mmio_config_readq(QVirtioDevice *d, uint64_t off)
     return qtest_readq(dev->qts, dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off);
 }
 
-static uint32_t qvirtio_mmio_get_features(QVirtioDevice *d)
+static uint64_t qvirtio_mmio_get_features(QVirtioDevice *d)
 {
     QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
+    uint64_t lo;
+    uint64_t hi = 0;
+
     qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES_SEL, 0);
-    return qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES);
+    lo = qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES);
+
+    if (dev->version >= 2) {
+        qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES_SEL, 1);
+        hi = qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES);
+    }
+
+    return (hi << 32) | lo;
 }
 
-static void qvirtio_mmio_set_features(QVirtioDevice *d, uint32_t features)
+static void qvirtio_mmio_set_features(QVirtioDevice *d, uint64_t features)
 {
     QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
     dev->features = features;
     qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES_SEL, 0);
     qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES, features);
+
+    if (dev->version >= 2) {
+        qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES_SEL, 1);
+        qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES,
+                     features >> 32);
+    }
 }
 
-static uint32_t qvirtio_mmio_get_guest_features(QVirtioDevice *d)
+static uint64_t qvirtio_mmio_get_guest_features(QVirtioDevice *d)
 {
     QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
     return dev->features;
@@ -149,8 +165,8 @@ static QVirtQueue *qvirtio_mmio_virtqueue_setup(QVirtioDevice *d,
     vq->free_head = 0;
     vq->num_free = vq->size;
     vq->align = dev->page_size;
-    vq->indirect = (dev->features & (1u << VIRTIO_RING_F_INDIRECT_DESC)) != 0;
-    vq->event = (dev->features & (1u << VIRTIO_RING_F_EVENT_IDX)) != 0;
+    vq->indirect = dev->features & (1ull << VIRTIO_RING_F_INDIRECT_DESC);
+    vq->event = dev->features & (1ull << VIRTIO_RING_F_EVENT_IDX);
 
     qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_NUM, vq->size);
 
diff --git a/tests/libqos/virtio-net.c b/tests/libqos/virtio-net.c
index 6567beb553..710d440c3d 100644
--- a/tests/libqos/virtio-net.c
+++ b/tests/libqos/virtio-net.c
@@ -44,11 +44,11 @@ static void virtio_net_setup(QVirtioNet *interface)
 
     features = qvirtio_get_features(vdev);
     features &= ~(QVIRTIO_F_BAD_FEATURE |
-                  (1u << VIRTIO_RING_F_INDIRECT_DESC) |
-                  (1u << VIRTIO_RING_F_EVENT_IDX));
+                  (1ull << VIRTIO_RING_F_INDIRECT_DESC) |
+                  (1ull << VIRTIO_RING_F_EVENT_IDX));
     qvirtio_set_features(vdev, features);
 
-    if (features & (1u << VIRTIO_NET_F_MQ)) {
+    if (features & (1ull << VIRTIO_NET_F_MQ)) {
         interface->n_queues = qvirtio_config_readw(vdev, 8) * 2;
     } else {
         interface->n_queues = 2;
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 50499e75ef..1b6b760fc6 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -96,19 +96,19 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t off)
     return val;
 }
 
-static uint32_t qvirtio_pci_get_features(QVirtioDevice *d)
+static uint64_t qvirtio_pci_get_features(QVirtioDevice *d)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
     return qpci_io_readl(dev->pdev, dev->bar, VIRTIO_PCI_HOST_FEATURES);
 }
 
-static void qvirtio_pci_set_features(QVirtioDevice *d, uint32_t features)
+static void qvirtio_pci_set_features(QVirtioDevice *d, uint64_t features)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
     qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_GUEST_FEATURES, features);
 }
 
-static uint32_t qvirtio_pci_get_guest_features(QVirtioDevice *d)
+static uint64_t qvirtio_pci_get_guest_features(QVirtioDevice *d)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
     return qpci_io_readl(dev->pdev, dev->bar, VIRTIO_PCI_GUEST_FEATURES);
@@ -208,7 +208,7 @@ static void qvirtio_pci_set_queue_address(QVirtioDevice *d, uint32_t pfn)
 static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
                                         QGuestAllocator *alloc, uint16_t index)
 {
-    uint32_t feat;
+    uint64_t feat;
     uint64_t addr;
     QVirtQueuePCI *vqpci;
     QVirtioPCIDevice *qvpcidev = container_of(d, QVirtioPCIDevice, vdev);
@@ -222,8 +222,8 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
     vqpci->vq.free_head = 0;
     vqpci->vq.num_free = vqpci->vq.size;
     vqpci->vq.align = VIRTIO_PCI_VRING_ALIGN;
-    vqpci->vq.indirect = (feat & (1u << VIRTIO_RING_F_INDIRECT_DESC)) != 0;
-    vqpci->vq.event = (feat & (1u << VIRTIO_RING_F_EVENT_IDX)) != 0;
+    vqpci->vq.indirect = feat & (1ull << VIRTIO_RING_F_INDIRECT_DESC);
+    vqpci->vq.event = feat & (1ull << VIRTIO_RING_F_EVENT_IDX);
 
     vqpci->msix_entry = -1;
     vqpci->msix_addr = 0;
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 0ae9956fc8..4f7e6bb8a1 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -33,12 +33,12 @@ uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr)
     return d->bus->config_readq(d, addr);
 }
 
-uint32_t qvirtio_get_features(QVirtioDevice *d)
+uint64_t qvirtio_get_features(QVirtioDevice *d)
 {
     return d->bus->get_features(d);
 }
 
-void qvirtio_set_features(QVirtioDevice *d, uint32_t features)
+void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
 {
     d->features = features;
     d->bus->set_features(d, features);
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index f6674fb233..31680cc159 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -119,7 +119,7 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
     QVirtioBlkReq req;
     uint64_t req_addr;
     uint64_t capacity;
-    uint32_t features;
+    uint64_t features;
     uint32_t free_head;
     uint8_t status;
     char *data;
@@ -352,7 +352,7 @@ static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc)
     QVRingIndirectDesc *indirect;
     uint64_t req_addr;
     uint64_t capacity;
-    uint32_t features;
+    uint64_t features;
     uint32_t free_head;
     uint8_t status;
     char *data;
@@ -467,7 +467,7 @@ static void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
     int n_size = TEST_IMAGE_SIZE / 2;
     uint64_t req_addr;
     uint64_t capacity;
-    uint32_t features;
+    uint64_t features;
     uint32_t free_head;
     uint8_t status;
     char *data;
@@ -574,7 +574,7 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
     QVirtioBlkReq req;
     uint64_t req_addr;
     uint64_t capacity;
-    uint32_t features;
+    uint64_t features;
     uint32_t free_head;
     uint32_t write_head;
     uint32_t desc_idx;
-- 
2.21.0

Re: [PATCH v3 03/16] libqos: extend feature bits to 64-bit

[Thomas Huth]

On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> In VIRTIO 1.0 feature bits changed from 32-bit to 64-bit.  (In fact, the
> transports allow even more feature bits but nothing uses more than 64
> bits today.)
> 
> Add 64-bit feature bit support to virtio-mmio and virtio-pci.  This will
> be necessary for VIRTIO 1.0 support.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.h      | 12 ++++++------
>  tests/libqos/virtio-mmio.c | 28 ++++++++++++++++++++++------
>  tests/libqos/virtio-net.c  |  6 +++---
>  tests/libqos/virtio-pci.c  | 12 ++++++------
>  tests/libqos/virtio.c      |  4 ++--
>  tests/virtio-blk-test.c    |  8 ++++----
>  6 files changed, 43 insertions(+), 27 deletions(-)

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

Re: [PATCH v3 03/16] libqos: extend feature bits to 64-bit

[Philippe Mathieu-Daudé]

On 10/19/19 8:37 AM, Stefan Hajnoczi wrote:
> In VIRTIO 1.0 feature bits changed from 32-bit to 64-bit.  (In fact, the
> transports allow even more feature bits but nothing uses more than 64
> bits today.)
> 
> Add 64-bit feature bit support to virtio-mmio and virtio-pci.  This will
> be necessary for VIRTIO 1.0 support.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   tests/libqos/virtio.h      | 12 ++++++------
>   tests/libqos/virtio-mmio.c | 28 ++++++++++++++++++++++------
>   tests/libqos/virtio-net.c  |  6 +++---
>   tests/libqos/virtio-pci.c  | 12 ++++++------
>   tests/libqos/virtio.c      |  4 ++--
>   tests/virtio-blk-test.c    |  8 ++++----
>   6 files changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> index 2cb2448f46..a5c99fb3c9 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -13,7 +13,7 @@
>   #include "libqos/malloc.h"
>   #include "standard-headers/linux/virtio_ring.h"
>   
> -#define QVIRTIO_F_BAD_FEATURE           0x40000000
> +#define QVIRTIO_F_BAD_FEATURE           0x40000000ull
>   
>   typedef struct QVirtioBus QVirtioBus;
>   
> @@ -52,13 +52,13 @@ struct QVirtioBus {
>       uint64_t (*config_readq)(QVirtioDevice *d, uint64_t addr);
>   
>       /* Get features of the device */
> -    uint32_t (*get_features)(QVirtioDevice *d);
> +    uint64_t (*get_features)(QVirtioDevice *d);
>   
>       /* Set features of the device */
> -    void (*set_features)(QVirtioDevice *d, uint32_t features);
> +    void (*set_features)(QVirtioDevice *d, uint64_t features);
>   
>       /* Get features of the guest */
> -    uint32_t (*get_guest_features)(QVirtioDevice *d);
> +    uint64_t (*get_guest_features)(QVirtioDevice *d);
>   
>       /* Get status of the device */
>       uint8_t (*get_status)(QVirtioDevice *d);
> @@ -103,8 +103,8 @@ uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr);
>   uint16_t qvirtio_config_readw(QVirtioDevice *d, uint64_t addr);
>   uint32_t qvirtio_config_readl(QVirtioDevice *d, uint64_t addr);
>   uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr);
> -uint32_t qvirtio_get_features(QVirtioDevice *d);
> -void qvirtio_set_features(QVirtioDevice *d, uint32_t features);
> +uint64_t qvirtio_get_features(QVirtioDevice *d);
> +void qvirtio_set_features(QVirtioDevice *d, uint64_t features);
>   bool qvirtio_is_big_endian(QVirtioDevice *d);
>   
>   void qvirtio_reset(QVirtioDevice *d);
> diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c
> index 7154b03c1d..78066e6e05 100644
> --- a/tests/libqos/virtio-mmio.c
> +++ b/tests/libqos/virtio-mmio.c
> @@ -40,22 +40,38 @@ static uint64_t qvirtio_mmio_config_readq(QVirtioDevice *d, uint64_t off)
>       return qtest_readq(dev->qts, dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off);
>   }
>   
> -static uint32_t qvirtio_mmio_get_features(QVirtioDevice *d)
> +static uint64_t qvirtio_mmio_get_features(QVirtioDevice *d)
>   {
>       QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
> +    uint64_t lo;
> +    uint64_t hi = 0;
> +
>       qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES_SEL, 0);
> -    return qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES);
> +    lo = qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES);
> +
> +    if (dev->version >= 2) {
> +        qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES_SEL, 1);
> +        hi = qtest_readl(dev->qts, dev->addr + QVIRTIO_MMIO_HOST_FEATURES);
> +    }
> +
> +    return (hi << 32) | lo;
>   }
>   
> -static void qvirtio_mmio_set_features(QVirtioDevice *d, uint32_t features)
> +static void qvirtio_mmio_set_features(QVirtioDevice *d, uint64_t features)
>   {
>       QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
>       dev->features = features;
>       qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES_SEL, 0);
>       qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES, features);
> +
> +    if (dev->version >= 2) {
> +        qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES_SEL, 1);
> +        qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_GUEST_FEATURES,
> +                     features >> 32);
> +    }
>   }
>   
> -static uint32_t qvirtio_mmio_get_guest_features(QVirtioDevice *d)
> +static uint64_t qvirtio_mmio_get_guest_features(QVirtioDevice *d)
>   {
>       QVirtioMMIODevice *dev = container_of(d, QVirtioMMIODevice, vdev);
>       return dev->features;
> @@ -149,8 +165,8 @@ static QVirtQueue *qvirtio_mmio_virtqueue_setup(QVirtioDevice *d,
>       vq->free_head = 0;
>       vq->num_free = vq->size;
>       vq->align = dev->page_size;
> -    vq->indirect = (dev->features & (1u << VIRTIO_RING_F_INDIRECT_DESC)) != 0;
> -    vq->event = (dev->features & (1u << VIRTIO_RING_F_EVENT_IDX)) != 0;
> +    vq->indirect = dev->features & (1ull << VIRTIO_RING_F_INDIRECT_DESC);
> +    vq->event = dev->features & (1ull << VIRTIO_RING_F_EVENT_IDX);
>   
>       qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_NUM, vq->size);
>   
> diff --git a/tests/libqos/virtio-net.c b/tests/libqos/virtio-net.c
> index 6567beb553..710d440c3d 100644
> --- a/tests/libqos/virtio-net.c
> +++ b/tests/libqos/virtio-net.c
> @@ -44,11 +44,11 @@ static void virtio_net_setup(QVirtioNet *interface)
>   
>       features = qvirtio_get_features(vdev);
>       features &= ~(QVIRTIO_F_BAD_FEATURE |
> -                  (1u << VIRTIO_RING_F_INDIRECT_DESC) |
> -                  (1u << VIRTIO_RING_F_EVENT_IDX));
> +                  (1ull << VIRTIO_RING_F_INDIRECT_DESC) |
> +                  (1ull << VIRTIO_RING_F_EVENT_IDX));
>       qvirtio_set_features(vdev, features);
>   
> -    if (features & (1u << VIRTIO_NET_F_MQ)) {
> +    if (features & (1ull << VIRTIO_NET_F_MQ)) {
>           interface->n_queues = qvirtio_config_readw(vdev, 8) * 2;
>       } else {
>           interface->n_queues = 2;
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 50499e75ef..1b6b760fc6 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -96,19 +96,19 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t off)
>       return val;
>   }
>   
> -static uint32_t qvirtio_pci_get_features(QVirtioDevice *d)
> +static uint64_t qvirtio_pci_get_features(QVirtioDevice *d)
>   {
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
>       return qpci_io_readl(dev->pdev, dev->bar, VIRTIO_PCI_HOST_FEATURES);
>   }
>   
> -static void qvirtio_pci_set_features(QVirtioDevice *d, uint32_t features)
> +static void qvirtio_pci_set_features(QVirtioDevice *d, uint64_t features)
>   {
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
>       qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_GUEST_FEATURES, features);
>   }
>   
> -static uint32_t qvirtio_pci_get_guest_features(QVirtioDevice *d)
> +static uint64_t qvirtio_pci_get_guest_features(QVirtioDevice *d)
>   {
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
>       return qpci_io_readl(dev->pdev, dev->bar, VIRTIO_PCI_GUEST_FEATURES);
> @@ -208,7 +208,7 @@ static void qvirtio_pci_set_queue_address(QVirtioDevice *d, uint32_t pfn)
>   static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
>                                           QGuestAllocator *alloc, uint16_t index)
>   {
> -    uint32_t feat;
> +    uint64_t feat;
>       uint64_t addr;
>       QVirtQueuePCI *vqpci;
>       QVirtioPCIDevice *qvpcidev = container_of(d, QVirtioPCIDevice, vdev);
> @@ -222,8 +222,8 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
>       vqpci->vq.free_head = 0;
>       vqpci->vq.num_free = vqpci->vq.size;
>       vqpci->vq.align = VIRTIO_PCI_VRING_ALIGN;
> -    vqpci->vq.indirect = (feat & (1u << VIRTIO_RING_F_INDIRECT_DESC)) != 0;
> -    vqpci->vq.event = (feat & (1u << VIRTIO_RING_F_EVENT_IDX)) != 0;
> +    vqpci->vq.indirect = feat & (1ull << VIRTIO_RING_F_INDIRECT_DESC);
> +    vqpci->vq.event = feat & (1ull << VIRTIO_RING_F_EVENT_IDX);
>   
>       vqpci->msix_entry = -1;
>       vqpci->msix_addr = 0;
> diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
> index 0ae9956fc8..4f7e6bb8a1 100644
> --- a/tests/libqos/virtio.c
> +++ b/tests/libqos/virtio.c
> @@ -33,12 +33,12 @@ uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr)
>       return d->bus->config_readq(d, addr);
>   }
>   
> -uint32_t qvirtio_get_features(QVirtioDevice *d)
> +uint64_t qvirtio_get_features(QVirtioDevice *d)
>   {
>       return d->bus->get_features(d);
>   }
>   
> -void qvirtio_set_features(QVirtioDevice *d, uint32_t features)
> +void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
>   {
>       d->features = features;
>       d->bus->set_features(d, features);
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index f6674fb233..31680cc159 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -119,7 +119,7 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
>       QVirtioBlkReq req;
>       uint64_t req_addr;
>       uint64_t capacity;
> -    uint32_t features;
> +    uint64_t features;
>       uint32_t free_head;
>       uint8_t status;
>       char *data;
> @@ -352,7 +352,7 @@ static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc)
>       QVRingIndirectDesc *indirect;
>       uint64_t req_addr;
>       uint64_t capacity;
> -    uint32_t features;
> +    uint64_t features;
>       uint32_t free_head;
>       uint8_t status;
>       char *data;
> @@ -467,7 +467,7 @@ static void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
>       int n_size = TEST_IMAGE_SIZE / 2;
>       uint64_t req_addr;
>       uint64_t capacity;
> -    uint32_t features;
> +    uint64_t features;
>       uint32_t free_head;
>       uint8_t status;
>       char *data;
> @@ -574,7 +574,7 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
>       QVirtioBlkReq req;
>       uint64_t req_addr;
>       uint64_t capacity;
> -    uint32_t features;
> +    uint64_t features;
>       uint32_t free_head;
>       uint32_t write_head;
>       uint32_t desc_idx;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 04/16] virtio-scsi-test: add missing feature negotiation

[Stefan Hajnoczi]

VIRTIO Device Initialization requires feature negotiation.  Currently
virtio-scsi-test.c is non-compliant.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-scsi-test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 7c8f9b27f8..0415e75876 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev)
     QVirtioSCSIQueues *vs;
     const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {};
     struct virtio_scsi_cmd_resp resp;
+    uint64_t features;
     int i;
 
     vs = g_new0(QVirtioSCSIQueues, 1);
     vs->dev = dev;
+
+    features = qvirtio_get_features(dev);
+    features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX));
+    qvirtio_set_features(dev, features);
+
     vs->num_queues = qvirtio_config_readl(dev, 0);
 
     g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES);
@@ -135,6 +141,8 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev)
         vs->vq[i] = qvirtqueue_setup(dev, alloc, i);
     }
 
+    qvirtio_set_driver_ok(dev);
+
     /* Clear the POWER ON OCCURRED unit attention */
     g_assert_cmpint(virtio_scsi_do_command(vs, test_unit_ready_cdb,
                                            NULL, 0, NULL, 0, &resp),
-- 
2.21.0

Re: [PATCH v3 04/16] virtio-scsi-test: add missing feature negotiation

[Thomas Huth]

On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> VIRTIO Device Initialization requires feature negotiation.  Currently
> virtio-scsi-test.c is non-compliant.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/virtio-scsi-test.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index 7c8f9b27f8..0415e75876 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev)
>      QVirtioSCSIQueues *vs;
>      const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {};
>      struct virtio_scsi_cmd_resp resp;
> +    uint64_t features;
>      int i;
>  
>      vs = g_new0(QVirtioSCSIQueues, 1);
>      vs->dev = dev;
> +
> +    features = qvirtio_get_features(dev);
> +    features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX));
> +    qvirtio_set_features(dev, features);

I wonder whether this get_feat + "&=" + set_feat is really the right way
here? Maybe we should rather only set the feature bits that we care
about instead of setting all but BAD_FEATURE and RING_F_EVENT_IDX?

Otherwise, please mention in the commit message why you've chosen to
disable just these two bits.

 Thanks,
  Thomas

Re: [PATCH v3 04/16] virtio-scsi-test: add missing feature negotiation

[Stefan Hajnoczi]

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

On Mon, Oct 21, 2019 at 02:08:34PM +0200, Thomas Huth wrote:
> On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> > VIRTIO Device Initialization requires feature negotiation.  Currently
> > virtio-scsi-test.c is non-compliant.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/virtio-scsi-test.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> > index 7c8f9b27f8..0415e75876 100644
> > --- a/tests/virtio-scsi-test.c
> > +++ b/tests/virtio-scsi-test.c
> > @@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev)
> >      QVirtioSCSIQueues *vs;
> >      const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {};
> >      struct virtio_scsi_cmd_resp resp;
> > +    uint64_t features;
> >      int i;
> >  
> >      vs = g_new0(QVirtioSCSIQueues, 1);
> >      vs->dev = dev;
> > +
> > +    features = qvirtio_get_features(dev);
> > +    features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX));
> > +    qvirtio_set_features(dev, features);
> 
> I wonder whether this get_feat + "&=" + set_feat is really the right way
> here? Maybe we should rather only set the feature bits that we care
> about instead of setting all but BAD_FEATURE and RING_F_EVENT_IDX?
> 
> Otherwise, please mention in the commit message why you've chosen to
> disable just these two bits.

Enabling all features (except BAD_FEATURE and EVENT_IDX) is standard
practice in virtio qtests that I didn't want to diverge from.

I can mention this in the commit description and explain that
BAD_FEATURE must never be set (i.e. drivers that do so are considered
broken) and EVENT_IDX isn't supported by the libqos virtio code.

Stefan

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

[PATCH v3 05/16] tests/virtio-blk-test: set up virtqueue after feature negotiation

[Stefan Hajnoczi]

VIRTIO Device Initialization requires that feature negotiation has
completed before virtqueues are set up.  This makes sense because the
driver must know whether it is operating in Legacy or VIRTIO 1.0 mode
before it can access vring fields with the correct endianness.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-blk-test.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 31680cc159..fe0dc4a896 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -113,8 +113,8 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d,
     return addr;
 }
 
-static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
-                       QVirtQueue *vq)
+/* Returns the request virtqueue so the caller can perform further tests */
+static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc)
 {
     QVirtioBlkReq req;
     uint64_t req_addr;
@@ -124,6 +124,7 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
     uint8_t status;
     char *data;
     QTestState *qts = global_qtest;
+    QVirtQueue *vq;
 
     features = qvirtio_get_features(dev);
     features = features & ~(QVIRTIO_F_BAD_FEATURE |
@@ -135,6 +136,8 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
     capacity = qvirtio_config_readq(dev, 0);
     g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
 
+    vq = qvirtqueue_setup(dev, alloc, 0);
+
     qvirtio_set_driver_ok(dev);
 
     /* Write and read with 3 descriptor layout */
@@ -331,14 +334,16 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
 
         guest_free(alloc, req_addr);
     }
+
+    return vq;
 }
 
 static void basic(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtioBlk *blk_if = obj;
     QVirtQueue *vq;
-    vq = qvirtqueue_setup(blk_if->vdev, t_alloc, 0);
-    test_basic(blk_if->vdev, t_alloc, vq);
+
+    vq = test_basic(blk_if->vdev, t_alloc);
     qvirtqueue_cleanup(blk_if->vdev->bus, vq, t_alloc);
 
 }
@@ -746,9 +751,7 @@ static void resize(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtQueue *vq;
     QTestState *qts = global_qtest;
 
-    vq = qvirtqueue_setup(dev, t_alloc, 0);
-
-    test_basic(dev, t_alloc, vq);
+    vq = test_basic(dev, t_alloc);
 
     qmp_discard_response("{ 'execute': 'block_resize', "
                          " 'arguments': { 'device': 'drive0', "
-- 
2.21.0

Re: [PATCH v3 05/16] tests/virtio-blk-test: set up virtqueue after feature negotiation

[Thomas Huth]

On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> VIRTIO Device Initialization requires that feature negotiation has
> completed before virtqueues are set up.  This makes sense because the
> driver must know whether it is operating in Legacy or VIRTIO 1.0 mode
> before it can access vring fields with the correct endianness.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/virtio-blk-test.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 31680cc159..fe0dc4a896 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -113,8 +113,8 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d,
>      return addr;
>  }
>  
> -static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
> -                       QVirtQueue *vq)
> +/* Returns the request virtqueue so the caller can perform further tests */
> +static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc)
>  {
>      QVirtioBlkReq req;
>      uint64_t req_addr;
> @@ -124,6 +124,7 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
>      uint8_t status;
>      char *data;
>      QTestState *qts = global_qtest;
> +    QVirtQueue *vq;
>  
>      features = qvirtio_get_features(dev);
>      features = features & ~(QVIRTIO_F_BAD_FEATURE |
> @@ -135,6 +136,8 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
>      capacity = qvirtio_config_readq(dev, 0);
>      g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
>  
> +    vq = qvirtqueue_setup(dev, alloc, 0);
> +
>      qvirtio_set_driver_ok(dev);
>  
>      /* Write and read with 3 descriptor layout */
> @@ -331,14 +334,16 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
>  
>          guest_free(alloc, req_addr);
>      }
> +
> +    return vq;
>  }
>  
>  static void basic(void *obj, void *data, QGuestAllocator *t_alloc)
>  {
>      QVirtioBlk *blk_if = obj;
>      QVirtQueue *vq;
> -    vq = qvirtqueue_setup(blk_if->vdev, t_alloc, 0);
> -    test_basic(blk_if->vdev, t_alloc, vq);
> +
> +    vq = test_basic(blk_if->vdev, t_alloc);
>      qvirtqueue_cleanup(blk_if->vdev->bus, vq, t_alloc);
>  
>  }
> @@ -746,9 +751,7 @@ static void resize(void *obj, void *data, QGuestAllocator *t_alloc)
>      QVirtQueue *vq;
>      QTestState *qts = global_qtest;
>  
> -    vq = qvirtqueue_setup(dev, t_alloc, 0);
> -
> -    test_basic(dev, t_alloc, vq);
> +    vq = test_basic(dev, t_alloc);
>  
>      qmp_discard_response("{ 'execute': 'block_resize', "
>                           " 'arguments': { 'device': 'drive0', "
> 

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

[PATCH v3 06/16] libqos: add missing virtio-9p feature negotiation

[Stefan Hajnoczi]

VIRTIO Device Initialization requires feature negotiation.  The libqos
virtio-9p driver lacks feature negotiation and is therefore
non-compliant.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio-9p.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/libqos/virtio-9p.c b/tests/libqos/virtio-9p.c
index 8c9efce3e1..77dbfb62ad 100644
--- a/tests/libqos/virtio-9p.c
+++ b/tests/libqos/virtio-9p.c
@@ -32,6 +32,12 @@ static void virtio_9p_cleanup(QVirtio9P *interface)
 
 static void virtio_9p_setup(QVirtio9P *interface)
 {
+    uint64_t features;
+
+    features = qvirtio_get_features(interface->vdev);
+    features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX));
+    qvirtio_set_features(interface->vdev, features);
+
     interface->vq = qvirtqueue_setup(interface->vdev, alloc, 0);
     qvirtio_set_driver_ok(interface->vdev);
 }
-- 
2.21.0

Re: [PATCH v3 06/16] libqos: add missing virtio-9p feature negotiation

[Thomas Huth]

On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> VIRTIO Device Initialization requires feature negotiation.  The libqos
> virtio-9p driver lacks feature negotiation and is therefore
> non-compliant.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio-9p.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/libqos/virtio-9p.c b/tests/libqos/virtio-9p.c
> index 8c9efce3e1..77dbfb62ad 100644
> --- a/tests/libqos/virtio-9p.c
> +++ b/tests/libqos/virtio-9p.c
> @@ -32,6 +32,12 @@ static void virtio_9p_cleanup(QVirtio9P *interface)
>  
>  static void virtio_9p_setup(QVirtio9P *interface)
>  {
> +    uint64_t features;
> +
> +    features = qvirtio_get_features(interface->vdev);
> +    features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX));
> +    qvirtio_set_features(interface->vdev, features);

Same question as with patch 4: Why just disabling these two bits and not
rather set the bits we know about?

 Thomas

[PATCH v3 07/16] libqos: enforce Device Initialization order

[Stefan Hajnoczi]

According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device
Initialization", configuration space and virtqueues cannot be accessed
before features have been negotiated.  Enforce this requirement.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 4f7e6bb8a1..2593996c98 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -13,23 +13,33 @@
 #include "standard-headers/linux/virtio_config.h"
 #include "standard-headers/linux/virtio_ring.h"
 
+/* Features must be negotiated before config space or virtqueue access */
+static void check_features_negotiated(QVirtioDevice *d)
+{
+    g_assert_cmphex(d->features, !=, 0);
+}
+
 uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr)
 {
+    check_features_negotiated(d);
     return d->bus->config_readb(d, addr);
 }
 
 uint16_t qvirtio_config_readw(QVirtioDevice *d, uint64_t addr)
 {
+    check_features_negotiated(d);
     return d->bus->config_readw(d, addr);
 }
 
 uint32_t qvirtio_config_readl(QVirtioDevice *d, uint64_t addr)
 {
+    check_features_negotiated(d);
     return d->bus->config_readl(d, addr);
 }
 
 uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr)
 {
+    check_features_negotiated(d);
     return d->bus->config_readq(d, addr);
 }
 
@@ -47,6 +57,7 @@ void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
 QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
                              QGuestAllocator *alloc, uint16_t index)
 {
+    check_features_negotiated(d);
     return d->bus->virtqueue_setup(d, alloc, index);
 }
 
-- 
2.21.0

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

[Thomas Huth]

On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device
> Initialization", configuration space and virtqueues cannot be accessed
> before features have been negotiated.  Enforce this requirement.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
> index 4f7e6bb8a1..2593996c98 100644
> --- a/tests/libqos/virtio.c
> +++ b/tests/libqos/virtio.c
> @@ -13,23 +13,33 @@
>  #include "standard-headers/linux/virtio_config.h"
>  #include "standard-headers/linux/virtio_ring.h"
>  
> +/* Features must be negotiated before config space or virtqueue access */
> +static void check_features_negotiated(QVirtioDevice *d)
> +{
> +    g_assert_cmphex(d->features, !=, 0);
> +}

Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)?

 Thomas

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

[Stefan Hajnoczi]

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

On Mon, Oct 21, 2019 at 02:15:53PM +0200, Thomas Huth wrote:
> On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> > According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device
> > Initialization", configuration space and virtqueues cannot be accessed
> > before features have been negotiated.  Enforce this requirement.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/libqos/virtio.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
> > index 4f7e6bb8a1..2593996c98 100644
> > --- a/tests/libqos/virtio.c
> > +++ b/tests/libqos/virtio.c
> > @@ -13,23 +13,33 @@
> >  #include "standard-headers/linux/virtio_config.h"
> >  #include "standard-headers/linux/virtio_ring.h"
> >  
> > +/* Features must be negotiated before config space or virtqueue access */
> > +static void check_features_negotiated(QVirtioDevice *d)
> > +{
> > +    g_assert_cmphex(d->features, !=, 0);
> > +}
> 
> Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)?

Yes, it's possible for Legacy devices.  If someone ever does that
they'll need to extend this code, but it's unlikely so I'd rather not
complicate this.

Stefan

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

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

[Thomas Huth]

On 22/10/2019 17.48, Stefan Hajnoczi wrote:
> On Mon, Oct 21, 2019 at 02:15:53PM +0200, Thomas Huth wrote:
>> On 19/10/2019 08.38, Stefan Hajnoczi wrote:
>>> According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device
>>> Initialization", configuration space and virtqueues cannot be accessed
>>> before features have been negotiated.  Enforce this requirement.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  tests/libqos/virtio.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
>>> index 4f7e6bb8a1..2593996c98 100644
>>> --- a/tests/libqos/virtio.c
>>> +++ b/tests/libqos/virtio.c
>>> @@ -13,23 +13,33 @@
>>>  #include "standard-headers/linux/virtio_config.h"
>>>  #include "standard-headers/linux/virtio_ring.h"
>>>  
>>> +/* Features must be negotiated before config space or virtqueue access */
>>> +static void check_features_negotiated(QVirtioDevice *d)
>>> +{
>>> +    g_assert_cmphex(d->features, !=, 0);
>>> +}
>>
>> Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)?
> 
> Yes, it's possible for Legacy devices.  If someone ever does that
> they'll need to extend this code, but it's unlikely so I'd rather not
> complicate this.

Could you please add at least a comment here with that explanation?

 Thanks,
  Thomas

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

[Stefan Hajnoczi]

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

On Tue, Oct 22, 2019 at 08:48:31PM +0200, Thomas Huth wrote:
> On 22/10/2019 17.48, Stefan Hajnoczi wrote:
> > On Mon, Oct 21, 2019 at 02:15:53PM +0200, Thomas Huth wrote:
> >> On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> >>> According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device
> >>> Initialization", configuration space and virtqueues cannot be accessed
> >>> before features have been negotiated.  Enforce this requirement.
> >>>
> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>>  tests/libqos/virtio.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
> >>> index 4f7e6bb8a1..2593996c98 100644
> >>> --- a/tests/libqos/virtio.c
> >>> +++ b/tests/libqos/virtio.c
> >>> @@ -13,23 +13,33 @@
> >>>  #include "standard-headers/linux/virtio_config.h"
> >>>  #include "standard-headers/linux/virtio_ring.h"
> >>>  
> >>> +/* Features must be negotiated before config space or virtqueue access */
> >>> +static void check_features_negotiated(QVirtioDevice *d)
> >>> +{
> >>> +    g_assert_cmphex(d->features, !=, 0);
> >>> +}
> >>
> >> Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)?
> > 
> > Yes, it's possible for Legacy devices.  If someone ever does that
> > they'll need to extend this code, but it's unlikely so I'd rather not
> > complicate this.
> 
> Could you please add at least a comment here with that explanation?

I'll try adding a bool field in v4 so there are no problems with a 0
feature bit set.

Stefan

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

[PATCH v3 08/16] libqos: implement VIRTIO 1.0 FEATURES_OK step

[Stefan Hajnoczi]

Device initialization has an extra step in VIRTIO 1.0.  The FEATURES_OK
status bit is set to indicate that feature negotiation has completed.
The driver then reads the status register again to check that the device
agrees with the final features.

Implement this step as part of qvirtio_set_features() instead of
introducing a separate function.  This way all existing code works
without modifications.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 2593996c98..57fa79373b 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -52,6 +52,19 @@ void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
 {
     d->features = features;
     d->bus->set_features(d, features);
+
+    /*
+     * This could be a separate function for drivers that want to access
+     * configuration space before setting FEATURES_OK, but no existing users
+     * need that and it's less code for callers if this is done implicitly.
+    */
+    if (features & (1ull << VIRTIO_F_VERSION_1)) {
+        uint8_t status = d->bus->get_status(d) |
+                         VIRTIO_CONFIG_S_FEATURES_OK;
+
+        d->bus->set_status(d, status);
+        g_assert_cmphex(d->bus->get_status(d), ==, status);
+    }
 }
 
 QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
@@ -88,9 +101,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
 
 void qvirtio_set_driver_ok(QVirtioDevice *d)
 {
-    d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
-    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
-                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
+    uint8_t status = d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK;
+
+    d->bus->set_status(d, status);
+    g_assert_cmphex(d->bus->get_status(d), ==, status);
 }
 
 void qvirtio_wait_queue_isr(QTestState *qts, QVirtioDevice *d,
-- 
2.21.0

Re: [PATCH v3 08/16] libqos: implement VIRTIO 1.0 FEATURES_OK step

[Thomas Huth]

On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> Device initialization has an extra step in VIRTIO 1.0.  The FEATURES_OK
> status bit is set to indicate that feature negotiation has completed.
> The driver then reads the status register again to check that the device
> agrees with the final features.
> 
> Implement this step as part of qvirtio_set_features() instead of
> introducing a separate function.  This way all existing code works
> without modifications.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
[...]
> @@ -88,9 +101,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
>  
>  void qvirtio_set_driver_ok(QVirtioDevice *d)
>  {
> -    d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
> -    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
> -                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
> +    uint8_t status = d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK;
> +
> +    d->bus->set_status(d, status);
> +    g_assert_cmphex(d->bus->get_status(d), ==, status);
>  }

The changes to qvirtio_set_driver_ok() are not mentioned in the patch
description. Please either mention them there, or move this to a
separate patch.

 Thanks,
  Thomas

Re: [PATCH v3 08/16] libqos: implement VIRTIO 1.0 FEATURES_OK step

[Stefan Hajnoczi]

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

On Mon, Oct 21, 2019 at 02:23:15PM +0200, Thomas Huth wrote:
> On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> > Device initialization has an extra step in VIRTIO 1.0.  The FEATURES_OK
> > status bit is set to indicate that feature negotiation has completed.
> > The driver then reads the status register again to check that the device
> > agrees with the final features.
> > 
> > Implement this step as part of qvirtio_set_features() instead of
> > introducing a separate function.  This way all existing code works
> > without modifications.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/libqos/virtio.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> [...]
> > @@ -88,9 +101,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
> >  
> >  void qvirtio_set_driver_ok(QVirtioDevice *d)
> >  {
> > -    d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
> > -    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
> > -                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
> > +    uint8_t status = d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK;
> > +
> > +    d->bus->set_status(d, status);
> > +    g_assert_cmphex(d->bus->get_status(d), ==, status);
> >  }
> 
> The changes to qvirtio_set_driver_ok() are not mentioned in the patch
> description. Please either mention them there, or move this to a
> separate patch.

Will fix in v4.

We can no longer hardcode the status register value for DRIVER_OK since
it now depends on whether VIRTIO 1.0 is used or not.

Here is an alternative that is smaller and makes the VIRTIO 1.0
dependency obvious:

  -    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
  -                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
  +    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
  +                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE |
  +                    (d->features & (1ull << VIRTIO_F_VERSION_1) ?
  +                     VIRTIO_CONFIG_S_FEATURES_OK : 0));

Stefan

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

[PATCH v3 09/16] libqos: access VIRTIO 1.0 vring in little-endian

[Stefan Hajnoczi]

VIRTIO 1.0 uses little-endian for the vring.  Legacy VIRTIO uses guest
endianness.  Adjust the code to handle both.

Note that qvirtio_readq() is not defined because it has no users.  All
the other accessors are really needed.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.h      |   4 +-
 tests/libqos/virtio-mmio.c |   1 +
 tests/libqos/virtio-pci.c  |   1 +
 tests/libqos/virtio.c      | 131 +++++++++++++++++++++++++++----------
 tests/virtio-blk-test.c    |   8 +--
 5 files changed, 106 insertions(+), 39 deletions(-)

diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index a5c99fb3c9..5777683a2a 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -26,6 +26,7 @@ typedef struct QVirtioDevice {
 } QVirtioDevice;
 
 typedef struct QVirtQueue {
+    QVirtioDevice *vdev;
     uint64_t desc; /* This points to an array of struct vring_desc */
     uint64_t avail; /* This points to a struct vring_avail */
     uint64_t used; /* This points to a struct vring_used */
@@ -134,7 +135,8 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
 QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
                                                QGuestAllocator *alloc,
                                                uint16_t elem);
-void qvring_indirect_desc_add(QTestState *qts, QVRingIndirectDesc *indirect,
+void qvring_indirect_desc_add(QVirtioDevice *d, QTestState *qts,
+                              QVRingIndirectDesc *indirect,
                               uint64_t data, uint32_t len, bool write);
 uint32_t qvirtqueue_add(QTestState *qts, QVirtQueue *vq, uint64_t data,
                         uint32_t len, bool write, bool next);
diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c
index 78066e6e05..4db1f1b8bc 100644
--- a/tests/libqos/virtio-mmio.c
+++ b/tests/libqos/virtio-mmio.c
@@ -157,6 +157,7 @@ static QVirtQueue *qvirtio_mmio_virtqueue_setup(QVirtioDevice *d,
     uint64_t addr;
 
     vq = g_malloc0(sizeof(*vq));
+    vq->vdev = d;
     qvirtio_mmio_queue_select(d, index);
     qtest_writel(dev->qts, dev->addr + QVIRTIO_MMIO_QUEUE_ALIGN, dev->page_size);
 
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 1b6b760fc6..7ecf5d0a52 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -217,6 +217,7 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
     feat = qvirtio_pci_get_guest_features(d);
 
     qvirtio_pci_queue_select(d, index);
+    vqpci->vq.vdev = d;
     vqpci->vq.index = index;
     vqpci->vq.size = qvirtio_pci_get_queue_size(d);
     vqpci->vq.free_head = 0;
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 57fa79373b..b9b501a3d4 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
 #include "libqtest.h"
 #include "libqos/virtio.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -19,6 +20,62 @@ static void check_features_negotiated(QVirtioDevice *d)
     g_assert_cmphex(d->features, !=, 0);
 }
 
+/*
+ * qtest_readX/writeX() functions transfer host endian from/to guest endian.
+ * This works great for Legacy VIRTIO devices where we need guest endian
+ * accesses.  For VIRTIO 1.0 the vring is little-endian so the automatic guest
+ * endianness conversion is not wanted.
+ *
+ * The following qvirtio_readX/writeX() functions handle Legacy and VIRTIO 1.0
+ * accesses seamlessly.
+ */
+static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, uint64_t addr)
+{
+    uint16_t val = qtest_readw(qts, addr);
+
+    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
+        val = bswap16(val);
+    }
+    return val;
+}
+
+static uint32_t qvirtio_readl(QVirtioDevice *d, QTestState *qts, uint64_t addr)
+{
+    uint32_t val = qtest_readl(qts, addr);
+
+    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
+        val = bswap32(val);
+    }
+    return val;
+}
+
+static void qvirtio_writew(QVirtioDevice *d, QTestState *qts,
+                           uint64_t addr, uint16_t val)
+{
+    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
+        val = bswap16(val);
+    }
+    qtest_writew(qts, addr, val);
+}
+
+static void qvirtio_writel(QVirtioDevice *d, QTestState *qts,
+                           uint64_t addr, uint32_t val)
+{
+    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
+        val = bswap32(val);
+    }
+    qtest_writel(qts, addr, val);
+}
+
+static void qvirtio_writeq(QVirtioDevice *d, QTestState *qts,
+                           uint64_t addr, uint64_t val)
+{
+    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
+        val = bswap64(val);
+    }
+    qtest_writeq(qts, addr, val);
+}
+
 uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr)
 {
     check_features_negotiated(d);
@@ -191,23 +248,23 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
 
     for (i = 0; i < vq->size - 1; i++) {
         /* vq->desc[i].addr */
-        qtest_writeq(qts, vq->desc + (16 * i), 0);
+        qvirtio_writeq(vq->vdev, qts, vq->desc + (16 * i), 0);
         /* vq->desc[i].next */
-        qtest_writew(qts, vq->desc + (16 * i) + 14, i + 1);
+        qvirtio_writew(vq->vdev, qts, vq->desc + (16 * i) + 14, i + 1);
     }
 
     /* vq->avail->flags */
-    qtest_writew(qts, vq->avail, 0);
+    qvirtio_writew(vq->vdev, qts, vq->avail, 0);
     /* vq->avail->idx */
-    qtest_writew(qts, vq->avail + 2, 0);
+    qvirtio_writew(vq->vdev, qts, vq->avail + 2, 0);
     /* vq->avail->used_event */
-    qtest_writew(qts, vq->avail + 4 + (2 * vq->size), 0);
+    qvirtio_writew(vq->vdev, qts, vq->avail + 4 + (2 * vq->size), 0);
 
     /* vq->used->flags */
-    qtest_writew(qts, vq->used, 0);
+    qvirtio_writew(vq->vdev, qts, vq->used, 0);
     /* vq->used->avail_event */
-    qtest_writew(qts, vq->used + 2 + sizeof(struct vring_used_elem) * vq->size,
-                 0);
+    qvirtio_writew(vq->vdev, qts, vq->used + 2 +
+            sizeof(struct vring_used_elem) * vq->size, 0);
 }
 
 QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
@@ -223,35 +280,39 @@ QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
 
     for (i = 0; i < elem - 1; ++i) {
         /* indirect->desc[i].addr */
-        qtest_writeq(qs, indirect->desc + (16 * i), 0);
+        qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
         /* indirect->desc[i].flags */
-        qtest_writew(qs, indirect->desc + (16 * i) + 12, VRING_DESC_F_NEXT);
+        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
+                       VRING_DESC_F_NEXT);
         /* indirect->desc[i].next */
-        qtest_writew(qs, indirect->desc + (16 * i) + 14, i + 1);
+        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
     }
 
     return indirect;
 }
 
-void qvring_indirect_desc_add(QTestState *qts, QVRingIndirectDesc *indirect,
+void qvring_indirect_desc_add(QVirtioDevice *d, QTestState *qts,
+                              QVRingIndirectDesc *indirect,
                               uint64_t data, uint32_t len, bool write)
 {
     uint16_t flags;
 
     g_assert_cmpint(indirect->index, <, indirect->elem);
 
-    flags = qtest_readw(qts, indirect->desc + (16 * indirect->index) + 12);
+    flags = qvirtio_readw(d, qts, indirect->desc +
+                                  (16 * indirect->index) + 12);
 
     if (write) {
         flags |= VRING_DESC_F_WRITE;
     }
 
     /* indirect->desc[indirect->index].addr */
-    qtest_writeq(qts, indirect->desc + (16 * indirect->index), data);
+    qvirtio_writeq(d, qts, indirect->desc + (16 * indirect->index), data);
     /* indirect->desc[indirect->index].len */
-    qtest_writel(qts, indirect->desc + (16 * indirect->index) + 8, len);
+    qvirtio_writel(d, qts, indirect->desc + (16 * indirect->index) + 8, len);
     /* indirect->desc[indirect->index].flags */
-    qtest_writew(qts, indirect->desc + (16 * indirect->index) + 12, flags);
+    qvirtio_writew(d, qts, indirect->desc + (16 * indirect->index) + 12,
+                   flags);
 
     indirect->index++;
 }
@@ -271,11 +332,11 @@ uint32_t qvirtqueue_add(QTestState *qts, QVirtQueue *vq, uint64_t data,
     }
 
     /* vq->desc[vq->free_head].addr */
-    qtest_writeq(qts, vq->desc + (16 * vq->free_head), data);
+    qvirtio_writeq(vq->vdev, qts, vq->desc + (16 * vq->free_head), data);
     /* vq->desc[vq->free_head].len */
-    qtest_writel(qts, vq->desc + (16 * vq->free_head) + 8, len);
+    qvirtio_writel(vq->vdev, qts, vq->desc + (16 * vq->free_head) + 8, len);
     /* vq->desc[vq->free_head].flags */
-    qtest_writew(qts, vq->desc + (16 * vq->free_head) + 12, flags);
+    qvirtio_writew(vq->vdev, qts, vq->desc + (16 * vq->free_head) + 12, flags);
 
     return vq->free_head++; /* Return and increase, in this order */
 }
@@ -290,13 +351,14 @@ uint32_t qvirtqueue_add_indirect(QTestState *qts, QVirtQueue *vq,
     vq->num_free--;
 
     /* vq->desc[vq->free_head].addr */
-    qtest_writeq(qts, vq->desc + (16 * vq->free_head), indirect->desc);
+    qvirtio_writeq(vq->vdev, qts, vq->desc + (16 * vq->free_head),
+                   indirect->desc);
     /* vq->desc[vq->free_head].len */
-    qtest_writel(qts, vq->desc + (16 * vq->free_head) + 8,
-           sizeof(struct vring_desc) * indirect->elem);
+    qvirtio_writel(vq->vdev, qts, vq->desc + (16 * vq->free_head) + 8,
+                   sizeof(struct vring_desc) * indirect->elem);
     /* vq->desc[vq->free_head].flags */
-    qtest_writew(qts, vq->desc + (16 * vq->free_head) + 12,
-                 VRING_DESC_F_INDIRECT);
+    qvirtio_writew(vq->vdev, qts, vq->desc + (16 * vq->free_head) + 12,
+                   VRING_DESC_F_INDIRECT);
 
     return vq->free_head++; /* Return and increase, in this order */
 }
@@ -305,21 +367,21 @@ void qvirtqueue_kick(QTestState *qts, QVirtioDevice *d, QVirtQueue *vq,
                      uint32_t free_head)
 {
     /* vq->avail->idx */
-    uint16_t idx = qtest_readw(qts, vq->avail + 2);
+    uint16_t idx = qvirtio_readw(d, qts, vq->avail + 2);
     /* vq->used->flags */
     uint16_t flags;
     /* vq->used->avail_event */
     uint16_t avail_event;
 
     /* vq->avail->ring[idx % vq->size] */
-    qtest_writew(qts, vq->avail + 4 + (2 * (idx % vq->size)), free_head);
+    qvirtio_writew(d, qts, vq->avail + 4 + (2 * (idx % vq->size)), free_head);
     /* vq->avail->idx */
-    qtest_writew(qts, vq->avail + 2, idx + 1);
+    qvirtio_writew(d, qts, vq->avail + 2, idx + 1);
 
     /* Must read after idx is updated */
-    flags = qtest_readw(qts, vq->avail);
-    avail_event = qtest_readw(qts, vq->used + 4 +
-                                   sizeof(struct vring_used_elem) * vq->size);
+    flags = qvirtio_readw(d, qts, vq->avail);
+    avail_event = qvirtio_readw(d, qts, vq->used + 4 +
+                                sizeof(struct vring_used_elem) * vq->size);
 
     /* < 1 because we add elements to avail queue one by one */
     if ((flags & VRING_USED_F_NO_NOTIFY) == 0 &&
@@ -344,7 +406,8 @@ bool qvirtqueue_get_buf(QTestState *qts, QVirtQueue *vq, uint32_t *desc_idx,
     uint16_t idx;
     uint64_t elem_addr, addr;
 
-    idx = qtest_readw(qts, vq->used + offsetof(struct vring_used, idx));
+    idx = qvirtio_readw(vq->vdev, qts,
+                        vq->used + offsetof(struct vring_used, idx));
     if (idx == vq->last_used_idx) {
         return false;
     }
@@ -356,12 +419,12 @@ bool qvirtqueue_get_buf(QTestState *qts, QVirtQueue *vq, uint32_t *desc_idx,
 
     if (desc_idx) {
         addr = elem_addr + offsetof(struct vring_used_elem, id);
-        *desc_idx = qtest_readl(qts, addr);
+        *desc_idx = qvirtio_readl(vq->vdev, qts, addr);
     }
 
     if (len) {
         addr = elem_addr + offsetof(struct vring_used_elem, len);
-        *len = qtest_readw(qts, addr);
+        *len = qvirtio_readw(vq->vdev, qts, addr);
     }
 
     vq->last_used_idx++;
@@ -373,7 +436,7 @@ void qvirtqueue_set_used_event(QTestState *qts, QVirtQueue *vq, uint16_t idx)
     g_assert(vq->event);
 
     /* vq->avail->used_event */
-    qtest_writew(qts, vq->avail + 4 + (2 * vq->size), idx);
+    qvirtio_writew(vq->vdev, qts, vq->avail + 4 + (2 * vq->size), idx);
 }
 
 void qvirtio_start_device(QVirtioDevice *vdev)
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index fe0dc4a896..2a23698211 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -388,8 +388,8 @@ static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc)
     g_free(req.data);
 
     indirect = qvring_indirect_desc_setup(qts, dev, t_alloc, 2);
-    qvring_indirect_desc_add(qts, indirect, req_addr, 528, false);
-    qvring_indirect_desc_add(qts, indirect, req_addr + 528, 1, true);
+    qvring_indirect_desc_add(dev, qts, indirect, req_addr, 528, false);
+    qvring_indirect_desc_add(dev, qts, indirect, req_addr + 528, 1, true);
     free_head = qvirtqueue_add_indirect(qts, vq, indirect);
     qvirtqueue_kick(qts, dev, vq, free_head);
 
@@ -413,8 +413,8 @@ static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc)
     g_free(req.data);
 
     indirect = qvring_indirect_desc_setup(qts, dev, t_alloc, 2);
-    qvring_indirect_desc_add(qts, indirect, req_addr, 16, false);
-    qvring_indirect_desc_add(qts, indirect, req_addr + 16, 513, true);
+    qvring_indirect_desc_add(dev, qts, indirect, req_addr, 16, false);
+    qvring_indirect_desc_add(dev, qts, indirect, req_addr + 16, 513, true);
     free_head = qvirtqueue_add_indirect(qts, vq, indirect);
     qvirtqueue_kick(qts, dev, vq, free_head);
 
-- 
2.21.0

Re: [PATCH v3 09/16] libqos: access VIRTIO 1.0 vring in little-endian

[Thomas Huth]

On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> VIRTIO 1.0 uses little-endian for the vring.  Legacy VIRTIO uses guest
> endianness.  Adjust the code to handle both.
> 
> Note that qvirtio_readq() is not defined because it has no users.  All
> the other accessors are really needed.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.h      |   4 +-
>  tests/libqos/virtio-mmio.c |   1 +
>  tests/libqos/virtio-pci.c  |   1 +
>  tests/libqos/virtio.c      | 131 +++++++++++++++++++++++++++----------
>  tests/virtio-blk-test.c    |   8 +--
>  5 files changed, 106 insertions(+), 39 deletions(-)
[...]
> @@ -191,23 +248,23 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
>  
>      for (i = 0; i < vq->size - 1; i++) {
>          /* vq->desc[i].addr */
> -        qtest_writeq(qts, vq->desc + (16 * i), 0);
> +        qvirtio_writeq(vq->vdev, qts, vq->desc + (16 * i), 0);
>          /* vq->desc[i].next */
> -        qtest_writew(qts, vq->desc + (16 * i) + 14, i + 1);
> +        qvirtio_writew(vq->vdev, qts, vq->desc + (16 * i) + 14, i + 1);
>      }
>  
>      /* vq->avail->flags */
> -    qtest_writew(qts, vq->avail, 0);
> +    qvirtio_writew(vq->vdev, qts, vq->avail, 0);
>      /* vq->avail->idx */
> -    qtest_writew(qts, vq->avail + 2, 0);
> +    qvirtio_writew(vq->vdev, qts, vq->avail + 2, 0);
>      /* vq->avail->used_event */
> -    qtest_writew(qts, vq->avail + 4 + (2 * vq->size), 0);
> +    qvirtio_writew(vq->vdev, qts, vq->avail + 4 + (2 * vq->size), 0);
>  
>      /* vq->used->flags */
> -    qtest_writew(qts, vq->used, 0);
> +    qvirtio_writew(vq->vdev, qts, vq->used, 0);
>      /* vq->used->avail_event */
> -    qtest_writew(qts, vq->used + 2 + sizeof(struct vring_used_elem) * vq->size,
> -                 0);
> +    qvirtio_writew(vq->vdev, qts, vq->used + 2 +
> +            sizeof(struct vring_used_elem) * vq->size, 0);

Fix indentation in the above line?

Apart from that, patch looks fine to me.

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

Re: [PATCH v3 09/16] libqos: access VIRTIO 1.0 vring in little-endian

[Stefan Hajnoczi]

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

On Mon, Oct 21, 2019 at 02:48:08PM +0200, Thomas Huth wrote:
> On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> > VIRTIO 1.0 uses little-endian for the vring.  Legacy VIRTIO uses guest
> > endianness.  Adjust the code to handle both.
> > 
> > Note that qvirtio_readq() is not defined because it has no users.  All
> > the other accessors are really needed.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/libqos/virtio.h      |   4 +-
> >  tests/libqos/virtio-mmio.c |   1 +
> >  tests/libqos/virtio-pci.c  |   1 +
> >  tests/libqos/virtio.c      | 131 +++++++++++++++++++++++++++----------
> >  tests/virtio-blk-test.c    |   8 +--
> >  5 files changed, 106 insertions(+), 39 deletions(-)
> [...]
> > @@ -191,23 +248,23 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
> >  
> >      for (i = 0; i < vq->size - 1; i++) {
> >          /* vq->desc[i].addr */
> > -        qtest_writeq(qts, vq->desc + (16 * i), 0);
> > +        qvirtio_writeq(vq->vdev, qts, vq->desc + (16 * i), 0);
> >          /* vq->desc[i].next */
> > -        qtest_writew(qts, vq->desc + (16 * i) + 14, i + 1);
> > +        qvirtio_writew(vq->vdev, qts, vq->desc + (16 * i) + 14, i + 1);
> >      }
> >  
> >      /* vq->avail->flags */
> > -    qtest_writew(qts, vq->avail, 0);
> > +    qvirtio_writew(vq->vdev, qts, vq->avail, 0);
> >      /* vq->avail->idx */
> > -    qtest_writew(qts, vq->avail + 2, 0);
> > +    qvirtio_writew(vq->vdev, qts, vq->avail + 2, 0);
> >      /* vq->avail->used_event */
> > -    qtest_writew(qts, vq->avail + 4 + (2 * vq->size), 0);
> > +    qvirtio_writew(vq->vdev, qts, vq->avail + 4 + (2 * vq->size), 0);
> >  
> >      /* vq->used->flags */
> > -    qtest_writew(qts, vq->used, 0);
> > +    qvirtio_writew(vq->vdev, qts, vq->used, 0);
> >      /* vq->used->avail_event */
> > -    qtest_writew(qts, vq->used + 2 + sizeof(struct vring_used_elem) * vq->size,
> > -                 0);
> > +    qvirtio_writew(vq->vdev, qts, vq->used + 2 +
> > +            sizeof(struct vring_used_elem) * vq->size, 0);
> 
> Fix indentation in the above line?

Will fix in v4.

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

Re: [PATCH v3 09/16] libqos: access VIRTIO 1.0 vring in little-endian

[Christophe de Dinechin]

Stefan Hajnoczi writes:
[...]
> +static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, uint64_t addr)
> +{
> +    uint16_t val = qtest_readw(qts, addr);
> +
> +    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {

For my education, I was wondering why tests use the (1ull << FEATURE)
notation and not an equivalent of virtio_has_feature()? Is this
intentional, or just legacy?


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v3 09/16] libqos: access VIRTIO 1.0 vring in little-endian

[Stefan Hajnoczi]

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

On Tue, Oct 22, 2019 at 06:51:39PM +0200, Christophe de Dinechin wrote:
> 
> Stefan Hajnoczi writes:
> [...]
> > +static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, uint64_t addr)
> > +{
> > +    uint16_t val = qtest_readw(qts, addr);
> > +
> > +    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
> 
> For my education, I was wondering why tests use the (1ull << FEATURE)
> notation and not an equivalent of virtio_has_feature()? Is this
> intentional, or just legacy?

It's historical, libqos never defined an equivalent helper function.

Stefan

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

[PATCH v3 10/16] 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>
--
v3:
 * Document qpci_find_capability()
---
 tests/libqos/pci.h |  2 +-
 tests/libqos/pci.c | 30 ++++++++++++++++++++++++------
 2 files changed, 25 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..2309a724e4 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -115,10 +115,28 @@ 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)
+/**
+ * qpci_find_capability:
+ * @dev: the PCI device
+ * @id: the PCI Capability ID (PCI_CAP_ID_*)
+ * @start_addr: 0 to begin iteration or the last return value to continue
+ *              iteration
+ *
+ * Iterate over the PCI Capabilities List.
+ *
+ * Returns: PCI Configuration Space offset of the capabililty structure or
+ *          0 if no further matching capability is found
+ */
+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 +156,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 +185,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 +221,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 +239,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

Re: [PATCH v3 10/16] libqos: add iteration support to qpci_find_capability()

[Thomas Huth]

On 19/10/2019 08.38, 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>
> --
> v3:
>  * Document qpci_find_capability()
> ---
>  tests/libqos/pci.h |  2 +-
>  tests/libqos/pci.c | 30 ++++++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 7 deletions(-)

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

[PATCH v3 11/16] 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>
Reviewed-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Thomas Huth <thuth@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 5777683a2a..407d8fc088 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -80,7 +80,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 4db1f1b8bc..e0a2bd7bc6 100644
--- a/tests/libqos/virtio-mmio.c
+++ b/tests/libqos/virtio-mmio.c
@@ -143,9 +143,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);
 }
 
@@ -179,7 +181,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 7ecf5d0a52..e4fa318dcc 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -199,9 +199,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);
 }
 
@@ -239,7 +241,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

Re: [PATCH v3 11/16] libqos: pass full QVirtQueue to set_queue_address()

[Philippe Mathieu-Daudé]

On 10/19/19 8:38 AM, 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>
> Reviewed-by: Sergio Lopez <slp@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@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 5777683a2a..407d8fc088 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -80,7 +80,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 4db1f1b8bc..e0a2bd7bc6 100644
> --- a/tests/libqos/virtio-mmio.c
> +++ b/tests/libqos/virtio-mmio.c
> @@ -143,9 +143,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);
>   }
>   
> @@ -179,7 +181,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 7ecf5d0a52..e4fa318dcc 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -199,9 +199,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);
>   }
>   
> @@ -239,7 +241,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;
>   }
> 

[PATCH v3 12/16] 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>
Reviewed-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Thomas Huth <thuth@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 728b4715f1..4299efc023 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 e4fa318dcc..0725777a8d 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -281,6 +281,31 @@ const QVirtioBus qvirtio_pci = {
     .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);
@@ -295,7 +320,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;
 
@@ -321,16 +345,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;
 
@@ -358,9 +378,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)
@@ -383,6 +401,7 @@ static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_
     dev->vdev.device_type = qpci_config_readw(pci_dev, PCI_SUBSYSTEM_ID);
 
     dev->config_msix_entry = -1;
+    dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
 
     dev->vdev.bus = &qvirtio_pci;
     dev->vdev.big_endian = qvirtio_pci_is_big_endian(dev);
-- 
2.21.0

[PATCH v3 13/16] 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>
Reviewed-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Thomas Huth <thuth@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 4299efc023..0e4a8b7b00 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -65,4 +65,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 0725777a8d..c900742f96 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -207,8 +207,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)
 {
     uint64_t feat;
     uint64_t addr;
@@ -216,12 +217,12 @@ 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.vdev = d;
     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;
@@ -241,12 +242,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);
@@ -276,8 +277,8 @@ const QVirtioBus qvirtio_pci = {
     .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 v3 14/16] 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>
---
v3:
 * Change uint8_t bar_idx to int [Thomas]
---
 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 0e4a8b7b00..78a1c15c2a 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;
+
+    int bar_idx;
 } QVirtioPCIDevice;
 
 struct QVirtioPCIMSIXOps {
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index c900742f96..e9595603f5 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -310,7 +310,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)
@@ -400,6 +400,7 @@ static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_
 {
     dev->pdev = pci_dev;
     dev->vdev.device_type = qpci_config_readw(pci_dev, PCI_SUBSYSTEM_ID);
+    dev->bar_idx = 0;
 
     dev->config_msix_entry = -1;
     dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
-- 
2.21.0

Re: [PATCH v3 14/16] libqos: make the virtio-pci BAR index configurable

[Thomas Huth]

On 19/10/2019 08.38, 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>
> ---
> v3:
>  * Change uint8_t bar_idx to int [Thomas]
> ---
>  tests/libqos/virtio-pci.h | 2 ++
>  tests/libqos/virtio-pci.c | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)

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

Re: [PATCH v3 14/16] libqos: make the virtio-pci BAR index configurable

[Philippe Mathieu-Daudé]

On 10/19/19 8:38 AM, 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>
> ---
> v3:
>   * Change uint8_t bar_idx to int [Thomas]
> ---
>   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 0e4a8b7b00..78a1c15c2a 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;
> +
> +    int bar_idx;
>   } QVirtioPCIDevice;
>   
>   struct QVirtioPCIMSIXOps {
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index c900742f96..e9595603f5 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -310,7 +310,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)
> @@ -400,6 +400,7 @@ static void qvirtio_pci_init_from_pcidev(QVirtioPCIDevice *dev, QPCIDevice *pci_
>   {
>       dev->pdev = pci_dev;
>       dev->vdev.device_type = qpci_config_readw(pci_dev, PCI_SUBSYSTEM_ID);
> +    dev->bar_idx = 0;
>   
>       dev->config_msix_entry = -1;
>       dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 15/16] 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>
Reviewed-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/libqos/virtio-pci.h |  2 --
 tests/libqos/virtio-pci.c | 29 ++++++++++++-----------------
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index 78a1c15c2a..6b3a385b06 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -45,8 +45,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 e9595603f5..11866f7772 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)
@@ -262,7 +253,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,
@@ -396,17 +387,21 @@ 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->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);
+}
+
 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->bar_idx = 0;
-
     dev->config_msix_entry = -1;
-    dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
 
-    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

Re: [PATCH v3 15/16] libqos: extract Legacy virtio-pci.c code

[Philippe Mathieu-Daudé]

On 10/19/19 8:38 AM, 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>
> Reviewed-by: Sergio Lopez <slp@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/libqos/virtio-pci.h |  2 --
>   tests/libqos/virtio-pci.c | 29 ++++++++++++-----------------
>   2 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
> index 78a1c15c2a..6b3a385b06 100644
> --- a/tests/libqos/virtio-pci.h
> +++ b/tests/libqos/virtio-pci.h
> @@ -45,8 +45,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 e9595603f5..11866f7772 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)
> @@ -262,7 +253,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,
> @@ -396,17 +387,21 @@ 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->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);
> +}
> +
>   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->bar_idx = 0;
> -
>       dev->config_msix_entry = -1;
> -    dev->msix_ops = &qvirtio_pci_msix_ops_legacy;
>   
> -    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: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 16/16] 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.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Sergio Lopez <slp@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 | 443 +++++++++++++++++++++++++++++++
 tests/libqos/virtio-pci.c        |   6 +-
 5 files changed, 476 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 6b3a385b06..294d5567ee 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;
 
     int 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..18d118866f
--- /dev/null
+++ b/tests/libqos/virtio-pci-modern.c
@@ -0,0 +1,443 @@
+/*
+ * 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 "standard-headers/linux/virtio_config.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 uint64_t get_features(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    uint64_t lo, hi;
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            device_feature_select),
+                   0);
+    lo = qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
+                       offsetof(struct virtio_pci_common_cfg, device_feature));
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            device_feature_select),
+                   1);
+    hi = qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
+                       offsetof(struct virtio_pci_common_cfg, device_feature));
+
+    return (hi << 32) | lo;
+}
+
+static void set_features(QVirtioDevice *d, uint64_t features)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+
+    /* Drivers must enable VIRTIO 1.0 or else use the Legacy interface */
+    g_assert_cmphex(features & (1ull << VIRTIO_F_VERSION_1), !=, 0);
+
+    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);
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature_select),
+                   1);
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature),
+                   features >> 32);
+}
+
+static uint64_t get_guest_features(QVirtioDevice *d)
+{
+    QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
+    uint64_t lo, hi;
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature_select),
+                   0);
+    lo = qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
+                       offsetof(struct virtio_pci_common_cfg, guest_feature));
+
+    qpci_io_writel(dev->pdev, dev->bar, dev->common_cfg_offset +
+                   offsetof(struct virtio_pci_common_cfg,
+                            guest_feature_select),
+                   1);
+    hi = qpci_io_readl(dev->pdev, dev->bar, dev->common_cfg_offset +
+                       offsetof(struct virtio_pci_common_cfg, guest_feature));
+
+    return (hi << 32) | lo;
+}
+
+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, &bar,
+                        &dev->common_cfg_offset, &length, NULL)) {
+        return false;
+    }
+    dev->bar_idx = bar;
+
+    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 11866f7772..62851c29bb 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
@@ -401,7 +403,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