[PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Alex Bennée]

Hi,

This is an update to the previous series which fixes the last few
niggling CI failures I was seeing.

   Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
   Date: Tue, 26 Jul 2022 20:21:29 +0100
   Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>

The CI failures were tricky to track down because they didn't occur
locally but after patching to dump backtraces they all seem to involve
updates to virtio_set_status() as the machine was torn down. I think
patch that switches all users to use virtio_device_started() along
with consistent checking of vhost_dev->started stops this from
happening. The clean-up seems worthwhile in reducing boilerplate
anyway.

The following patches still need review:

  - tests/qtest: enable tests for virtio-gpio
  - tests/qtest: add a get_features op to vhost-user-test
  - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
  - tests/qtest: add assert to catch bad features
  - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
  - tests/qtest: catch unhandled vhost-user messages
  - tests/qtest: use qos_printf instead of g_test_message
  - tests/qtest: pass stdout/stderr down to subtests
  - hw/virtio: move vhd->started check into helper and add FIXME
  - hw/virtio: move vm_running check to virtio_device_started
  - hw/virtio: add some vhost-user trace events
  - hw/virtio: log potentially buggy guest drivers
  - hw/virtio: fix some coding style issues
  - include/hw: document vhost_dev feature life-cycle
  - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
  - hw/virtio: fix vhost_user_read tracepoint
  - hw/virtio: handle un-configured shutdown in virtio-pci
  - hw/virtio: gracefully handle unset vhost_dev vdev
  - hw/virtio: incorporate backend features in features


Alex Bennée (20):
  hw/virtio: incorporate backend features in features
  hw/virtio: gracefully handle unset vhost_dev vdev
  hw/virtio: handle un-configured shutdown in virtio-pci
  hw/virtio: fix vhost_user_read tracepoint
  include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
  include/hw: document vhost_dev feature life-cycle
  hw/virtio: fix some coding style issues
  hw/virtio: log potentially buggy guest drivers
  hw/virtio: add some vhost-user trace events
  hw/virtio: move vm_running check to virtio_device_started
  hw/virtio: move vhd->started check into helper and add FIXME
  tests/qtest: pass stdout/stderr down to subtests
  tests/qtest: add a timeout for subprocess_run_one_test
  tests/qtest: use qos_printf instead of g_test_message
  tests/qtest: catch unhandled vhost-user messages
  tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
  tests/qtest: add assert to catch bad features
  tests/qtest: implement stub for VHOST_USER_GET_CONFIG
  tests/qtest: add a get_features op to vhost-user-test
  tests/qtest: enable tests for virtio-gpio

Viresh Kumar (2):
  hw/virtio: add boilerplate for vhost-user-gpio device
  hw/virtio: add vhost-user-gpio-pci boilerplate

 include/hw/virtio/vhost-user-gpio.h |  35 +++
 include/hw/virtio/vhost.h           |  15 +
 include/hw/virtio/virtio.h          |  12 +-
 tests/qtest/libqos/virtio-gpio.h    |  35 +++
 hw/block/vhost-user-blk.c           |  10 +-
 hw/scsi/vhost-scsi.c                |   4 +-
 hw/scsi/vhost-user-scsi.c           |   2 +-
 hw/virtio/vhost-user-fs.c           |   9 +-
 hw/virtio/vhost-user-gpio-pci.c     |  69 +++++
 hw/virtio/vhost-user-gpio.c         | 411 ++++++++++++++++++++++++++++
 hw/virtio/vhost-user-i2c.c          |  10 +-
 hw/virtio/vhost-user-rng.c          |  10 +-
 hw/virtio/vhost-user-vsock.c        |   8 +-
 hw/virtio/vhost-user.c              |  20 +-
 hw/virtio/vhost-vsock-common.c      |   3 +-
 hw/virtio/vhost-vsock.c             |   8 +-
 hw/virtio/vhost.c                   |  16 +-
 hw/virtio/virtio-pci.c              |   9 +-
 hw/virtio/virtio.c                  |   7 +
 tests/qtest/libqos/virtio-gpio.c    | 171 ++++++++++++
 tests/qtest/libqos/virtio.c         |   4 +-
 tests/qtest/qos-test.c              |   9 +-
 tests/qtest/vhost-user-test.c       | 175 ++++++++++--
 MAINTAINERS                         |   8 +
 hw/virtio/Kconfig                   |   5 +
 hw/virtio/meson.build               |   2 +
 hw/virtio/trace-events              |   9 +
 tests/qtest/libqos/meson.build      |   1 +
 28 files changed, 1007 insertions(+), 70 deletions(-)
 create mode 100644 include/hw/virtio/vhost-user-gpio.h
 create mode 100644 tests/qtest/libqos/virtio-gpio.h
 create mode 100644 hw/virtio/vhost-user-gpio-pci.c
 create mode 100644 hw/virtio/vhost-user-gpio.c
 create mode 100644 tests/qtest/libqos/virtio-gpio.c

-- 
2.30.2

[PATCH v4 01/22] hw/virtio: incorporate backend features in features

[Alex Bennée]

There are some extra bits used over a vhost-user connection which are
hidden from the device itself. We need to set them here to ensure we
enable things like the protocol extensions.

Currently net/vhost-user.c has it's own inscrutable way of persisting
this data but it really should live in the core vhost_user code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-7-alex.bennee@linaro.org>
---
 hw/virtio/vhost-user.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 75b8df21a4..1936a44e82 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1460,7 +1460,14 @@ static int vhost_user_set_features(struct vhost_dev *dev,
      */
     bool log_enabled = features & (0x1ULL << VHOST_F_LOG_ALL);
 
-    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+    /*
+     * We need to include any extra backend only feature bits that
+     * might be needed by our device. Currently this includes the
+     * VHOST_USER_F_PROTOCOL_FEATURES bit for enabling protocol
+     * features.
+     */
+    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
+                              features | dev->backend_features,
                               log_enabled);
 }
 
-- 
2.30.2

[PATCH v4 02/22] hw/virtio: gracefully handle unset vhost_dev vdev

[Alex Bennée]

I've noticed asserts firing because we query the status of vdev after
a vhost connection is closed down. Rather than faulting on the NULL
indirect just quietly reply false.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-8-alex.bennee@linaro.org>
---
 hw/virtio/vhost.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0827d631c0..f758f177bb 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -306,7 +306,7 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
     dev->log_size = size;
 }
 
-static int vhost_dev_has_iommu(struct vhost_dev *dev)
+static bool vhost_dev_has_iommu(struct vhost_dev *dev)
 {
     VirtIODevice *vdev = dev->vdev;
 
@@ -316,8 +316,12 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
      * does not have IOMMU, there's no need to enable this feature
      * which may cause unnecessary IOTLB miss/update transactions.
      */
-    return virtio_bus_device_iommu_enabled(vdev) &&
-           virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+    if (vdev) {
+        return virtio_bus_device_iommu_enabled(vdev) &&
+            virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+    } else {
+        return false;
+    }
 }
 
 static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
-- 
2.30.2

[PATCH v4 03/22] hw/virtio: handle un-configured shutdown in virtio-pci

[Alex Bennée]

The assert() protecting against leakage is a little aggressive and
causes needless crashes if a device is shutdown without having been
configured. In this case no descriptors are lost because none have
been assigned.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-9-alex.bennee@linaro.org>
---
 hw/virtio/virtio-pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 45327f0b31..5ce61f9b45 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -996,9 +996,14 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
 
     nvqs = MIN(nvqs, VIRTIO_QUEUE_MAX);
 
-    /* When deassigning, pass a consistent nvqs value
-     * to avoid leaking notifiers.
+    /*
+     * When deassigning, pass a consistent nvqs value to avoid leaking
+     * notifiers. But first check we've actually been configured, exit
+     * early if we haven't.
      */
+    if (!assign && !proxy->nvqs_with_notifiers) {
+        return 0;
+    }
     assert(assign || nvqs == proxy->nvqs_with_notifiers);
 
     proxy->nvqs_with_notifiers = nvqs;
-- 
2.30.2

[PATCH v4 04/22] hw/virtio: fix vhost_user_read tracepoint

[Alex Bennée]

As reads happen in the callback we were never seeing them. We only
really care about the header so move the tracepoint to when the header
is complete.

Fixes: 6ca6d8ee9d (hw/virtio: add vhost_user_[read|write] trace points)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20220726192150.2435175-10-alex.bennee@linaro.org>
---
 hw/virtio/vhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1936a44e82..c0b50deaf2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -295,6 +295,8 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
         return -EPROTO;
     }
 
+    trace_vhost_user_read(msg->hdr.request, msg->hdr.flags);
+
     return 0;
 }
 
@@ -544,8 +546,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
         }
     }
 
-    trace_vhost_user_read(msg.hdr.request, msg.hdr.flags);
-
     return 0;
 }
 
-- 
2.30.2

[PATCH v4 05/22] include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE

[Alex Bennée]

When debugging a new vhost user you may be surprised to see
VHOST_USER_F_PROTOCOL getting squashed in the maze of
backend_features, acked_features and guest_features. Expand the
description here to help the next poor soul trying to work through
this.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - s/vhost/vhost-user/
---
 include/hw/virtio/virtio.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..9bb2485415 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -24,7 +24,12 @@
 #include "qom/object.h"
 #include "hw/virtio/vhost.h"
 
-/* A guest should never accept this.  It implies negotiation is broken. */
+/*
+ * A guest should never accept this. It implies negotiation is broken
+ * between the driver frontend and the device. This bit is re-used for
+ * vhost-user to advertise VHOST_USER_F_PROTOCOL_FEATURES between QEMU
+ * and a vhost-user backend.
+ */
 #define VIRTIO_F_BAD_FEATURE		30
 
 #define VIRTIO_LEGACY_FEATURES ((0x1ULL << VIRTIO_F_BAD_FEATURE) | \
-- 
2.30.2

[PATCH v4 06/22] include/hw: document vhost_dev feature life-cycle

[Alex Bennée]

Try and explicitly document the various state of feature bits as
related to the vhost_dev structure. Importantly the backend_features
can advertise things like VHOST_USER_F_PROTOCOL_FEATURES which is
never exposed to the driver and is only present in the vhost-user
feature negotiation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 include/hw/virtio/vhost.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a346f23d13..586c5457e2 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -86,8 +86,11 @@ struct vhost_dev {
     /* if non-zero, minimum required value for max_queues */
     int num_queues;
     uint64_t features;
+    /** @acked_features: final set of negotiated features */
     uint64_t acked_features;
+    /** @backend_features: backend specific feature bits */
     uint64_t backend_features;
+    /** @protocol_features: final negotiated protocol features */
     uint64_t protocol_features;
     uint64_t max_queues;
     uint64_t backend_cap;
-- 
2.30.2

[PATCH v4 07/22] hw/virtio: fix some coding style issues

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-user.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index c0b50deaf2..b7c13e7e16 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -200,7 +200,7 @@ typedef struct {
     VhostUserRequest request;
 
 #define VHOST_USER_VERSION_MASK     (0x3)
-#define VHOST_USER_REPLY_MASK       (0x1<<2)
+#define VHOST_USER_REPLY_MASK       (0x1 << 2)
 #define VHOST_USER_NEED_REPLY_MASK  (0x1 << 3)
     uint32_t flags;
     uint32_t size; /* the following payload size */
@@ -208,7 +208,7 @@ typedef struct {
 
 typedef union {
 #define VHOST_USER_VRING_IDX_MASK   (0xff)
-#define VHOST_USER_VRING_NOFD_MASK  (0x1<<8)
+#define VHOST_USER_VRING_NOFD_MASK  (0x1 << 8)
         uint64_t u64;
         struct vhost_vring_state state;
         struct vhost_vring_addr addr;
@@ -248,7 +248,8 @@ struct vhost_user {
     size_t             region_rb_len;
     /* RAMBlock associated with a given region */
     RAMBlock         **region_rb;
-    /* The offset from the start of the RAMBlock to the start of the
+    /*
+     * The offset from the start of the RAMBlock to the start of the
      * vhost region.
      */
     ram_addr_t        *region_rb_offset;
-- 
2.30.2

[PATCH v4 08/22] hw/virtio: log potentially buggy guest drivers

[Alex Bennée]

If the guest driver attempts to use the UNUSED(30) bit it is
potentially buggy as 6.3 Legacy Interface: Reserved Feature Bits
states it "SHOULD NOT be negotiated". For now just log this guest
error.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5d607aeaa0..97a6307c0f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2980,6 +2980,13 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
     if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) {
         return -EINVAL;
     }
+
+    if (val & (1ull << VIRTIO_F_BAD_FEATURE)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: guest driver for %s has enabled UNUSED(30) feature bit!\n",
+                      __func__, vdev->name);
+    }
+
     ret = virtio_set_features_nocheck(vdev, val);
     if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
-- 
2.30.2

[PATCH v4 09/22] hw/virtio: add some vhost-user trace events

[Alex Bennée]

These are useful for tracing the lifetime of vhost-user connections.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/virtio/vhost.c      | 6 ++++++
 hw/virtio/trace-events | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index f758f177bb..5185c15295 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1477,6 +1477,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
 {
     int i;
 
+    trace_vhost_dev_cleanup(hdev);
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev->vqs + i);
     }
@@ -1783,6 +1785,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
+    trace_vhost_dev_start(hdev, vdev->name);
+
     vdev->vhost_started = true;
     hdev->started = true;
     hdev->vdev = vdev;
@@ -1869,6 +1873,8 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
+    trace_vhost_dev_stop(hdev, vdev->name);
+
     if (hdev->vhost_ops->vhost_dev_start) {
         hdev->vhost_ops->vhost_dev_start(hdev, false);
     }
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 20af2e7ebd..887ca7afa8 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -8,6 +8,10 @@ vhost_region_add_section_aligned(const char *name, uint64_t gpa, uint64_t size,
 vhost_section(const char *name) "%s"
 vhost_reject_section(const char *name, int d) "%s:%d"
 vhost_iotlb_miss(void *dev, int step) "%p step %d"
+vhost_dev_cleanup(void *dev) "%p"
+vhost_dev_start(void *dev, const char *name) "%p:%s"
+vhost_dev_stop(void *dev, const char *name) "%p:%s"
+
 
 # vhost-user.c
 vhost_user_postcopy_end_entry(void) ""
-- 
2.30.2

[PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Alex Bennée]

All the boilerplate virtio code does the same thing (or should at
least) of checking to see if the VM is running before attempting to
start VirtIO. Push the logic up to the common function to avoid
getting a copy and paste wrong.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/virtio.h   | 5 +++++
 hw/virtio/vhost-user-fs.c    | 6 +-----
 hw/virtio/vhost-user-i2c.c   | 6 +-----
 hw/virtio/vhost-user-rng.c   | 6 +-----
 hw/virtio/vhost-user-vsock.c | 6 +-----
 hw/virtio/vhost-vsock.c      | 6 +-----
 6 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9bb2485415..74e7ad5a92 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -100,6 +100,7 @@ struct VirtIODevice
     VirtQueue *vq;
     MemoryListener listener;
     uint16_t device_id;
+    /* @vm_running: current VM running state via virtio_vmstate_change() */
     bool vm_running;
     bool broken; /* device in invalid state, needs reset */
     bool use_disabled_flag; /* allow use of 'disable' flag when needed */
@@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
         return vdev->started;
     }
 
+    if (!vdev->vm_running) {
+        return false;
+    }
+
     return status & VIRTIO_CONFIG_S_DRIVER_OK;
 }
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index e513e4fdda..d2bebba785 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
 static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserFS *fs = VHOST_USER_FS(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (fs->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 6020eee093..b930cf6d5e 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
 static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (i2c->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index 3a7bf8e32d..a9c1c4bc79 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
 static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (rng->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 0f8ff99f85..22c1616ebd 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
 static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (vvc->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 0338de892f..8031c164a5 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
 static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+    bool should_start = virtio_device_started(vdev, status);
     int ret;
 
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
-
     if (vvc->vhost_dev.started == should_start) {
         return;
     }
-- 
2.30.2

[Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Alex Bennée]

All the boilerplate virtio code does the same thing (or should at
least) of checking to see if the VM is running before attempting to
start VirtIO. Push the logic up to the common function to avoid
getting a copy and paste wrong.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/virtio.h   | 5 +++++
 hw/virtio/vhost-user-fs.c    | 6 +-----
 hw/virtio/vhost-user-i2c.c   | 6 +-----
 hw/virtio/vhost-user-rng.c   | 6 +-----
 hw/virtio/vhost-user-vsock.c | 6 +-----
 hw/virtio/vhost-vsock.c      | 6 +-----
 6 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9bb2485415..74e7ad5a92 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -100,6 +100,7 @@ struct VirtIODevice
     VirtQueue *vq;
     MemoryListener listener;
     uint16_t device_id;
+    /* @vm_running: current VM running state via virtio_vmstate_change() */
     bool vm_running;
     bool broken; /* device in invalid state, needs reset */
     bool use_disabled_flag; /* allow use of 'disable' flag when needed */
@@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
         return vdev->started;
     }
 
+    if (!vdev->vm_running) {
+        return false;
+    }
+
     return status & VIRTIO_CONFIG_S_DRIVER_OK;
 }
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index e513e4fdda..d2bebba785 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
 static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserFS *fs = VHOST_USER_FS(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (fs->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 6020eee093..b930cf6d5e 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
 static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (i2c->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index 3a7bf8e32d..a9c1c4bc79 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
 static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (rng->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 0f8ff99f85..22c1616ebd 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
 static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
-
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
+    bool should_start = virtio_device_started(vdev, status);
 
     if (vvc->vhost_dev.started == should_start) {
         return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 0338de892f..8031c164a5 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
 static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+    bool should_start = virtio_device_started(vdev, status);
     int ret;
 
-    if (!vdev->vm_running) {
-        should_start = false;
-    }
-
     if (vvc->vhost_dev.started == should_start) {
         return;
     }
-- 
2.30.2

[PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME

[Alex Bennée]

The `started` field is manipulated internally within the vhost code
except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck
dev state in the vhost_migration_log routine). Mark that as a FIXME
because it introduces a potential race. I think the referenced fix
should be tracking its state locally.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost.h      | 12 ++++++++++++
 hw/block/vhost-user-blk.c      | 10 ++++++++--
 hw/scsi/vhost-scsi.c           |  4 ++--
 hw/scsi/vhost-user-scsi.c      |  2 +-
 hw/virtio/vhost-user-fs.c      |  3 ++-
 hw/virtio/vhost-user-i2c.c     |  4 ++--
 hw/virtio/vhost-user-rng.c     |  4 ++--
 hw/virtio/vhost-user-vsock.c   |  2 +-
 hw/virtio/vhost-vsock-common.c |  3 ++-
 hw/virtio/vhost-vsock.c        |  2 +-
 10 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 586c5457e2..61b957e927 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -94,6 +94,7 @@ struct vhost_dev {
     uint64_t protocol_features;
     uint64_t max_queues;
     uint64_t backend_cap;
+    /* @started: is the vhost device started? */
     bool started;
     bool log_enabled;
     uint64_t log_size;
@@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
  */
 void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
 
+/**
+ * vhost_dev_is_started() - report status of vhost device
+ * @hdev: common vhost_dev structure
+ *
+ * Return the started status of the vhost device
+ */
+static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
+{
+    return hdev->started;
+}
+
 /**
  * vhost_dev_start() - start the vhost device
  * @hdev: common vhost_dev structure
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..2bba42478d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
         return;
     }
 
-    if (s->dev.started == should_start) {
+    if (vhost_dev_is_started(&s->dev) == should_start) {
         return;
     }
 
@@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         return;
     }
 
-    if (s->dev.started) {
+    if (vhost_dev_is_started(&s->dev)) {
         return;
     }
 
@@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
              * the vhost migration code. If disconnect was caught there is an
              * option for the general vhost code to get the dev state without
              * knowing its type (in this case vhost-user).
+             *
+             * FIXME: this is sketchy to be reaching into vhost_dev
+             * now because we are forcing something that implies we
+             * have executed vhost_dev_stop() but that won't happen
+             * until vhost_user_blk_stop() gets called from the bh.
+             * Really this state check should be tracked locally.
              */
             s->dev.started = false;
         }
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 3059068175..bdf337a7a2 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
         start = false;
     }
 
-    if (vsc->dev.started == start) {
+    if (vhost_dev_is_started(&vsc->dev) == start) {
         return;
     }
 
@@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque)
 
     /* At this point, backend must be stopped, otherwise
      * it might keep writing to memory. */
-    assert(!vsc->dev.started);
+    assert(!vhost_dev_is_started(&vsc->dev));
 
     return 0;
 }
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 1b2f7eed98..bc37317d55 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
     VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
     bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
 
-    if (vsc->dev.started == start) {
+    if (vhost_dev_is_started(&vsc->dev) == start) {
         return;
     }
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index d2bebba785..ad0f91c607 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -20,6 +20,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 #include "qemu/error-report.h"
+#include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user-fs.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
@@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
     VHostUserFS *fs = VHOST_USER_FS(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (fs->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&fs->vhost_dev) == should_start) {
         return;
     }
 
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index b930cf6d5e..bc58b6c0d1 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
     VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (i2c->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
         return;
     }
 
@@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev)
     }
     i2c->connected = false;
 
-    if (i2c->vhost_dev.started) {
+    if (vhost_dev_is_started(&i2c->vhost_dev)) {
         vu_i2c_stop(vdev);
     }
 }
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index a9c1c4bc79..bc1f36c5ac 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
     VHostUserRNG *rng = VHOST_USER_RNG(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (rng->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
         return;
     }
 
@@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev)
 
     rng->connected = false;
 
-    if (rng->vhost_dev.started) {
+    if (vhost_dev_is_started(&rng->vhost_dev)) {
         vu_rng_stop(vdev);
     }
 }
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 22c1616ebd..7b67e29d83 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (vvc->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
         return;
     }
 
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index 7394818e00..29b9ab4f72 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -14,6 +14,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "qemu/error-report.h"
 #include "hw/qdev-properties.h"
+#include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-vsock.h"
 #include "qemu/iov.h"
 #include "monitor/monitor.h"
@@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque)
      * At this point, backend must be stopped, otherwise
      * it might keep writing to memory.
      */
-    assert(!vvc->vhost_dev.started);
+    assert(!vhost_dev_is_started(&vvc->vhost_dev));
 
     return 0;
 }
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 8031c164a5..7dc3c73931 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
     bool should_start = virtio_device_started(vdev, status);
     int ret;
 
-    if (vvc->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
         return;
     }
 
-- 
2.30.2

[Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME

[Alex Bennée]

The `started` field is manipulated internally within the vhost code
except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck
dev state in the vhost_migration_log routine). Mark that as a FIXME
because it introduces a potential race. I think the referenced fix
should be tracking its state locally.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost.h      | 12 ++++++++++++
 hw/block/vhost-user-blk.c      | 10 ++++++++--
 hw/scsi/vhost-scsi.c           |  4 ++--
 hw/scsi/vhost-user-scsi.c      |  2 +-
 hw/virtio/vhost-user-fs.c      |  3 ++-
 hw/virtio/vhost-user-i2c.c     |  4 ++--
 hw/virtio/vhost-user-rng.c     |  4 ++--
 hw/virtio/vhost-user-vsock.c   |  2 +-
 hw/virtio/vhost-vsock-common.c |  3 ++-
 hw/virtio/vhost-vsock.c        |  2 +-
 10 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 586c5457e2..61b957e927 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -94,6 +94,7 @@ struct vhost_dev {
     uint64_t protocol_features;
     uint64_t max_queues;
     uint64_t backend_cap;
+    /* @started: is the vhost device started? */
     bool started;
     bool log_enabled;
     uint64_t log_size;
@@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
  */
 void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
 
+/**
+ * vhost_dev_is_started() - report status of vhost device
+ * @hdev: common vhost_dev structure
+ *
+ * Return the started status of the vhost device
+ */
+static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
+{
+    return hdev->started;
+}
+
 /**
  * vhost_dev_start() - start the vhost device
  * @hdev: common vhost_dev structure
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..2bba42478d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
         return;
     }
 
-    if (s->dev.started == should_start) {
+    if (vhost_dev_is_started(&s->dev) == should_start) {
         return;
     }
 
@@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         return;
     }
 
-    if (s->dev.started) {
+    if (vhost_dev_is_started(&s->dev)) {
         return;
     }
 
@@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
              * the vhost migration code. If disconnect was caught there is an
              * option for the general vhost code to get the dev state without
              * knowing its type (in this case vhost-user).
+             *
+             * FIXME: this is sketchy to be reaching into vhost_dev
+             * now because we are forcing something that implies we
+             * have executed vhost_dev_stop() but that won't happen
+             * until vhost_user_blk_stop() gets called from the bh.
+             * Really this state check should be tracked locally.
              */
             s->dev.started = false;
         }
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 3059068175..bdf337a7a2 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
         start = false;
     }
 
-    if (vsc->dev.started == start) {
+    if (vhost_dev_is_started(&vsc->dev) == start) {
         return;
     }
 
@@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque)
 
     /* At this point, backend must be stopped, otherwise
      * it might keep writing to memory. */
-    assert(!vsc->dev.started);
+    assert(!vhost_dev_is_started(&vsc->dev));
 
     return 0;
 }
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 1b2f7eed98..bc37317d55 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
     VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
     bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
 
-    if (vsc->dev.started == start) {
+    if (vhost_dev_is_started(&vsc->dev) == start) {
         return;
     }
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index d2bebba785..ad0f91c607 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -20,6 +20,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 #include "qemu/error-report.h"
+#include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user-fs.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
@@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
     VHostUserFS *fs = VHOST_USER_FS(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (fs->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&fs->vhost_dev) == should_start) {
         return;
     }
 
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index b930cf6d5e..bc58b6c0d1 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
     VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (i2c->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
         return;
     }
 
@@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev)
     }
     i2c->connected = false;
 
-    if (i2c->vhost_dev.started) {
+    if (vhost_dev_is_started(&i2c->vhost_dev)) {
         vu_i2c_stop(vdev);
     }
 }
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index a9c1c4bc79..bc1f36c5ac 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
     VHostUserRNG *rng = VHOST_USER_RNG(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (rng->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
         return;
     }
 
@@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev)
 
     rng->connected = false;
 
-    if (rng->vhost_dev.started) {
+    if (vhost_dev_is_started(&rng->vhost_dev)) {
         vu_rng_stop(vdev);
     }
 }
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 22c1616ebd..7b67e29d83 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
     bool should_start = virtio_device_started(vdev, status);
 
-    if (vvc->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
         return;
     }
 
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index 7394818e00..29b9ab4f72 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -14,6 +14,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "qemu/error-report.h"
 #include "hw/qdev-properties.h"
+#include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-vsock.h"
 #include "qemu/iov.h"
 #include "monitor/monitor.h"
@@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque)
      * At this point, backend must be stopped, otherwise
      * it might keep writing to memory.
      */
-    assert(!vvc->vhost_dev.started);
+    assert(!vhost_dev_is_started(&vvc->vhost_dev));
 
     return 0;
 }
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 8031c164a5..7dc3c73931 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
     bool should_start = virtio_device_started(vdev, status);
     int ret;
 
-    if (vvc->vhost_dev.started == should_start) {
+    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
         return;
     }
 
-- 
2.30.2

[PATCH v4 12/22] hw/virtio: add boilerplate for vhost-user-gpio device

[Alex Bennée]

From: Viresh Kumar <viresh.kumar@linaro.org>

This creates the QEMU side of the vhost-user-gpio device which connects
to the remote daemon. It is based of vhost-user-i2c code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <5390324a748194a21bc99b1538e19761a8c64092.1641987128.git.viresh.kumar@linaro.org>
[AJB: fixes for qtest, tweaks to feature bits]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>

---
v2
  - set VIRTIO_F_VERSION_1
  - set VHOST_USER_F_PROTOCOL_FEATURES
  - terminate feature_bits with VHOST_INVALID_FEATURE_BIT
  - ensure vdev->backend_features set
  - ensure vhost_dev.acked_features set
v3
  - break out vhost_dev structure for code flow reasons
  - use the vhost-user-blk connection lifecycle code
  - follow ->parent_obj style for VHostUserGPIO object
  - add all feature bits supported by the rust-vmm backend
  - clean-up errp propagation to avoid local_err and use ERRP_GAURD
  - s/vhost_dev->features/vdev->guest_features/ when calling vhost_ack_features
  - drop VHOST_USER_F_PROTOCOL_FEATURES definition (pushed to vhost-user)
  - explicitly call vhost_set_vring_enable due to properly negotiated VHOST_USER_F_PROTOCOL_FEATURES
  - use virtio_device_started() check instead of open code.
  - update MAINTAINERS
---
 include/hw/virtio/vhost-user-gpio.h |  35 +++
 hw/virtio/vhost-user-gpio.c         | 411 ++++++++++++++++++++++++++++
 MAINTAINERS                         |   7 +
 hw/virtio/Kconfig                   |   5 +
 hw/virtio/meson.build               |   1 +
 hw/virtio/trace-events              |   5 +
 6 files changed, 464 insertions(+)
 create mode 100644 include/hw/virtio/vhost-user-gpio.h
 create mode 100644 hw/virtio/vhost-user-gpio.c

diff --git a/include/hw/virtio/vhost-user-gpio.h b/include/hw/virtio/vhost-user-gpio.h
new file mode 100644
index 0000000000..4fe9aeecc0
--- /dev/null
+++ b/include/hw/virtio/vhost-user-gpio.h
@@ -0,0 +1,35 @@
+/*
+ * Vhost-user GPIO virtio device
+ *
+ * Copyright (c) 2021 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _QEMU_VHOST_USER_GPIO_H
+#define _QEMU_VHOST_USER_GPIO_H
+
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user.h"
+#include "standard-headers/linux/virtio_gpio.h"
+#include "chardev/char-fe.h"
+
+#define TYPE_VHOST_USER_GPIO "vhost-user-gpio-device"
+OBJECT_DECLARE_SIMPLE_TYPE(VHostUserGPIO, VHOST_USER_GPIO);
+
+struct VHostUserGPIO {
+    /*< private >*/
+    VirtIODevice parent_obj;
+    CharBackend chardev;
+    struct virtio_gpio_config config;
+    struct vhost_virtqueue *vhost_vq;
+    struct vhost_dev vhost_dev;
+    VhostUserState vhost_user;
+    VirtQueue *command_vq;
+    VirtQueue *interrupt_vq;
+    bool connected;
+    /*< public >*/
+};
+
+#endif /* _QEMU_VHOST_USER_GPIO_H */
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
new file mode 100644
index 0000000000..8b40fe450c
--- /dev/null
+++ b/hw/virtio/vhost-user-gpio.c
@@ -0,0 +1,411 @@
+/*
+ * Vhost-user GPIO virtio device
+ *
+ * Copyright (c) 2022 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/vhost-user-gpio.h"
+#include "qemu/error-report.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "trace.h"
+
+#define REALIZE_CONNECTION_RETRIES 3
+
+/* Features required from VirtIO */
+static const int feature_bits[] = {
+    VIRTIO_F_VERSION_1,
+    VIRTIO_F_NOTIFY_ON_EMPTY,
+    VIRTIO_RING_F_INDIRECT_DESC,
+    VIRTIO_RING_F_EVENT_IDX,
+    VIRTIO_GPIO_F_IRQ,
+    VHOST_INVALID_FEATURE_BIT
+};
+
+static void vu_gpio_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    memcpy(config, &gpio->config, sizeof(gpio->config));
+}
+
+static int vu_gpio_config_notifier(struct vhost_dev *dev)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev->vdev);
+
+    memcpy(dev->vdev->config, &gpio->config, sizeof(gpio->config));
+    virtio_notify_config(dev->vdev);
+
+    return 0;
+}
+
+const VhostDevConfigOps gpio_ops = {
+    .vhost_dev_config_notifier = vu_gpio_config_notifier,
+};
+
+static int vu_gpio_start(VirtIODevice *vdev)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
+    int ret, i;
+
+    if (!k->set_guest_notifiers) {
+        error_report("binding does not support guest notifiers");
+        return -ENOSYS;
+    }
+
+    ret = vhost_dev_enable_notifiers(vhost_dev, vdev);
+    if (ret < 0) {
+        error_report("Error enabling host notifiers: %d", ret);
+        return ret;
+    }
+
+    ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, true);
+    if (ret < 0) {
+        error_report("Error binding guest notifier: %d", ret);
+        goto err_host_notifiers;
+    }
+
+    /*
+     * Before we start up we need to ensure we have the final feature
+     * set needed for the vhost configuration. The backend may also
+     * apply backend_features when the feature set is sent.
+     */
+    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
+
+    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
+    if (ret < 0) {
+        error_report("Error starting vhost-user-gpio: %d", ret);
+        goto err_guest_notifiers;
+    }
+
+    /*
+     * guest_notifier_mask/pending not used yet, so just unmask
+     * everything here. virtio-pci will do the right thing by
+     * enabling/disabling irqfd.
+     */
+    for (i = 0; i < gpio->vhost_dev.nvqs; i++) {
+        vhost_virtqueue_mask(&gpio->vhost_dev, vdev, i, false);
+    }
+
+    /*
+     * As we must have VHOST_USER_F_PROTOCOL_FEATURES (because
+     * VHOST_USER_GET_CONFIG requires it) we need to explicitly enable
+     * the vrings.
+     */
+    g_assert(vhost_dev->vhost_ops &&
+             vhost_dev->vhost_ops->vhost_set_vring_enable);
+    ret = vhost_dev->vhost_ops->vhost_set_vring_enable(vhost_dev, true);
+    if (ret == 0) {
+        return 0;
+    }
+
+    error_report("Failed to start vrings for vhost-user-gpio: %d", ret);
+
+err_guest_notifiers:
+    k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, false);
+err_host_notifiers:
+    vhost_dev_disable_notifiers(&gpio->vhost_dev, vdev);
+
+    return ret;
+}
+
+static void vu_gpio_stop(VirtIODevice *vdev)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
+    int ret;
+
+    if (!k->set_guest_notifiers) {
+        return;
+    }
+
+    /*
+     * We can call vu_gpio_stop multiple times, for example from
+     * vm_state_notify and the final object finalisation. Check we
+     * aren't already stopped before doing so.
+     */
+    if (!vhost_dev_is_started(vhost_dev)) {
+        return;
+    }
+
+    vhost_dev_stop(vhost_dev, vdev);
+
+    ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
+    if (ret < 0) {
+        error_report("vhost guest notifier cleanup failed: %d", ret);
+        return;
+    }
+
+    vhost_dev_disable_notifiers(vhost_dev, vdev);
+}
+
+static void vu_gpio_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    bool should_start = virtio_device_started(vdev, status);
+
+    trace_virtio_gpio_set_status(status);
+
+    if (!gpio->connected) {
+        return;
+    }
+
+    if (vhost_dev_is_started(&gpio->vhost_dev) == should_start) {
+        return;
+    }
+
+    if (should_start) {
+        if (vu_gpio_start(vdev)) {
+            qemu_chr_fe_disconnect(&gpio->chardev);
+        }
+    } else {
+        vu_gpio_stop(vdev);
+    }
+}
+
+static uint64_t vu_gpio_get_features(VirtIODevice *vdev, uint64_t features,
+                                     Error **errp)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    return vhost_get_features(&gpio->vhost_dev, feature_bits, features);
+}
+
+static void vu_gpio_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    /*
+     * Not normally called; it's the daemon that handles the queue;
+     * however virtio's cleanup path can call this.
+     */
+}
+
+static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
+}
+
+static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
+{
+    virtio_delete_queue(gpio->command_vq);
+    virtio_delete_queue(gpio->interrupt_vq);
+    g_free(gpio->vhost_dev.vqs);
+    gpio->vhost_dev.vqs = NULL;
+    virtio_cleanup(vdev);
+    vhost_user_cleanup(&gpio->vhost_user);
+}
+
+static int vu_gpio_connect(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
+    int ret;
+
+    if (gpio->connected) {
+        return 0;
+    }
+    gpio->connected = true;
+
+    vhost_dev_set_config_notifier(vhost_dev, &gpio_ops);
+    gpio->vhost_user.supports_config = true;
+
+    ret = vhost_dev_init(vhost_dev, &gpio->vhost_user,
+                         VHOST_BACKEND_TYPE_USER, 0, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* restore vhost state */
+    if (virtio_device_started(vdev, vdev->status)) {
+        vu_gpio_start(vdev);
+    }
+
+    return 0;
+}
+
+static void vu_gpio_disconnect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    if (!gpio->connected) {
+        return;
+    }
+    gpio->connected = false;
+
+    vu_gpio_stop(vdev);
+    vhost_dev_cleanup(&gpio->vhost_dev);
+}
+
+static void vu_gpio_event(void *opaque, QEMUChrEvent event)
+{
+    DeviceState *dev = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    Error *local_err = NULL;
+
+    switch (event) {
+    case CHR_EVENT_OPENED:
+        if (vu_gpio_connect(dev, &local_err) < 0) {
+            qemu_chr_fe_disconnect(&gpio->chardev);
+            return;
+        }
+        break;
+    case CHR_EVENT_CLOSED:
+        vu_gpio_disconnect(dev);
+        break;
+    case CHR_EVENT_BREAK:
+    case CHR_EVENT_MUX_IN:
+    case CHR_EVENT_MUX_OUT:
+        /* Ignore */
+        break;
+    }
+}
+
+static int vu_gpio_realize_connect(VHostUserGPIO *gpio, Error **errp)
+{
+    VirtIODevice *vdev = &gpio->parent_obj;
+    DeviceState *dev = &vdev->parent_obj;
+    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
+    int ret;
+
+    ret = qemu_chr_fe_wait_connected(&gpio->chardev, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /*
+     * vu_gpio_connect() may have already connected (via the event
+     * callback) in which case it will just report success.
+     */
+    ret = vu_gpio_connect(dev, errp);
+    if (ret < 0) {
+        qemu_chr_fe_disconnect(&gpio->chardev);
+        return ret;
+    }
+    g_assert(gpio->connected);
+
+    ret = vhost_dev_get_config(vhost_dev, (uint8_t *)&gpio->config,
+                               sizeof(gpio->config), errp);
+
+    if (ret < 0) {
+        error_report("vhost-user-gpio: get config failed");
+
+        qemu_chr_fe_disconnect(&gpio->chardev);
+        vhost_dev_cleanup(vhost_dev);
+        return ret;
+    }
+
+    return 0;
+}
+
+static void vu_gpio_device_realize(DeviceState *dev, Error **errp)
+{
+    ERRP_GUARD();
+
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev);
+    int retries, ret;
+
+    if (!gpio->chardev.chr) {
+        error_setg(errp, "vhost-user-gpio: chardev is mandatory");
+        return;
+    }
+
+    if (!vhost_user_init(&gpio->vhost_user, &gpio->chardev, errp)) {
+        return;
+    }
+
+    virtio_init(vdev, VIRTIO_ID_GPIO, sizeof(gpio->config));
+
+    gpio->vhost_dev.nvqs = 2;
+    gpio->command_vq = virtio_add_queue(vdev, 256, vu_gpio_handle_output);
+    gpio->interrupt_vq = virtio_add_queue(vdev, 256, vu_gpio_handle_output);
+    gpio->vhost_dev.vqs = g_new0(struct vhost_virtqueue, gpio->vhost_dev.nvqs);
+
+    gpio->connected = false;
+
+    qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL,
+                             dev, NULL, true);
+
+    retries = REALIZE_CONNECTION_RETRIES;
+    g_assert(!*errp);
+    do {
+        if (*errp) {
+            error_prepend(errp, "Reconnecting after error: ");
+            error_report_err(*errp);
+            *errp = NULL;
+        }
+        ret = vu_gpio_realize_connect(gpio, errp);
+    } while (ret < 0 && retries--);
+
+    if (ret < 0) {
+        do_vhost_user_cleanup(vdev, gpio);
+    }
+
+    return;
+}
+
+static void vu_gpio_device_unrealize(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev);
+
+    vu_gpio_set_status(vdev, 0);
+    qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, NULL, NULL, NULL, NULL,
+                             false);
+    vhost_dev_cleanup(&gpio->vhost_dev);
+    do_vhost_user_cleanup(vdev, gpio);
+}
+
+static const VMStateDescription vu_gpio_vmstate = {
+    .name = "vhost-user-gpio",
+    .unmigratable = 1,
+};
+
+static Property vu_gpio_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserGPIO, chardev),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vu_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, vu_gpio_properties);
+    dc->vmsd = &vu_gpio_vmstate;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    vdc->realize = vu_gpio_device_realize;
+    vdc->unrealize = vu_gpio_device_unrealize;
+    vdc->get_features = vu_gpio_get_features;
+    vdc->get_config = vu_gpio_get_config;
+    vdc->set_status = vu_gpio_set_status;
+    vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
+}
+
+static const TypeInfo vu_gpio_info = {
+    .name = TYPE_VHOST_USER_GPIO,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VHostUserGPIO),
+    .class_init = vu_gpio_class_init,
+};
+
+static void vu_gpio_register_types(void)
+{
+    type_register_static(&vu_gpio_info);
+}
+
+type_init(vu_gpio_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index 5ce4227ff6..2c4749a110 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2098,6 +2098,13 @@ F: hw/virtio/vhost-user-rng-pci.c
 F: include/hw/virtio/vhost-user-rng.h
 F: tools/vhost-user-rng/*
 
+vhost-user-gpio
+M: Alex Bennée <alex.bennee@linaro.org>
+R: Viresh Kumar <viresh.kumar@linaro.org>
+S: Maintained
+F: hw/virtio/vhost-user-gpio.c
+F: include/hw/virtio/vhost-user-gpio.h
+
 virtio-crypto
 M: Gonglei <arei.gonglei@huawei.com>
 S: Supported
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index e9ecae1f50..cbfd8c7173 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -80,3 +80,8 @@ config VHOST_USER_FS
     bool
     default y
     depends on VIRTIO && VHOST_USER
+
+config VHOST_USER_GPIO
+    bool
+    default y
+    depends on VIRTIO && VHOST_USER
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 7e8877fd64..33c8e71fab 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -29,6 +29,7 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
+virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
 
 virtio_pci_ss = ss.source_set()
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-pci.c'))
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 887ca7afa8..820dadc26c 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -144,3 +144,8 @@ virtio_mem_state_response(uint16_t state) "state=%" PRIu16
 virtio_pmem_flush_request(void) "flush request"
 virtio_pmem_response(void) "flush response"
 virtio_pmem_flush_done(int type) "fsync return=%d"
+
+# virtio-gpio.c
+virtio_gpio_start(void) "start"
+virtio_gpio_stop(void) "stop"
+virtio_gpio_set_status(uint8_t status) "0x%x"
-- 
2.30.2

[PATCH v4 13/22] hw/virtio: add vhost-user-gpio-pci boilerplate

[Alex Bennée]

From: Viresh Kumar <viresh.kumar@linaro.org>

This allows is to instantiate a vhost-user-gpio device as part of a PCI
bus. It is mostly boilerplate which looks pretty similar to the
vhost-user-fs-pci device.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <5f560cab92d0d789b1c94295ec74b9952907d69d.1641987128.git.viresh.kumar@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v4
  - tweak MAINTAINER
---
 hw/virtio/vhost-user-gpio-pci.c | 69 +++++++++++++++++++++++++++++++++
 MAINTAINERS                     |  2 +-
 hw/virtio/meson.build           |  1 +
 3 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 hw/virtio/vhost-user-gpio-pci.c

diff --git a/hw/virtio/vhost-user-gpio-pci.c b/hw/virtio/vhost-user-gpio-pci.c
new file mode 100644
index 0000000000..b3028a24a1
--- /dev/null
+++ b/hw/virtio/vhost-user-gpio-pci.c
@@ -0,0 +1,69 @@
+/*
+ * Vhost-user gpio virtio device PCI glue
+ *
+ * Copyright (c) 2022 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/vhost-user-gpio.h"
+#include "hw/virtio/virtio-pci.h"
+
+struct VHostUserGPIOPCI {
+    VirtIOPCIProxy parent_obj;
+    VHostUserGPIO vdev;
+};
+
+typedef struct VHostUserGPIOPCI VHostUserGPIOPCI;
+
+#define TYPE_VHOST_USER_GPIO_PCI "vhost-user-gpio-pci-base"
+
+DECLARE_INSTANCE_CHECKER(VHostUserGPIOPCI, VHOST_USER_GPIO_PCI,
+                         TYPE_VHOST_USER_GPIO_PCI)
+
+static void vhost_user_gpio_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+    VHostUserGPIOPCI *dev = VHOST_USER_GPIO_PCI(vpci_dev);
+    DeviceState *vdev = DEVICE(&dev->vdev);
+
+    vpci_dev->nvectors = 1;
+    qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+}
+
+static void vhost_user_gpio_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+    PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+    k->realize = vhost_user_gpio_pci_realize;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
+    pcidev_k->revision = 0x00;
+    pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
+}
+
+static void vhost_user_gpio_pci_instance_init(Object *obj)
+{
+    VHostUserGPIOPCI *dev = VHOST_USER_GPIO_PCI(obj);
+
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VHOST_USER_GPIO);
+}
+
+static const VirtioPCIDeviceTypeInfo vhost_user_gpio_pci_info = {
+    .base_name = TYPE_VHOST_USER_GPIO_PCI,
+    .non_transitional_name = "vhost-user-gpio-pci",
+    .instance_size = sizeof(VHostUserGPIOPCI),
+    .instance_init = vhost_user_gpio_pci_instance_init,
+    .class_init = vhost_user_gpio_pci_class_init,
+};
+
+static void vhost_user_gpio_pci_register(void)
+{
+    virtio_pci_types_register(&vhost_user_gpio_pci_info);
+}
+
+type_init(vhost_user_gpio_pci_register);
diff --git a/MAINTAINERS b/MAINTAINERS
index 2c4749a110..bb526df674 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2102,7 +2102,7 @@ vhost-user-gpio
 M: Alex Bennée <alex.bennee@linaro.org>
 R: Viresh Kumar <viresh.kumar@linaro.org>
 S: Maintained
-F: hw/virtio/vhost-user-gpio.c
+F: hw/virtio/vhost-user-gpio*
 F: include/hw/virtio/vhost-user-gpio.h
 
 virtio-crypto
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 33c8e71fab..c14e3db10a 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -30,6 +30,7 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
+virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'], if_true: files('vhost-user-gpio-pci.c'))
 
 virtio_pci_ss = ss.source_set()
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-pci.c'))
-- 
2.30.2

[PATCH v4 14/22] tests/qtest: pass stdout/stderr down to subtests

[Alex Bennée]

When trying to work out what the virtio-net-tests where doing it was
hard because the g_test_trap_subprocess redirects all output to
/dev/null. Lift this restriction by using the appropriate flags so you
can see something similar to what the vhost-user-blk tests show when
running.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20220407150042.2338562-1-alex.bennee@linaro.org>

---
v2
  - keep dumping of CLI behind the g_test_verbose flag
v4
  - fix overly long line
---
 tests/qtest/qos-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index f97d0a08fd..01a9393399 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -185,7 +185,9 @@ static void run_one_test(const void *arg)
 static void subprocess_run_one_test(const void *arg)
 {
     const gchar *path = arg;
-    g_test_trap_subprocess(path, 0, 0);
+    g_test_trap_subprocess(path, 0,
+                           G_TEST_SUBPROCESS_INHERIT_STDOUT |
+                           G_TEST_SUBPROCESS_INHERIT_STDERR);
     g_test_trap_assert_passed();
 }
 
-- 
2.30.2

[PATCH v4 15/22] tests/qtest: add a timeout for subprocess_run_one_test

[Alex Bennée]

Hangs have been observed in the tests and currently we don't timeout
if a subprocess hangs. Rectify that.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>

---
v3
  - expand timeout to 180 at Thomas' suggestion
v4
  - fix merge conflict with earlier patch
---
 tests/qtest/qos-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 01a9393399..d958ef4be3 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -185,7 +185,7 @@ static void run_one_test(const void *arg)
 static void subprocess_run_one_test(const void *arg)
 {
     const gchar *path = arg;
-    g_test_trap_subprocess(path, 0,
+    g_test_trap_subprocess(path, 180 * G_USEC_PER_SEC,
                            G_TEST_SUBPROCESS_INHERIT_STDOUT |
                            G_TEST_SUBPROCESS_INHERIT_STDERR);
     g_test_trap_assert_passed();
-- 
2.30.2

[PATCH v4 16/22] tests/qtest: use qos_printf instead of g_test_message

[Alex Bennée]

The vhost-user tests respawn qos-test as a standalone process. As a
result the gtester framework squashes all messages coming out of it
which make it hard to debug. As the test does not care about asserting
certain messages just convert the tests to use the direct qos_printf.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/qos-test.c        |  5 +++++
 tests/qtest/vhost-user-test.c | 13 +++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index d958ef4be3..b30bbb30f7 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -321,6 +321,11 @@ static void walk_path(QOSGraphNode *orig_path, int len)
 int main(int argc, char **argv, char** envp)
 {
     g_test_init(&argc, &argv, NULL);
+
+    if (g_test_subprocess()) {
+        qos_printf("qos_test running single test in subprocess\n");
+    }
+
     if (g_test_verbose()) {
         qos_printf("ENVIRONMENT VARIABLES: {\n");
         for (char **env = envp; *env != 0; env++) {
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 8bf390be20..968113d591 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -26,6 +26,7 @@
 #include "libqos/virtio-pci.h"
 
 #include "libqos/malloc-pc.h"
+#include "libqos/qgraph_internal.h"
 #include "hw/virtio/virtio-net.h"
 
 #include "standard-headers/linux/vhost_types.h"
@@ -316,7 +317,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     }
 
     if (size != VHOST_USER_HDR_SIZE) {
-        g_test_message("Wrong message size received %d", size);
+        qos_printf("%s: Wrong message size received %d\n", __func__, size);
         return;
     }
 
@@ -327,8 +328,8 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         p += VHOST_USER_HDR_SIZE;
         size = qemu_chr_fe_read_all(chr, p, msg.size);
         if (size != msg.size) {
-            g_test_message("Wrong message size received %d != %d",
-                           size, msg.size);
+            qos_printf("%s: Wrong message size received %d != %d\n",
+                       __func__, size, msg.size);
             return;
         }
     }
@@ -450,7 +451,7 @@ static const char *init_hugepagefs(void)
     }
 
     if (access(path, R_OK | W_OK | X_OK)) {
-        g_test_message("access on path (%s): %s", path, strerror(errno));
+        qos_printf("access on path (%s): %s", path, strerror(errno));
         g_test_fail();
         return NULL;
     }
@@ -460,13 +461,13 @@ static const char *init_hugepagefs(void)
     } while (ret != 0 && errno == EINTR);
 
     if (ret != 0) {
-        g_test_message("statfs on path (%s): %s", path, strerror(errno));
+        qos_printf("statfs on path (%s): %s", path, strerror(errno));
         g_test_fail();
         return NULL;
     }
 
     if (fs.f_type != HUGETLBFS_MAGIC) {
-        g_test_message("Warning: path not on HugeTLBFS: %s", path);
+        qos_printf("Warning: path not on HugeTLBFS: %s", path);
         g_test_fail();
         return NULL;
     }
-- 
2.30.2

[PATCH v4 17/22] tests/qtest: catch unhandled vhost-user messages

[Alex Bennée]

We don't need to action every message but lets document the ones we
are expecting to consume so future tests don't get confused about
unhandled bits.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v1
  - drop g_test_fail() when we get unexpected result, that just hangs
v4
  - include ring addresses in set_vring_addr output
---
 tests/qtest/vhost-user-test.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 968113d591..f2c19839e0 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -358,12 +358,44 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         }
         break;
 
+    case VHOST_USER_SET_OWNER:
+        /*
+         * We don't need to do anything here, the remote is just
+         * letting us know it is in charge. Just log it.
+         */
+        qos_printf("set_owner: start of session\n");
+        break;
+
     case VHOST_USER_GET_PROTOCOL_FEATURES:
         if (s->vu_ops->get_protocol_features) {
             s->vu_ops->get_protocol_features(s, chr, &msg);
         }
         break;
 
+    case VHOST_USER_SET_PROTOCOL_FEATURES:
+        /*
+         * We did set VHOST_USER_F_PROTOCOL_FEATURES so its valid for
+         * the remote end to send this. There is no handshake reply so
+         * just log the details for debugging.
+         */
+        qos_printf("set_protocol_features: 0x%"PRIx64 "\n", msg.payload.u64);
+        break;
+
+        /*
+         * A real vhost-user backend would actually set the size and
+         * address of the vrings but we can simply report them.
+         */
+    case VHOST_USER_SET_VRING_NUM:
+        qos_printf("set_vring_num: %d/%d\n",
+                   msg.payload.state.index, msg.payload.state.num);
+        break;
+    case VHOST_USER_SET_VRING_ADDR:
+        qos_printf("set_vring_addr: 0x%"PRIx64"/0x%"PRIx64"/0x%"PRIx64"\n",
+                   msg.payload.addr.avail_user_addr,
+                   msg.payload.addr.desc_user_addr,
+                   msg.payload.addr.used_user_addr);
+        break;
+
     case VHOST_USER_GET_VRING_BASE:
         /* send back vring base to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
@@ -428,7 +460,18 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
 
+    case VHOST_USER_SET_VRING_ENABLE:
+        /*
+         * Another case we ignore as we don't need to respond. With a
+         * fully functioning vhost-user we would enable/disable the
+         * vring monitoring.
+         */
+        qos_printf("set_vring(%d)=%s\n", msg.payload.state.index,
+                   msg.payload.state.num ? "enabled" : "disabled");
+        break;
+
     default:
+        qos_printf("vhost-user: un-handled message: %d\n", msg.request);
         break;
     }
 
-- 
2.30.2

[PATCH v4 18/22] tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES

[Alex Bennée]

checkpatch.pl warns that non-plain asserts should be avoided so
convert the check to a plain g_assert.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index f2c19839e0..4af031c971 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -983,8 +983,7 @@ static void test_multiqueue(void *obj, void *arg, QGuestAllocator *alloc)
 static void vu_net_set_features(TestServer *s, CharBackend *chr,
         VhostUserMsg *msg)
 {
-    g_assert_cmpint(msg->payload.u64 &
-            (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES), !=, 0ULL);
+    g_assert(msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
     if (s->test_flags == TEST_FLAGS_DISCONNECT) {
         qemu_chr_fe_disconnect(chr);
         s->test_flags = TEST_FLAGS_BAD;
-- 
2.30.2

[PATCH v4 19/22] tests/qtest: add assert to catch bad features

[Alex Bennée]

No device driver (which is what the qvirtio_ access functions
represent) should be setting UNUSED(30) in the feature space. Although
existing libqos users mask it out lets ensure nothing sneaks through.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/libqos/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 09ec09b655..03056e5187 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -101,6 +101,8 @@ uint64_t qvirtio_get_features(QVirtioDevice *d)
 
 void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
 {
+    g_assert(!(features & QVIRTIO_F_BAD_FEATURE));
+
     d->features = features;
     d->bus->set_features(d, features);
 
-- 
2.30.2

[PATCH v4 20/22] tests/qtest: implement stub for VHOST_USER_GET_CONFIG

[Alex Bennée]

We don't implement the full solution because frankly none of the tests
need to at the moment. We may end up re-implementing libvhostuser in
the end.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 4af031c971..61980bfc6a 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -79,6 +79,8 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_PROTOCOL_FEATURES = 16,
     VHOST_USER_GET_QUEUE_NUM = 17,
     VHOST_USER_SET_VRING_ENABLE = 18,
+    VHOST_USER_GET_CONFIG = 24,
+    VHOST_USER_SET_CONFIG = 25,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -372,6 +374,17 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         }
         break;
 
+    case VHOST_USER_GET_CONFIG:
+        /*
+         * Treat GET_CONFIG as a NOP and just reply and let the guest
+         * consider we have updated its memory. Tests currently don't
+         * require working configs.
+         */
+        msg.flags |= VHOST_USER_REPLY_MASK;
+        p = (uint8_t *) &msg;
+        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+        break;
+
     case VHOST_USER_SET_PROTOCOL_FEATURES:
         /*
          * We did set VHOST_USER_F_PROTOCOL_FEATURES so its valid for
-- 
2.30.2

[PATCH v4 21/22] tests/qtest: add a get_features op to vhost-user-test

[Alex Bennée]

As we expand this test for more virtio devices we will need to support
different feature sets. Add a mandatory op field to fetch the list of
features needed for the test itself.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 37 +++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 61980bfc6a..fe46e28cf2 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -171,10 +171,11 @@ struct vhost_user_ops {
             const char *chr_opts);
 
     /* VHOST-USER commands. */
+    uint64_t (*get_features)(TestServer *s);
     void (*set_features)(TestServer *s, CharBackend *chr,
-            VhostUserMsg *msg);
+                         VhostUserMsg *msg);
     void (*get_protocol_features)(TestServer *s,
-            CharBackend *chr, VhostUserMsg *msg);
+                                  CharBackend *chr, VhostUserMsg *msg);
 };
 
 static const char *init_hugepagefs(void);
@@ -338,20 +339,22 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
 
     switch (msg.request) {
     case VHOST_USER_GET_FEATURES:
+        /* Mandatory for tests to define get_features */
+        g_assert(s->vu_ops->get_features);
+
         /* send back features to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.payload.u64);
-        msg.payload.u64 = 0x1ULL << VHOST_F_LOG_ALL |
-            0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
-        if (s->queues > 1) {
-            msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
-        }
+
         if (s->test_flags >= TEST_FLAGS_BAD) {
             msg.payload.u64 = 0;
             s->test_flags = TEST_FLAGS_END;
+        } else {
+            msg.payload.u64 = s->vu_ops->get_features(s);
         }
-        p = (uint8_t *) &msg;
-        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+
+        qemu_chr_fe_write_all(chr, (uint8_t *) &msg,
+                              VHOST_USER_HDR_SIZE + msg.size);
         break;
 
     case VHOST_USER_SET_FEATURES:
@@ -993,8 +996,21 @@ static void test_multiqueue(void *obj, void *arg, QGuestAllocator *alloc)
     wait_for_rings_started(s, s->queues * 2);
 }
 
+
+static uint64_t vu_net_get_features(TestServer *s)
+{
+    uint64_t features = 0x1ULL << VHOST_F_LOG_ALL |
+        0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+
+    if (s->queues > 1) {
+        features |= 0x1ULL << VIRTIO_NET_F_MQ;
+    }
+
+    return features;
+}
+
 static void vu_net_set_features(TestServer *s, CharBackend *chr,
-        VhostUserMsg *msg)
+                                VhostUserMsg *msg)
 {
     g_assert(msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
     if (s->test_flags == TEST_FLAGS_DISCONNECT) {
@@ -1023,6 +1039,7 @@ static struct vhost_user_ops g_vu_net_ops = {
 
     .append_opts = append_vhost_net_opts,
 
+    .get_features = vu_net_get_features,
     .set_features = vu_net_set_features,
     .get_protocol_features = vu_net_get_protocol_features,
 };
-- 
2.30.2

[PATCH v4 22/22] tests/qtest: enable tests for virtio-gpio

[Alex Bennée]

We don't have a virtio-gpio implementation in QEMU and only
support a vhost-user backend. The QEMU side of the code is minimal so
it should be enough to instantiate the device and pass some vhost-user
messages over the control socket. To do this we hook into the existing
vhost-user-test code and just add the bits required for gpio.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Message-Id: <20220408155704.2777166-1-alex.bennee@linaro.org>

---
v2
  - add more of the missing boilerplate
  - don't request LOG_SHMD
  - use get_features op
  - report VIRTIO_F_VERSION_1
  - more comments
v4
  - update MAINTAINERS
---
 tests/qtest/libqos/virtio-gpio.h |  35 +++++++
 tests/qtest/libqos/virtio-gpio.c | 171 +++++++++++++++++++++++++++++++
 tests/qtest/libqos/virtio.c      |   2 +-
 tests/qtest/vhost-user-test.c    |  66 ++++++++++++
 MAINTAINERS                      |   1 +
 tests/qtest/libqos/meson.build   |   1 +
 6 files changed, 275 insertions(+), 1 deletion(-)
 create mode 100644 tests/qtest/libqos/virtio-gpio.h
 create mode 100644 tests/qtest/libqos/virtio-gpio.c

diff --git a/tests/qtest/libqos/virtio-gpio.h b/tests/qtest/libqos/virtio-gpio.h
new file mode 100644
index 0000000000..f11d41bd19
--- /dev/null
+++ b/tests/qtest/libqos/virtio-gpio.h
@@ -0,0 +1,35 @@
+/*
+ * virtio-gpio structures
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef TESTS_LIBQOS_VIRTIO_GPIO_H
+#define TESTS_LIBQOS_VIRTIO_GPIO_H
+
+#include "qgraph.h"
+#include "virtio.h"
+#include "virtio-pci.h"
+
+typedef struct QVhostUserGPIO QVhostUserGPIO;
+typedef struct QVhostUserGPIOPCI QVhostUserGPIOPCI;
+typedef struct QVhostUserGPIODevice QVhostUserGPIODevice;
+
+struct QVhostUserGPIO {
+    QVirtioDevice *vdev;
+    QVirtQueue **queues;
+};
+
+struct QVhostUserGPIOPCI {
+    QVirtioPCIDevice pci_vdev;
+    QVhostUserGPIO gpio;
+};
+
+struct QVhostUserGPIODevice {
+    QOSGraphObject obj;
+    QVhostUserGPIO gpio;
+};
+
+#endif
diff --git a/tests/qtest/libqos/virtio-gpio.c b/tests/qtest/libqos/virtio-gpio.c
new file mode 100644
index 0000000000..762aa6695b
--- /dev/null
+++ b/tests/qtest/libqos/virtio-gpio.c
@@ -0,0 +1,171 @@
+/*
+ * virtio-gpio nodes for testing
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "standard-headers/linux/virtio_config.h"
+#include "../libqtest.h"
+#include "qemu/module.h"
+#include "qgraph.h"
+#include "virtio-gpio.h"
+
+static QGuestAllocator *alloc;
+
+static void virtio_gpio_cleanup(QVhostUserGPIO *gpio)
+{
+    QVirtioDevice *vdev = gpio->vdev;
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        qvirtqueue_cleanup(vdev->bus, gpio->queues[i], alloc);
+    }
+    g_free(gpio->queues);
+}
+
+/*
+ * This handles the VirtIO setup from the point of view of the driver
+ * frontend and therefor doesn't present any vhost specific features
+ * and in fact masks of the re-used bit.
+ */
+static void virtio_gpio_setup(QVhostUserGPIO *gpio)
+{
+    QVirtioDevice *vdev = gpio->vdev;
+    uint64_t features;
+    int i;
+
+    features = qvirtio_get_features(vdev);
+    features &= ~QVIRTIO_F_BAD_FEATURE;
+    qvirtio_set_features(vdev, features);
+
+    gpio->queues = g_new(QVirtQueue *, 2);
+    for (i = 0; i < 2; i++) {
+        gpio->queues[i] = qvirtqueue_setup(vdev, alloc, i);
+    }
+    qvirtio_set_driver_ok(vdev);
+}
+
+static void *qvirtio_gpio_get_driver(QVhostUserGPIO *v_gpio,
+                                     const char *interface)
+{
+    if (!g_strcmp0(interface, "vhost-user-gpio")) {
+        return v_gpio;
+    }
+    if (!g_strcmp0(interface, "virtio")) {
+        return v_gpio->vdev;
+    }
+
+    g_assert_not_reached();
+}
+
+static void *qvirtio_gpio_device_get_driver(void *object,
+                                            const char *interface)
+{
+    QVhostUserGPIODevice *v_gpio = object;
+    return qvirtio_gpio_get_driver(&v_gpio->gpio, interface);
+}
+
+/* virtio-gpio (mmio) */
+static void qvirtio_gpio_device_destructor(QOSGraphObject *obj)
+{
+    QVhostUserGPIODevice *gpio_dev = (QVhostUserGPIODevice *) obj;
+    virtio_gpio_cleanup(&gpio_dev->gpio);
+}
+
+static void qvirtio_gpio_device_start_hw(QOSGraphObject *obj)
+{
+    QVhostUserGPIODevice *gpio_dev = (QVhostUserGPIODevice *) obj;
+    virtio_gpio_setup(&gpio_dev->gpio);
+}
+
+static void *virtio_gpio_device_create(void *virtio_dev,
+                                       QGuestAllocator *t_alloc,
+                                       void *addr)
+{
+    QVhostUserGPIODevice *virtio_device = g_new0(QVhostUserGPIODevice, 1);
+    QVhostUserGPIO *interface = &virtio_device->gpio;
+
+    interface->vdev = virtio_dev;
+    alloc = t_alloc;
+
+    virtio_device->obj.get_driver = qvirtio_gpio_device_get_driver;
+    virtio_device->obj.start_hw = qvirtio_gpio_device_start_hw;
+    virtio_device->obj.destructor = qvirtio_gpio_device_destructor;
+
+    return &virtio_device->obj;
+}
+
+/* virtio-gpio-pci */
+static void qvirtio_gpio_pci_destructor(QOSGraphObject *obj)
+{
+    QVhostUserGPIOPCI *gpio_pci = (QVhostUserGPIOPCI *) obj;
+    QOSGraphObject *pci_vobj =  &gpio_pci->pci_vdev.obj;
+
+    virtio_gpio_cleanup(&gpio_pci->gpio);
+    qvirtio_pci_destructor(pci_vobj);
+}
+
+static void qvirtio_gpio_pci_start_hw(QOSGraphObject *obj)
+{
+    QVhostUserGPIOPCI *gpio_pci = (QVhostUserGPIOPCI *) obj;
+    QOSGraphObject *pci_vobj =  &gpio_pci->pci_vdev.obj;
+
+    qvirtio_pci_start_hw(pci_vobj);
+    virtio_gpio_setup(&gpio_pci->gpio);
+}
+
+static void *qvirtio_gpio_pci_get_driver(void *object, const char *interface)
+{
+    QVhostUserGPIOPCI *v_gpio = object;
+
+    if (!g_strcmp0(interface, "pci-device")) {
+        return v_gpio->pci_vdev.pdev;
+    }
+    return qvirtio_gpio_get_driver(&v_gpio->gpio, interface);
+}
+
+static void *virtio_gpio_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
+                                    void *addr)
+{
+    QVhostUserGPIOPCI *virtio_spci = g_new0(QVhostUserGPIOPCI, 1);
+    QVhostUserGPIO *interface = &virtio_spci->gpio;
+    QOSGraphObject *obj = &virtio_spci->pci_vdev.obj;
+
+    virtio_pci_init(&virtio_spci->pci_vdev, pci_bus, addr);
+    interface->vdev = &virtio_spci->pci_vdev.vdev;
+    alloc = t_alloc;
+
+    obj->get_driver = qvirtio_gpio_pci_get_driver;
+    obj->start_hw = qvirtio_gpio_pci_start_hw;
+    obj->destructor = qvirtio_gpio_pci_destructor;
+
+    return obj;
+}
+
+static void virtio_gpio_register_nodes(void)
+{
+    QPCIAddress addr = {
+        .devfn = QPCI_DEVFN(4, 0),
+    };
+
+    QOSGraphEdgeOptions edge_opts = { };
+
+    /* vhost-user-gpio-device */
+    edge_opts.extra_device_opts = "id=gpio0,chardev=chr-vhost-user-test";
+    qos_node_create_driver("vhost-user-gpio-device",
+                            virtio_gpio_device_create);
+    qos_node_consumes("vhost-user-gpio-device", "virtio-bus", &edge_opts);
+    qos_node_produces("vhost-user-gpio-device", "vhost-user-gpio");
+
+    /* virtio-gpio-pci */
+    edge_opts.extra_device_opts = "id=gpio0,addr=04.0,chardev=chr-vhost-user-test";
+    add_qpci_address(&edge_opts, &addr);
+    qos_node_create_driver("vhost-user-gpio-pci", virtio_gpio_pci_create);
+    qos_node_consumes("vhost-user-gpio-pci", "pci-bus", &edge_opts);
+    qos_node_produces("vhost-user-gpio-pci", "vhost-user-gpio");
+}
+
+libqos_init(virtio_gpio_register_nodes);
diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 03056e5187..410513225f 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -110,7 +110,7 @@ void qvirtio_set_features(QVirtioDevice *d, uint64_t 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;
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index fe46e28cf2..d79bd28cd1 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -32,6 +32,7 @@
 #include "standard-headers/linux/vhost_types.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_net.h"
+#include "standard-headers/linux/virtio_gpio.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/vfs.h>
@@ -53,9 +54,12 @@
 #define VHOST_MAX_VIRTQUEUES    0x100
 
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VIRTIO_F_VERSION_1 32
+
 #define VHOST_USER_PROTOCOL_F_MQ 0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN   6
+#define VHOST_USER_PROTOCOL_F_CONFIG 9
 
 #define VHOST_LOG_PAGE 0x1000
 
@@ -140,6 +144,7 @@ enum {
 
 enum {
     VHOST_USER_NET,
+    VHOST_USER_GPIO,
 };
 
 typedef struct TestServer {
@@ -198,6 +203,19 @@ static void append_vhost_net_opts(TestServer *s, GString *cmd_line,
                            chr_opts, s->chr_name);
 }
 
+/*
+ * For GPIO there are no other magic devices we need to add (like
+ * block or netdev) so all we need to worry about is the vhost-user
+ * chardev socket.
+ */
+static void append_vhost_gpio_opts(TestServer *s, GString *cmd_line,
+                             const char *chr_opts)
+{
+    g_string_append_printf(cmd_line, QEMU_CMD_CHR,
+                           s->chr_name, s->socket_path,
+                           chr_opts);
+}
+
 static void append_mem_opts(TestServer *server, GString *cmd_line,
                             int size, enum test_memfd memfd)
 {
@@ -1088,3 +1106,51 @@ static void register_vhost_user_test(void)
                  test_multiqueue, &opts);
 }
 libqos_init(register_vhost_user_test);
+
+static uint64_t vu_gpio_get_features(TestServer *s)
+{
+    return 0x1ULL << VIRTIO_F_VERSION_1 |
+        0x1ULL << VIRTIO_GPIO_F_IRQ |
+        0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+}
+
+/*
+ * This stub can't handle all the message types but we should reply
+ * that we support VHOST_USER_PROTOCOL_F_CONFIG as gpio would use it
+ * talking to a read vhost-user daemon.
+ */
+static void vu_gpio_get_protocol_features(TestServer *s, CharBackend *chr,
+                                          VhostUserMsg *msg)
+{
+    /* send back features to qemu */
+    msg->flags |= VHOST_USER_REPLY_MASK;
+    msg->size = sizeof(m.payload.u64);
+    msg->payload.u64 = 1ULL << VHOST_USER_PROTOCOL_F_CONFIG;
+
+    qemu_chr_fe_write_all(chr, (uint8_t *)msg, VHOST_USER_HDR_SIZE + msg->size);
+}
+
+static struct vhost_user_ops g_vu_gpio_ops = {
+    .type = VHOST_USER_GPIO,
+
+    .append_opts = append_vhost_gpio_opts,
+
+    .get_features = vu_gpio_get_features,
+    .set_features = vu_net_set_features,
+    .get_protocol_features = vu_gpio_get_protocol_features,
+};
+
+static void register_vhost_gpio_test(void)
+{
+    QOSGraphTestOptions opts = {
+        .before = vhost_user_test_setup,
+        .subprocess = true,
+        .arg = &g_vu_gpio_ops,
+    };
+
+    qemu_add_opts(&qemu_chardev_opts);
+
+    qos_add_test("read-guest-mem/memfile",
+                 "vhost-user-gpio", test_read_guest_mem, &opts);
+}
+libqos_init(register_vhost_gpio_test);
diff --git a/MAINTAINERS b/MAINTAINERS
index bb526df674..a8a94da424 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2104,6 +2104,7 @@ R: Viresh Kumar <viresh.kumar@linaro.org>
 S: Maintained
 F: hw/virtio/vhost-user-gpio*
 F: include/hw/virtio/vhost-user-gpio.h
+F: tests/qtest/libqos/virtio-gpio.*
 
 virtio-crypto
 M: Gonglei <arei.gonglei@huawei.com>
diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
index fd5d6e5ae1..9dc815ddd4 100644
--- a/tests/qtest/libqos/meson.build
+++ b/tests/qtest/libqos/meson.build
@@ -45,6 +45,7 @@ libqos_srcs = files(
         'virtio-scsi.c',
         'virtio-serial.c',
         'virtio-iommu.c',
+        'virtio-gpio.c',
         'generic-pcihost.c',
 
         # qgraph machines:
-- 
2.30.2

Re: [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME

[Raphael Norwitz]

On Tue, Aug 02, 2022 at 10:49:59AM +0100, Alex Bennée wrote:
> The `started` field is manipulated internally within the vhost code
> except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck
> dev state in the vhost_migration_log routine). Mark that as a FIXME
> because it introduces a potential race. I think the referenced fix
> should be tracking its state locally.

I don't think we can track the state locally. As described in the commit
message for f5b22d06fb, the state is used by vhost code in the
vhost_migration_log() function so we probably need something at the
vhost level. I do agree we shouldn't re-use vdev->started.

Maybe we should add another 'active' variable in vhost_dev? I'm happy
to send a patch for that.

Until we agree on a better solution I'm happy with the FIXME.

Reviewed-by: Raphael Norwitz <raphael.norwittz@nutanix.com>

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/virtio/vhost.h      | 12 ++++++++++++
>  hw/block/vhost-user-blk.c      | 10 ++++++++--
>  hw/scsi/vhost-scsi.c           |  4 ++--
>  hw/scsi/vhost-user-scsi.c      |  2 +-
>  hw/virtio/vhost-user-fs.c      |  3 ++-
>  hw/virtio/vhost-user-i2c.c     |  4 ++--
>  hw/virtio/vhost-user-rng.c     |  4 ++--
>  hw/virtio/vhost-user-vsock.c   |  2 +-
>  hw/virtio/vhost-vsock-common.c |  3 ++-
>  hw/virtio/vhost-vsock.c        |  2 +-
>  10 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 586c5457e2..61b957e927 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -94,6 +94,7 @@ struct vhost_dev {
>      uint64_t protocol_features;
>      uint64_t max_queues;
>      uint64_t backend_cap;
> +    /* @started: is the vhost device started? */
>      bool started;
>      bool log_enabled;
>      uint64_t log_size;
> @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>   */
>  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>  
> +/**
> + * vhost_dev_is_started() - report status of vhost device
> + * @hdev: common vhost_dev structure
> + *
> + * Return the started status of the vhost device
> + */
> +static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> +{
> +    return hdev->started;
> +}
> +
>  /**
>   * vhost_dev_start() - start the vhost device
>   * @hdev: common vhost_dev structure
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..2bba42478d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>          return;
>      }
>  
> -    if (s->dev.started == should_start) {
> +    if (vhost_dev_is_started(&s->dev) == should_start) {
>          return;
>      }
>  
> @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          return;
>      }
>  
> -    if (s->dev.started) {
> +    if (vhost_dev_is_started(&s->dev)) {
>          return;
>      }
>  
> @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>               * the vhost migration code. If disconnect was caught there is an
>               * option for the general vhost code to get the dev state without
>               * knowing its type (in this case vhost-user).
> +             *
> +             * FIXME: this is sketchy to be reaching into vhost_dev
> +             * now because we are forcing something that implies we
> +             * have executed vhost_dev_stop() but that won't happen
> +             * until vhost_user_blk_stop() gets called from the bh.
> +             * Really this state check should be tracked locally.
>               */
>              s->dev.started = false;
>          }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 3059068175..bdf337a7a2 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
>          start = false;
>      }
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque)
>  
>      /* At this point, backend must be stopped, otherwise
>       * it might keep writing to memory. */
> -    assert(!vsc->dev.started);
> +    assert(!vhost_dev_is_started(&vsc->dev));
>  
>      return 0;
>  }
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 1b2f7eed98..bc37317d55 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>      bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index d2bebba785..ad0f91c607 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -20,6 +20,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-user-fs.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
> @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (fs->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&fs->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index b930cf6d5e..bc58b6c0d1 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (i2c->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev)
>      }
>      i2c->connected = false;
>  
> -    if (i2c->vhost_dev.started) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev)) {
>          vu_i2c_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index a9c1c4bc79..bc1f36c5ac 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (rng->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev)
>  
>      rng->connected = false;
>  
> -    if (rng->vhost_dev.started) {
> +    if (vhost_dev_is_started(&rng->vhost_dev)) {
>          vu_rng_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 22c1616ebd..7b67e29d83 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 7394818e00..29b9ab4f72 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -14,6 +14,7 @@
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-vsock.h"
>  #include "qemu/iov.h"
>  #include "monitor/monitor.h"
> @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque)
>       * At this point, backend must be stopped, otherwise
>       * it might keep writing to memory.
>       */
> -    assert(!vvc->vhost_dev.started);
> +    assert(!vhost_dev_is_started(&vvc->vhost_dev));
>  
>      return 0;
>  }
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 8031c164a5..7dc3c73931 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>      bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> -- 
> 2.30.2
> 

Re: [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME

[Raphael Norwitz]

On Tue, Aug 02, 2022 at 10:49:59AM +0100, Alex Bennée wrote:
> The `started` field is manipulated internally within the vhost code
> except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck
> dev state in the vhost_migration_log routine). Mark that as a FIXME
> because it introduces a potential race. I think the referenced fix
> should be tracking its state locally.

I don't think we can track the state locally. As described in the commit
message for f5b22d06fb, the state is used by vhost code in the
vhost_migration_log() function so we probably need something at the
vhost level. I do agree we shouldn't re-use vdev->started.

Maybe we should add another 'active' variable in vhost_dev? I'm happy
to send a patch for that.

Until we agree on a better solution I'm happy with the FIXME.

Reviewed-by: Raphael Norwitz <raphael.norwittz@nutanix.com>

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/virtio/vhost.h      | 12 ++++++++++++
>  hw/block/vhost-user-blk.c      | 10 ++++++++--
>  hw/scsi/vhost-scsi.c           |  4 ++--
>  hw/scsi/vhost-user-scsi.c      |  2 +-
>  hw/virtio/vhost-user-fs.c      |  3 ++-
>  hw/virtio/vhost-user-i2c.c     |  4 ++--
>  hw/virtio/vhost-user-rng.c     |  4 ++--
>  hw/virtio/vhost-user-vsock.c   |  2 +-
>  hw/virtio/vhost-vsock-common.c |  3 ++-
>  hw/virtio/vhost-vsock.c        |  2 +-
>  10 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 586c5457e2..61b957e927 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -94,6 +94,7 @@ struct vhost_dev {
>      uint64_t protocol_features;
>      uint64_t max_queues;
>      uint64_t backend_cap;
> +    /* @started: is the vhost device started? */
>      bool started;
>      bool log_enabled;
>      uint64_t log_size;
> @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>   */
>  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>  
> +/**
> + * vhost_dev_is_started() - report status of vhost device
> + * @hdev: common vhost_dev structure
> + *
> + * Return the started status of the vhost device
> + */
> +static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> +{
> +    return hdev->started;
> +}
> +
>  /**
>   * vhost_dev_start() - start the vhost device
>   * @hdev: common vhost_dev structure
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..2bba42478d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>          return;
>      }
>  
> -    if (s->dev.started == should_start) {
> +    if (vhost_dev_is_started(&s->dev) == should_start) {
>          return;
>      }
>  
> @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          return;
>      }
>  
> -    if (s->dev.started) {
> +    if (vhost_dev_is_started(&s->dev)) {
>          return;
>      }
>  
> @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>               * the vhost migration code. If disconnect was caught there is an
>               * option for the general vhost code to get the dev state without
>               * knowing its type (in this case vhost-user).
> +             *
> +             * FIXME: this is sketchy to be reaching into vhost_dev
> +             * now because we are forcing something that implies we
> +             * have executed vhost_dev_stop() but that won't happen
> +             * until vhost_user_blk_stop() gets called from the bh.
> +             * Really this state check should be tracked locally.
>               */
>              s->dev.started = false;
>          }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 3059068175..bdf337a7a2 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
>          start = false;
>      }
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque)
>  
>      /* At this point, backend must be stopped, otherwise
>       * it might keep writing to memory. */
> -    assert(!vsc->dev.started);
> +    assert(!vhost_dev_is_started(&vsc->dev));
>  
>      return 0;
>  }
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 1b2f7eed98..bc37317d55 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>      bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index d2bebba785..ad0f91c607 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -20,6 +20,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-user-fs.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
> @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (fs->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&fs->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index b930cf6d5e..bc58b6c0d1 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (i2c->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev)
>      }
>      i2c->connected = false;
>  
> -    if (i2c->vhost_dev.started) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev)) {
>          vu_i2c_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index a9c1c4bc79..bc1f36c5ac 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (rng->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev)
>  
>      rng->connected = false;
>  
> -    if (rng->vhost_dev.started) {
> +    if (vhost_dev_is_started(&rng->vhost_dev)) {
>          vu_rng_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 22c1616ebd..7b67e29d83 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 7394818e00..29b9ab4f72 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -14,6 +14,7 @@
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-vsock.h"
>  #include "qemu/iov.h"
>  #include "monitor/monitor.h"
> @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque)
>       * At this point, backend must be stopped, otherwise
>       * it might keep writing to memory.
>       */
> -    assert(!vvc->vhost_dev.started);
> +    assert(!vhost_dev_is_started(&vvc->vhost_dev));
>  
>      return 0;
>  }
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 8031c164a5..7dc3c73931 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>      bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> -- 
> 2.30.2
>

Re: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> This is an update to the previous series which fixes the last few
> niggling CI failures I was seeing.
>
>    Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
>    Date: Tue, 26 Jul 2022 20:21:29 +0100
>    Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>
>
> The CI failures were tricky to track down because they didn't occur
> locally but after patching to dump backtraces they all seem to involve
> updates to virtio_set_status() as the machine was torn down. I think
> patch that switches all users to use virtio_device_started() along
> with consistent checking of vhost_dev->started stops this from
> happening. The clean-up seems worthwhile in reducing boilerplate
> anyway.
>
> The following patches still need review:
>
>   - tests/qtest: enable tests for virtio-gpio
>   - tests/qtest: add a get_features op to vhost-user-test
>   - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
>   - tests/qtest: add assert to catch bad features
>   - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
>   - tests/qtest: catch unhandled vhost-user messages
>   - tests/qtest: use qos_printf instead of g_test_message
>   - tests/qtest: pass stdout/stderr down to subtests
>   - hw/virtio: move vhd->started check into helper and add FIXME
>   - hw/virtio: move vm_running check to virtio_device_started
>   - hw/virtio: add some vhost-user trace events
>   - hw/virtio: log potentially buggy guest drivers
>   - hw/virtio: fix some coding style issues
>   - include/hw: document vhost_dev feature life-cycle
>   - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
>   - hw/virtio: fix vhost_user_read tracepoint
>   - hw/virtio: handle un-configured shutdown in virtio-pci
>   - hw/virtio: gracefully handle unset vhost_dev vdev
>   - hw/virtio: incorporate backend features in features
<snip>

Ping?

-- 
Alex Bennée

Re: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Stefan Hajnoczi]

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

On Fri, Sep 16, 2022 at 07:51:40AM +0100, Alex Bennée wrote:
> 
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Hi,
> >
> > This is an update to the previous series which fixes the last few
> > niggling CI failures I was seeing.
> >
> >    Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
> >    Date: Tue, 26 Jul 2022 20:21:29 +0100
> >    Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>
> >
> > The CI failures were tricky to track down because they didn't occur
> > locally but after patching to dump backtraces they all seem to involve
> > updates to virtio_set_status() as the machine was torn down. I think
> > patch that switches all users to use virtio_device_started() along
> > with consistent checking of vhost_dev->started stops this from
> > happening. The clean-up seems worthwhile in reducing boilerplate
> > anyway.
> >
> > The following patches still need review:
> >
> >   - tests/qtest: enable tests for virtio-gpio
> >   - tests/qtest: add a get_features op to vhost-user-test
> >   - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
> >   - tests/qtest: add assert to catch bad features
> >   - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
> >   - tests/qtest: catch unhandled vhost-user messages
> >   - tests/qtest: use qos_printf instead of g_test_message
> >   - tests/qtest: pass stdout/stderr down to subtests
> >   - hw/virtio: move vhd->started check into helper and add FIXME
> >   - hw/virtio: move vm_running check to virtio_device_started
> >   - hw/virtio: add some vhost-user trace events
> >   - hw/virtio: log potentially buggy guest drivers
> >   - hw/virtio: fix some coding style issues
> >   - include/hw: document vhost_dev feature life-cycle
> >   - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
> >   - hw/virtio: fix vhost_user_read tracepoint
> >   - hw/virtio: handle un-configured shutdown in virtio-pci
> >   - hw/virtio: gracefully handle unset vhost_dev vdev
> >   - hw/virtio: incorporate backend features in features
> <snip>
> 
> Ping?

Who are you pinging?

Only qemu-devel is on To and there are a bunch of people on Cc.

Stefan

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

Re: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Alex Bennée]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> [[PGP Signed Part:Undecided]]
> On Fri, Sep 16, 2022 at 07:51:40AM +0100, Alex Bennée wrote:
>> 
>> Alex Bennée <alex.bennee@linaro.org> writes:
>> 
>> > Hi,
>> >
>> > This is an update to the previous series which fixes the last few
>> > niggling CI failures I was seeing.
>> >
>> >    Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
>> >    Date: Tue, 26 Jul 2022 20:21:29 +0100
>> >    Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>
>> >
>> > The CI failures were tricky to track down because they didn't occur
>> > locally but after patching to dump backtraces they all seem to involve
>> > updates to virtio_set_status() as the machine was torn down. I think
>> > patch that switches all users to use virtio_device_started() along
>> > with consistent checking of vhost_dev->started stops this from
>> > happening. The clean-up seems worthwhile in reducing boilerplate
>> > anyway.
>> >
>> > The following patches still need review:
>> >
>> >   - tests/qtest: enable tests for virtio-gpio
>> >   - tests/qtest: add a get_features op to vhost-user-test
>> >   - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
>> >   - tests/qtest: add assert to catch bad features
>> >   - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
>> >   - tests/qtest: catch unhandled vhost-user messages
>> >   - tests/qtest: use qos_printf instead of g_test_message
>> >   - tests/qtest: pass stdout/stderr down to subtests
>> >   - hw/virtio: move vhd->started check into helper and add FIXME
>> >   - hw/virtio: move vm_running check to virtio_device_started
>> >   - hw/virtio: add some vhost-user trace events
>> >   - hw/virtio: log potentially buggy guest drivers
>> >   - hw/virtio: fix some coding style issues
>> >   - include/hw: document vhost_dev feature life-cycle
>> >   - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
>> >   - hw/virtio: fix vhost_user_read tracepoint
>> >   - hw/virtio: handle un-configured shutdown in virtio-pci
>> >   - hw/virtio: gracefully handle unset vhost_dev vdev
>> >   - hw/virtio: incorporate backend features in features
>> <snip>
>> 
>> Ping?
>
> Who are you pinging?
>
> Only qemu-devel is on To and there are a bunch of people on Cc.

Well I guess MST is the maintainer for the sub-system but I was hoping
other virtio contributors had some sort of view. The process of
up-streaming a simple vhost-user stub has flushed out all sorts of
stuff.

>
> Stefan
>
> [[End of PGP Signed Part]]


-- 
Alex Bennée

Re: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Stefan Hajnoczi]

On Tue, 20 Sept 2022 at 10:18, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > [[PGP Signed Part:Undecided]]
> > On Fri, Sep 16, 2022 at 07:51:40AM +0100, Alex Bennée wrote:
> >>
> >> Alex Bennée <alex.bennee@linaro.org> writes:
> >>
> >> > Hi,
> >> >
> >> > This is an update to the previous series which fixes the last few
> >> > niggling CI failures I was seeing.
> >> >
> >> >    Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
> >> >    Date: Tue, 26 Jul 2022 20:21:29 +0100
> >> >    Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>
> >> >
> >> > The CI failures were tricky to track down because they didn't occur
> >> > locally but after patching to dump backtraces they all seem to involve
> >> > updates to virtio_set_status() as the machine was torn down. I think
> >> > patch that switches all users to use virtio_device_started() along
> >> > with consistent checking of vhost_dev->started stops this from
> >> > happening. The clean-up seems worthwhile in reducing boilerplate
> >> > anyway.
> >> >
> >> > The following patches still need review:
> >> >
> >> >   - tests/qtest: enable tests for virtio-gpio
> >> >   - tests/qtest: add a get_features op to vhost-user-test
> >> >   - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
> >> >   - tests/qtest: add assert to catch bad features
> >> >   - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
> >> >   - tests/qtest: catch unhandled vhost-user messages
> >> >   - tests/qtest: use qos_printf instead of g_test_message
> >> >   - tests/qtest: pass stdout/stderr down to subtests
> >> >   - hw/virtio: move vhd->started check into helper and add FIXME
> >> >   - hw/virtio: move vm_running check to virtio_device_started
> >> >   - hw/virtio: add some vhost-user trace events
> >> >   - hw/virtio: log potentially buggy guest drivers
> >> >   - hw/virtio: fix some coding style issues
> >> >   - include/hw: document vhost_dev feature life-cycle
> >> >   - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
> >> >   - hw/virtio: fix vhost_user_read tracepoint
> >> >   - hw/virtio: handle un-configured shutdown in virtio-pci
> >> >   - hw/virtio: gracefully handle unset vhost_dev vdev
> >> >   - hw/virtio: incorporate backend features in features
> >> <snip>
> >>
> >> Ping?
> >
> > Who are you pinging?
> >
> > Only qemu-devel is on To and there are a bunch of people on Cc.
>
> Well I guess MST is the maintainer for the sub-system but I was hoping
> other virtio contributors had some sort of view. The process of
> up-streaming a simple vhost-user stub has flushed out all sorts of
> stuff.

Okay, moving MST to To in case it helps get his attention.

Thanks,
Stefan

Re: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Michael S. Tsirkin]

On Tue, Sep 20, 2022 at 02:25:48PM -0400, Stefan Hajnoczi wrote:
> On Tue, 20 Sept 2022 at 10:18, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> >
> > > [[PGP Signed Part:Undecided]]
> > > On Fri, Sep 16, 2022 at 07:51:40AM +0100, Alex Bennée wrote:
> > >>
> > >> Alex Bennée <alex.bennee@linaro.org> writes:
> > >>
> > >> > Hi,
> > >> >
> > >> > This is an update to the previous series which fixes the last few
> > >> > niggling CI failures I was seeing.
> > >> >
> > >> >    Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
> > >> >    Date: Tue, 26 Jul 2022 20:21:29 +0100
> > >> >    Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>
> > >> >
> > >> > The CI failures were tricky to track down because they didn't occur
> > >> > locally but after patching to dump backtraces they all seem to involve
> > >> > updates to virtio_set_status() as the machine was torn down. I think
> > >> > patch that switches all users to use virtio_device_started() along
> > >> > with consistent checking of vhost_dev->started stops this from
> > >> > happening. The clean-up seems worthwhile in reducing boilerplate
> > >> > anyway.
> > >> >
> > >> > The following patches still need review:
> > >> >
> > >> >   - tests/qtest: enable tests for virtio-gpio
> > >> >   - tests/qtest: add a get_features op to vhost-user-test
> > >> >   - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
> > >> >   - tests/qtest: add assert to catch bad features
> > >> >   - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
> > >> >   - tests/qtest: catch unhandled vhost-user messages
> > >> >   - tests/qtest: use qos_printf instead of g_test_message
> > >> >   - tests/qtest: pass stdout/stderr down to subtests
> > >> >   - hw/virtio: move vhd->started check into helper and add FIXME
> > >> >   - hw/virtio: move vm_running check to virtio_device_started
> > >> >   - hw/virtio: add some vhost-user trace events
> > >> >   - hw/virtio: log potentially buggy guest drivers
> > >> >   - hw/virtio: fix some coding style issues
> > >> >   - include/hw: document vhost_dev feature life-cycle
> > >> >   - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
> > >> >   - hw/virtio: fix vhost_user_read tracepoint
> > >> >   - hw/virtio: handle un-configured shutdown in virtio-pci
> > >> >   - hw/virtio: gracefully handle unset vhost_dev vdev
> > >> >   - hw/virtio: incorporate backend features in features
> > >> <snip>
> > >>
> > >> Ping?
> > >
> > > Who are you pinging?
> > >
> > > Only qemu-devel is on To and there are a bunch of people on Cc.
> >
> > Well I guess MST is the maintainer for the sub-system but I was hoping
> > other virtio contributors had some sort of view. The process of
> > up-streaming a simple vhost-user stub has flushed out all sorts of
> > stuff.
> 
> Okay, moving MST to To in case it helps get his attention.
> 
> Thanks,
> Stefan

thanks, it's in my queue, just pulling in backlog that built up
during the forum.

-- 
MST

Re: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups

[Alex Bennée]

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

> On Tue, Sep 20, 2022 at 02:25:48PM -0400, Stefan Hajnoczi wrote:
>> On Tue, 20 Sept 2022 at 10:18, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >
>> >
>> > Stefan Hajnoczi <stefanha@redhat.com> writes:
>> >
>> > > [[PGP Signed Part:Undecided]]
>> > > On Fri, Sep 16, 2022 at 07:51:40AM +0100, Alex Bennée wrote:
>> > >>
>> > >> Alex Bennée <alex.bennee@linaro.org> writes:
>> > >>
>> > >> > Hi,
>> > >> >
>> > >> > This is an update to the previous series which fixes the last few
>> > >> > niggling CI failures I was seeing.
>> > >> >
>> > >> >    Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
>> > >> >    Date: Tue, 26 Jul 2022 20:21:29 +0100
>> > >> >    Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>
>> > >> >
>> > >> > The CI failures were tricky to track down because they didn't occur
>> > >> > locally but after patching to dump backtraces they all seem to involve
>> > >> > updates to virtio_set_status() as the machine was torn down. I think
>> > >> > patch that switches all users to use virtio_device_started() along
>> > >> > with consistent checking of vhost_dev->started stops this from
>> > >> > happening. The clean-up seems worthwhile in reducing boilerplate
>> > >> > anyway.
>> > >> >
>> > >> > The following patches still need review:
>> > >> >
>> > >> >   - tests/qtest: enable tests for virtio-gpio
>> > >> >   - tests/qtest: add a get_features op to vhost-user-test
>> > >> >   - tests/qtest: implement stub for VHOST_USER_GET_CONFIG
>> > >> >   - tests/qtest: add assert to catch bad features
>> > >> >   - tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
>> > >> >   - tests/qtest: catch unhandled vhost-user messages
>> > >> >   - tests/qtest: use qos_printf instead of g_test_message
>> > >> >   - tests/qtest: pass stdout/stderr down to subtests
>> > >> >   - hw/virtio: move vhd->started check into helper and add FIXME
>> > >> >   - hw/virtio: move vm_running check to virtio_device_started
>> > >> >   - hw/virtio: add some vhost-user trace events
>> > >> >   - hw/virtio: log potentially buggy guest drivers
>> > >> >   - hw/virtio: fix some coding style issues
>> > >> >   - include/hw: document vhost_dev feature life-cycle
>> > >> >   - include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
>> > >> >   - hw/virtio: fix vhost_user_read tracepoint
>> > >> >   - hw/virtio: handle un-configured shutdown in virtio-pci
>> > >> >   - hw/virtio: gracefully handle unset vhost_dev vdev
>> > >> >   - hw/virtio: incorporate backend features in features
>> > >> <snip>
>> > >>
>> > >> Ping?
>> > >
>> > > Who are you pinging?
>> > >
>> > > Only qemu-devel is on To and there are a bunch of people on Cc.
>> >
>> > Well I guess MST is the maintainer for the sub-system but I was hoping
>> > other virtio contributors had some sort of view. The process of
>> > up-streaming a simple vhost-user stub has flushed out all sorts of
>> > stuff.
>> 
>> Okay, moving MST to To in case it helps get his attention.
>> 
>> Thanks,
>> Stefan
>
> thanks, it's in my queue, just pulling in backlog that built up
> during the forum.

Thanks, doing the same myself ;-)

-- 
Alex Bennée

Re: [PATCH v4 01/22] hw/virtio: incorporate backend features in features

[Philippe Mathieu-Daudé via]

On 2/8/22 11:49, Alex Bennée wrote:
> There are some extra bits used over a vhost-user connection which are
> hidden from the device itself. We need to set them here to ensure we
> enable things like the protocol extensions.
> 
> Currently net/vhost-user.c has it's own inscrutable way of persisting
> this data but it really should live in the core vhost_user code.

TIL inscrutable!

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220726192150.2435175-7-alex.bennee@linaro.org>
> ---
>   hw/virtio/vhost-user.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 02/22] hw/virtio: gracefully handle unset vhost_dev vdev

[Philippe Mathieu-Daudé via]

On 2/8/22 11:49, Alex Bennée wrote:
> I've noticed asserts firing because we query the status of vdev after
> a vhost connection is closed down. Rather than faulting on the NULL
> indirect just quietly reply false.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220726192150.2435175-8-alex.bennee@linaro.org>
> ---
>   hw/virtio/vhost.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 06/22] include/hw: document vhost_dev feature life-cycle

[Philippe Mathieu-Daudé via]

On 2/8/22 11:49, Alex Bennée wrote:
> Try and explicitly document the various state of feature bits as
> related to the vhost_dev structure. Importantly the backend_features
> can advertise things like VHOST_USER_F_PROTOCOL_FEATURES which is
> never exposed to the driver and is only present in the vhost-user
> feature negotiation.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>   include/hw/virtio/vhost.h | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 07/22] hw/virtio: fix some coding style issues

[Philippe Mathieu-Daudé via]

On 2/8/22 11:49, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/virtio/vhost-user.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 09/22] hw/virtio: add some vhost-user trace events

[Philippe Mathieu-Daudé via]

On 2/8/22 11:49, Alex Bennée wrote:
> These are useful for tracing the lifetime of vhost-user connections.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/virtio/vhost.c      | 6 ++++++
>   hw/virtio/trace-events | 4 ++++
>   2 files changed, 10 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 15/22] tests/qtest: add a timeout for subprocess_run_one_test

[Philippe Mathieu-Daudé via]

On 2/8/22 11:50, Alex Bennée wrote:
> Hangs have been observed in the tests and currently we don't timeout
> if a subprocess hangs. Rectify that.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> ---
> v3
>    - expand timeout to 180 at Thomas' suggestion
> v4
>    - fix merge conflict with earlier patch
> ---
>   tests/qtest/qos-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> All the boilerplate virtio code does the same thing (or should at
> least) of checking to see if the VM is running before attempting to
> start VirtIO. Push the logic up to the common function to avoid
> getting a copy and paste wrong.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

How bad is it if we drop this?

> ---
>  include/hw/virtio/virtio.h   | 5 +++++
>  hw/virtio/vhost-user-fs.c    | 6 +-----
>  hw/virtio/vhost-user-i2c.c   | 6 +-----
>  hw/virtio/vhost-user-rng.c   | 6 +-----
>  hw/virtio/vhost-user-vsock.c | 6 +-----
>  hw/virtio/vhost-vsock.c      | 6 +-----
>  6 files changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 9bb2485415..74e7ad5a92 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -100,6 +100,7 @@ struct VirtIODevice
>      VirtQueue *vq;
>      MemoryListener listener;
>      uint16_t device_id;
> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
>      bool vm_running;
>      bool broken; /* device in invalid state, needs reset */
>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>          return vdev->started;
>      }
>  
> +    if (!vdev->vm_running) {
> +        return false;
> +    }
> +
>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
>  }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index e513e4fdda..d2bebba785 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (fs->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index 6020eee093..b930cf6d5e 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (i2c->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index 3a7bf8e32d..a9c1c4bc79 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (rng->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 0f8ff99f85..22c1616ebd 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (vvc->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 0338de892f..8031c164a5 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> +    bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> -
>      if (vvc->vhost_dev.started == should_start) {
>          return;
>      }
> -- 
> 2.30.2

Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> All the boilerplate virtio code does the same thing (or should at
> least) of checking to see if the VM is running before attempting to
> start VirtIO. Push the logic up to the common function to avoid
> getting a copy and paste wrong.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

How bad is it if we drop this?

> ---
>  include/hw/virtio/virtio.h   | 5 +++++
>  hw/virtio/vhost-user-fs.c    | 6 +-----
>  hw/virtio/vhost-user-i2c.c   | 6 +-----
>  hw/virtio/vhost-user-rng.c   | 6 +-----
>  hw/virtio/vhost-user-vsock.c | 6 +-----
>  hw/virtio/vhost-vsock.c      | 6 +-----
>  6 files changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 9bb2485415..74e7ad5a92 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -100,6 +100,7 @@ struct VirtIODevice
>      VirtQueue *vq;
>      MemoryListener listener;
>      uint16_t device_id;
> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
>      bool vm_running;
>      bool broken; /* device in invalid state, needs reset */
>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>          return vdev->started;
>      }
>  
> +    if (!vdev->vm_running) {
> +        return false;
> +    }
> +
>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
>  }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index e513e4fdda..d2bebba785 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (fs->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index 6020eee093..b930cf6d5e 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (i2c->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index 3a7bf8e32d..a9c1c4bc79 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (rng->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 0f8ff99f85..22c1616ebd 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> -
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> +    bool should_start = virtio_device_started(vdev, status);
>  
>      if (vvc->vhost_dev.started == should_start) {
>          return;
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 0338de892f..8031c164a5 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> +    bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (!vdev->vm_running) {
> -        should_start = false;
> -    }
> -
>      if (vvc->vhost_dev.started == should_start) {
>          return;
>      }
> -- 
> 2.30.2

Re: [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME

[Michael S. Tsirkin]

On Tue, Aug 02, 2022 at 10:49:59AM +0100, Alex Bennée wrote:
> The `started` field is manipulated internally within the vhost code
> except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck
> dev state in the vhost_migration_log routine). Mark that as a FIXME
> because it introduces a potential race. I think the referenced fix
> should be tracking its state locally.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

And I guess this for good measure.

> ---
>  include/hw/virtio/vhost.h      | 12 ++++++++++++
>  hw/block/vhost-user-blk.c      | 10 ++++++++--
>  hw/scsi/vhost-scsi.c           |  4 ++--
>  hw/scsi/vhost-user-scsi.c      |  2 +-
>  hw/virtio/vhost-user-fs.c      |  3 ++-
>  hw/virtio/vhost-user-i2c.c     |  4 ++--
>  hw/virtio/vhost-user-rng.c     |  4 ++--
>  hw/virtio/vhost-user-vsock.c   |  2 +-
>  hw/virtio/vhost-vsock-common.c |  3 ++-
>  hw/virtio/vhost-vsock.c        |  2 +-
>  10 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 586c5457e2..61b957e927 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -94,6 +94,7 @@ struct vhost_dev {
>      uint64_t protocol_features;
>      uint64_t max_queues;
>      uint64_t backend_cap;
> +    /* @started: is the vhost device started? */
>      bool started;
>      bool log_enabled;
>      uint64_t log_size;
> @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>   */
>  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>  
> +/**
> + * vhost_dev_is_started() - report status of vhost device
> + * @hdev: common vhost_dev structure
> + *
> + * Return the started status of the vhost device
> + */
> +static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> +{
> +    return hdev->started;
> +}
> +
>  /**
>   * vhost_dev_start() - start the vhost device
>   * @hdev: common vhost_dev structure
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..2bba42478d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>          return;
>      }
>  
> -    if (s->dev.started == should_start) {
> +    if (vhost_dev_is_started(&s->dev) == should_start) {
>          return;
>      }
>  
> @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          return;
>      }
>  
> -    if (s->dev.started) {
> +    if (vhost_dev_is_started(&s->dev)) {
>          return;
>      }
>  
> @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>               * the vhost migration code. If disconnect was caught there is an
>               * option for the general vhost code to get the dev state without
>               * knowing its type (in this case vhost-user).
> +             *
> +             * FIXME: this is sketchy to be reaching into vhost_dev
> +             * now because we are forcing something that implies we
> +             * have executed vhost_dev_stop() but that won't happen
> +             * until vhost_user_blk_stop() gets called from the bh.
> +             * Really this state check should be tracked locally.
>               */
>              s->dev.started = false;
>          }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 3059068175..bdf337a7a2 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
>          start = false;
>      }
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque)
>  
>      /* At this point, backend must be stopped, otherwise
>       * it might keep writing to memory. */
> -    assert(!vsc->dev.started);
> +    assert(!vhost_dev_is_started(&vsc->dev));
>  
>      return 0;
>  }
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 1b2f7eed98..bc37317d55 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>      bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index d2bebba785..ad0f91c607 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -20,6 +20,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-user-fs.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
> @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (fs->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&fs->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index b930cf6d5e..bc58b6c0d1 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (i2c->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev)
>      }
>      i2c->connected = false;
>  
> -    if (i2c->vhost_dev.started) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev)) {
>          vu_i2c_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index a9c1c4bc79..bc1f36c5ac 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (rng->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev)
>  
>      rng->connected = false;
>  
> -    if (rng->vhost_dev.started) {
> +    if (vhost_dev_is_started(&rng->vhost_dev)) {
>          vu_rng_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 22c1616ebd..7b67e29d83 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 7394818e00..29b9ab4f72 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -14,6 +14,7 @@
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-vsock.h"
>  #include "qemu/iov.h"
>  #include "monitor/monitor.h"
> @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque)
>       * At this point, backend must be stopped, otherwise
>       * it might keep writing to memory.
>       */
> -    assert(!vvc->vhost_dev.started);
> +    assert(!vhost_dev_is_started(&vvc->vhost_dev));
>  
>      return 0;
>  }
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 8031c164a5..7dc3c73931 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>      bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> -- 
> 2.30.2

Re: [Virtio-fs] [PATCH v4 11/22] hw/virtio: move vhd->started check into helper and add FIXME

[Michael S. Tsirkin]

On Tue, Aug 02, 2022 at 10:49:59AM +0100, Alex Bennée wrote:
> The `started` field is manipulated internally within the vhost code
> except for one place, vhost-user-blk via f5b22d06fb (vhost: recheck
> dev state in the vhost_migration_log routine). Mark that as a FIXME
> because it introduces a potential race. I think the referenced fix
> should be tracking its state locally.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

And I guess this for good measure.

> ---
>  include/hw/virtio/vhost.h      | 12 ++++++++++++
>  hw/block/vhost-user-blk.c      | 10 ++++++++--
>  hw/scsi/vhost-scsi.c           |  4 ++--
>  hw/scsi/vhost-user-scsi.c      |  2 +-
>  hw/virtio/vhost-user-fs.c      |  3 ++-
>  hw/virtio/vhost-user-i2c.c     |  4 ++--
>  hw/virtio/vhost-user-rng.c     |  4 ++--
>  hw/virtio/vhost-user-vsock.c   |  2 +-
>  hw/virtio/vhost-vsock-common.c |  3 ++-
>  hw/virtio/vhost-vsock.c        |  2 +-
>  10 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 586c5457e2..61b957e927 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -94,6 +94,7 @@ struct vhost_dev {
>      uint64_t protocol_features;
>      uint64_t max_queues;
>      uint64_t backend_cap;
> +    /* @started: is the vhost device started? */
>      bool started;
>      bool log_enabled;
>      uint64_t log_size;
> @@ -165,6 +166,17 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>   */
>  void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>  
> +/**
> + * vhost_dev_is_started() - report status of vhost device
> + * @hdev: common vhost_dev structure
> + *
> + * Return the started status of the vhost device
> + */
> +static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> +{
> +    return hdev->started;
> +}
> +
>  /**
>   * vhost_dev_start() - start the vhost device
>   * @hdev: common vhost_dev structure
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..2bba42478d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -229,7 +229,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>          return;
>      }
>  
> -    if (s->dev.started == should_start) {
> +    if (vhost_dev_is_started(&s->dev) == should_start) {
>          return;
>      }
>  
> @@ -286,7 +286,7 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          return;
>      }
>  
> -    if (s->dev.started) {
> +    if (vhost_dev_is_started(&s->dev)) {
>          return;
>      }
>  
> @@ -415,6 +415,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>               * the vhost migration code. If disconnect was caught there is an
>               * option for the general vhost code to get the dev state without
>               * knowing its type (in this case vhost-user).
> +             *
> +             * FIXME: this is sketchy to be reaching into vhost_dev
> +             * now because we are forcing something that implies we
> +             * have executed vhost_dev_stop() but that won't happen
> +             * until vhost_user_blk_stop() gets called from the bh.
> +             * Really this state check should be tracked locally.
>               */
>              s->dev.started = false;
>          }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 3059068175..bdf337a7a2 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -120,7 +120,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
>          start = false;
>      }
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> @@ -147,7 +147,7 @@ static int vhost_scsi_pre_save(void *opaque)
>  
>      /* At this point, backend must be stopped, otherwise
>       * it might keep writing to memory. */
> -    assert(!vsc->dev.started);
> +    assert(!vhost_dev_is_started(&vsc->dev));
>  
>      return 0;
>  }
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 1b2f7eed98..bc37317d55 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -49,7 +49,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>      bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
>  
> -    if (vsc->dev.started == start) {
> +    if (vhost_dev_is_started(&vsc->dev) == start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index d2bebba785..ad0f91c607 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -20,6 +20,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-user-fs.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
> @@ -124,7 +125,7 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (fs->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&fs->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index b930cf6d5e..bc58b6c0d1 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -95,7 +95,7 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (i2c->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -174,7 +174,7 @@ static void vu_i2c_disconnect(DeviceState *dev)
>      }
>      i2c->connected = false;
>  
> -    if (i2c->vhost_dev.started) {
> +    if (vhost_dev_is_started(&i2c->vhost_dev)) {
>          vu_i2c_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index a9c1c4bc79..bc1f36c5ac 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -92,7 +92,7 @@ static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (rng->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
>          return;
>      }
>  
> @@ -160,7 +160,7 @@ static void vu_rng_disconnect(DeviceState *dev)
>  
>      rng->connected = false;
>  
> -    if (rng->vhost_dev.started) {
> +    if (vhost_dev_is_started(&rng->vhost_dev)) {
>          vu_rng_stop(vdev);
>      }
>  }
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index 22c1616ebd..7b67e29d83 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -57,7 +57,7 @@ static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>      bool should_start = virtio_device_started(vdev, status);
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 7394818e00..29b9ab4f72 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -14,6 +14,7 @@
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/error-report.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-vsock.h"
>  #include "qemu/iov.h"
>  #include "monitor/monitor.h"
> @@ -199,7 +200,7 @@ int vhost_vsock_common_pre_save(void *opaque)
>       * At this point, backend must be stopped, otherwise
>       * it might keep writing to memory.
>       */
> -    assert(!vvc->vhost_dev.started);
> +    assert(!vhost_dev_is_started(&vvc->vhost_dev));
>  
>      return 0;
>  }
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 8031c164a5..7dc3c73931 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -73,7 +73,7 @@ static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>      bool should_start = virtio_device_started(vdev, status);
>      int ret;
>  
> -    if (vvc->vhost_dev.started == should_start) {
> +    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>          return;
>      }
>  
> -- 
> 2.30.2

Re: [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Alex Bennée]

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

> On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
>> All the boilerplate virtio code does the same thing (or should at
>> least) of checking to see if the VM is running before attempting to
>> start VirtIO. Push the logic up to the common function to avoid
>> getting a copy and paste wrong.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> How bad is it if we drop this?


I assume it will break gpio. Why do we want to drop this now? It has
been merged awhile. However there was a follow-up patch which tweaked
the order of checks in virtio_device_started.

>
>> ---
>>  include/hw/virtio/virtio.h   | 5 +++++
>>  hw/virtio/vhost-user-fs.c    | 6 +-----
>>  hw/virtio/vhost-user-i2c.c   | 6 +-----
>>  hw/virtio/vhost-user-rng.c   | 6 +-----
>>  hw/virtio/vhost-user-vsock.c | 6 +-----
>>  hw/virtio/vhost-vsock.c      | 6 +-----
>>  6 files changed, 10 insertions(+), 25 deletions(-)
>> 
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 9bb2485415..74e7ad5a92 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -100,6 +100,7 @@ struct VirtIODevice
>>      VirtQueue *vq;
>>      MemoryListener listener;
>>      uint16_t device_id;
>> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
>>      bool vm_running;
>>      bool broken; /* device in invalid state, needs reset */
>>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
>> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>>          return vdev->started;
>>      }
>>  
>> +    if (!vdev->vm_running) {
>> +        return false;
>> +    }
>> +
>>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
>>  }
>>  
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index e513e4fdda..d2bebba785 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (fs->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>> index 6020eee093..b930cf6d5e 100644
>> --- a/hw/virtio/vhost-user-i2c.c
>> +++ b/hw/virtio/vhost-user-i2c.c
>> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (i2c->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>> index 3a7bf8e32d..a9c1c4bc79 100644
>> --- a/hw/virtio/vhost-user-rng.c
>> +++ b/hw/virtio/vhost-user-rng.c
>> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (rng->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>> index 0f8ff99f85..22c1616ebd 100644
>> --- a/hw/virtio/vhost-user-vsock.c
>> +++ b/hw/virtio/vhost-user-vsock.c
>> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (vvc->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index 0338de892f..8031c164a5 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> +    bool should_start = virtio_device_started(vdev, status);
>>      int ret;
>>  
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> -
>>      if (vvc->vhost_dev.started == should_start) {
>>          return;
>>      }
>> -- 
>> 2.30.2


-- 
Alex Bennée

Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Alex Bennée]

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

> On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
>> All the boilerplate virtio code does the same thing (or should at
>> least) of checking to see if the VM is running before attempting to
>> start VirtIO. Push the logic up to the common function to avoid
>> getting a copy and paste wrong.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> How bad is it if we drop this?


I assume it will break gpio. Why do we want to drop this now? It has
been merged awhile. However there was a follow-up patch which tweaked
the order of checks in virtio_device_started.

>
>> ---
>>  include/hw/virtio/virtio.h   | 5 +++++
>>  hw/virtio/vhost-user-fs.c    | 6 +-----
>>  hw/virtio/vhost-user-i2c.c   | 6 +-----
>>  hw/virtio/vhost-user-rng.c   | 6 +-----
>>  hw/virtio/vhost-user-vsock.c | 6 +-----
>>  hw/virtio/vhost-vsock.c      | 6 +-----
>>  6 files changed, 10 insertions(+), 25 deletions(-)
>> 
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 9bb2485415..74e7ad5a92 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -100,6 +100,7 @@ struct VirtIODevice
>>      VirtQueue *vq;
>>      MemoryListener listener;
>>      uint16_t device_id;
>> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
>>      bool vm_running;
>>      bool broken; /* device in invalid state, needs reset */
>>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
>> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>>          return vdev->started;
>>      }
>>  
>> +    if (!vdev->vm_running) {
>> +        return false;
>> +    }
>> +
>>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
>>  }
>>  
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index e513e4fdda..d2bebba785 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (fs->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>> index 6020eee093..b930cf6d5e 100644
>> --- a/hw/virtio/vhost-user-i2c.c
>> +++ b/hw/virtio/vhost-user-i2c.c
>> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (i2c->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>> index 3a7bf8e32d..a9c1c4bc79 100644
>> --- a/hw/virtio/vhost-user-rng.c
>> +++ b/hw/virtio/vhost-user-rng.c
>> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (rng->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>> index 0f8ff99f85..22c1616ebd 100644
>> --- a/hw/virtio/vhost-user-vsock.c
>> +++ b/hw/virtio/vhost-user-vsock.c
>> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> +    bool should_start = virtio_device_started(vdev, status);
>>  
>>      if (vvc->vhost_dev.started == should_start) {
>>          return;
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index 0338de892f..8031c164a5 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>>  {
>>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> +    bool should_start = virtio_device_started(vdev, status);
>>      int ret;
>>  
>> -    if (!vdev->vm_running) {
>> -        should_start = false;
>> -    }
>> -
>>      if (vvc->vhost_dev.started == should_start) {
>>          return;
>>      }
>> -- 
>> 2.30.2


-- 
Alex Bennée

Re: [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote:
> 
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> >> All the boilerplate virtio code does the same thing (or should at
> >> least) of checking to see if the VM is running before attempting to
> >> start VirtIO. Push the logic up to the common function to avoid
> >> getting a copy and paste wrong.
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > How bad is it if we drop this?
> 
> 
> I assume it will break gpio. Why do we want to drop this now? It has
> been merged awhile. However there was a follow-up patch which tweaked
> the order of checks in virtio_device_started.

And that follow up patch trips up UBSAN:

https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327




> >
> >> ---
> >>  include/hw/virtio/virtio.h   | 5 +++++
> >>  hw/virtio/vhost-user-fs.c    | 6 +-----
> >>  hw/virtio/vhost-user-i2c.c   | 6 +-----
> >>  hw/virtio/vhost-user-rng.c   | 6 +-----
> >>  hw/virtio/vhost-user-vsock.c | 6 +-----
> >>  hw/virtio/vhost-vsock.c      | 6 +-----
> >>  6 files changed, 10 insertions(+), 25 deletions(-)
> >> 
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index 9bb2485415..74e7ad5a92 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -100,6 +100,7 @@ struct VirtIODevice
> >>      VirtQueue *vq;
> >>      MemoryListener listener;
> >>      uint16_t device_id;
> >> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
> >>      bool vm_running;
> >>      bool broken; /* device in invalid state, needs reset */
> >>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> >> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> >>          return vdev->started;
> >>      }
> >>  
> >> +    if (!vdev->vm_running) {
> >> +        return false;
> >> +    }
> >> +
> >>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
> >>  }
> >>  
> >> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> >> index e513e4fdda..d2bebba785 100644
> >> --- a/hw/virtio/vhost-user-fs.c
> >> +++ b/hw/virtio/vhost-user-fs.c
> >> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
> >>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (fs->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> >> index 6020eee093..b930cf6d5e 100644
> >> --- a/hw/virtio/vhost-user-i2c.c
> >> +++ b/hw/virtio/vhost-user-i2c.c
> >> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> >>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (i2c->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> >> index 3a7bf8e32d..a9c1c4bc79 100644
> >> --- a/hw/virtio/vhost-user-rng.c
> >> +++ b/hw/virtio/vhost-user-rng.c
> >> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> >>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (rng->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> >> index 0f8ff99f85..22c1616ebd 100644
> >> --- a/hw/virtio/vhost-user-vsock.c
> >> +++ b/hw/virtio/vhost-user-vsock.c
> >> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
> >>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (vvc->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> >> index 0338de892f..8031c164a5 100644
> >> --- a/hw/virtio/vhost-vsock.c
> >> +++ b/hw/virtio/vhost-vsock.c
> >> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
> >>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>      int ret;
> >>  
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> -
> >>      if (vvc->vhost_dev.started == should_start) {
> >>          return;
> >>      }
> >> -- 
> >> 2.30.2
> 
> 
> -- 
> Alex Bennée

Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote:
> 
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> >> All the boilerplate virtio code does the same thing (or should at
> >> least) of checking to see if the VM is running before attempting to
> >> start VirtIO. Push the logic up to the common function to avoid
> >> getting a copy and paste wrong.
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > How bad is it if we drop this?
> 
> 
> I assume it will break gpio. Why do we want to drop this now? It has
> been merged awhile. However there was a follow-up patch which tweaked
> the order of checks in virtio_device_started.

And that follow up patch trips up UBSAN:

https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327




> >
> >> ---
> >>  include/hw/virtio/virtio.h   | 5 +++++
> >>  hw/virtio/vhost-user-fs.c    | 6 +-----
> >>  hw/virtio/vhost-user-i2c.c   | 6 +-----
> >>  hw/virtio/vhost-user-rng.c   | 6 +-----
> >>  hw/virtio/vhost-user-vsock.c | 6 +-----
> >>  hw/virtio/vhost-vsock.c      | 6 +-----
> >>  6 files changed, 10 insertions(+), 25 deletions(-)
> >> 
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index 9bb2485415..74e7ad5a92 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -100,6 +100,7 @@ struct VirtIODevice
> >>      VirtQueue *vq;
> >>      MemoryListener listener;
> >>      uint16_t device_id;
> >> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
> >>      bool vm_running;
> >>      bool broken; /* device in invalid state, needs reset */
> >>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> >> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> >>          return vdev->started;
> >>      }
> >>  
> >> +    if (!vdev->vm_running) {
> >> +        return false;
> >> +    }
> >> +
> >>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
> >>  }
> >>  
> >> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> >> index e513e4fdda..d2bebba785 100644
> >> --- a/hw/virtio/vhost-user-fs.c
> >> +++ b/hw/virtio/vhost-user-fs.c
> >> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
> >>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (fs->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> >> index 6020eee093..b930cf6d5e 100644
> >> --- a/hw/virtio/vhost-user-i2c.c
> >> +++ b/hw/virtio/vhost-user-i2c.c
> >> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> >>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (i2c->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> >> index 3a7bf8e32d..a9c1c4bc79 100644
> >> --- a/hw/virtio/vhost-user-rng.c
> >> +++ b/hw/virtio/vhost-user-rng.c
> >> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> >>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (rng->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> >> index 0f8ff99f85..22c1616ebd 100644
> >> --- a/hw/virtio/vhost-user-vsock.c
> >> +++ b/hw/virtio/vhost-user-vsock.c
> >> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
> >>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> -
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>  
> >>      if (vvc->vhost_dev.started == should_start) {
> >>          return;
> >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> >> index 0338de892f..8031c164a5 100644
> >> --- a/hw/virtio/vhost-vsock.c
> >> +++ b/hw/virtio/vhost-vsock.c
> >> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
> >>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> >>  {
> >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> >> +    bool should_start = virtio_device_started(vdev, status);
> >>      int ret;
> >>  
> >> -    if (!vdev->vm_running) {
> >> -        should_start = false;
> >> -    }
> >> -
> >>      if (vvc->vhost_dev.started == should_start) {
> >>          return;
> >>      }
> >> -- 
> >> 2.30.2
> 
> 
> -- 
> Alex Bennée

Re: [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Thu, Nov 03, 2022 at 04:30:48PM -0400, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote:
> > 
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > 
> > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> > >> All the boilerplate virtio code does the same thing (or should at
> > >> least) of checking to see if the VM is running before attempting to
> > >> start VirtIO. Push the logic up to the common function to avoid
> > >> getting a copy and paste wrong.
> > >> 
> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > >
> > > How bad is it if we drop this?
> > 
> > 
> > I assume it will break gpio. Why do we want to drop this now? It has
> > been merged awhile. However there was a follow-up patch which tweaked
> > the order of checks in virtio_device_started.
> 
> And that follow up patch trips up UBSAN:
> 
> https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327
> 


And more specifically this

https://gitlab.com/mitsirkin/qemu/-/jobs/3269957848

> 
> 
> > >
> > >> ---
> > >>  include/hw/virtio/virtio.h   | 5 +++++
> > >>  hw/virtio/vhost-user-fs.c    | 6 +-----
> > >>  hw/virtio/vhost-user-i2c.c   | 6 +-----
> > >>  hw/virtio/vhost-user-rng.c   | 6 +-----
> > >>  hw/virtio/vhost-user-vsock.c | 6 +-----
> > >>  hw/virtio/vhost-vsock.c      | 6 +-----
> > >>  6 files changed, 10 insertions(+), 25 deletions(-)
> > >> 
> > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > >> index 9bb2485415..74e7ad5a92 100644
> > >> --- a/include/hw/virtio/virtio.h
> > >> +++ b/include/hw/virtio/virtio.h
> > >> @@ -100,6 +100,7 @@ struct VirtIODevice
> > >>      VirtQueue *vq;
> > >>      MemoryListener listener;
> > >>      uint16_t device_id;
> > >> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
> > >>      bool vm_running;
> > >>      bool broken; /* device in invalid state, needs reset */
> > >>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> > >> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> > >>          return vdev->started;
> > >>      }
> > >>  
> > >> +    if (!vdev->vm_running) {
> > >> +        return false;
> > >> +    }
> > >> +
> > >>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >>  }
> > >>  
> > >> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > >> index e513e4fdda..d2bebba785 100644
> > >> --- a/hw/virtio/vhost-user-fs.c
> > >> +++ b/hw/virtio/vhost-user-fs.c
> > >> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
> > >>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (fs->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > >> index 6020eee093..b930cf6d5e 100644
> > >> --- a/hw/virtio/vhost-user-i2c.c
> > >> +++ b/hw/virtio/vhost-user-i2c.c
> > >> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> > >>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (i2c->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > >> index 3a7bf8e32d..a9c1c4bc79 100644
> > >> --- a/hw/virtio/vhost-user-rng.c
> > >> +++ b/hw/virtio/vhost-user-rng.c
> > >> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> > >>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (rng->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> > >> index 0f8ff99f85..22c1616ebd 100644
> > >> --- a/hw/virtio/vhost-user-vsock.c
> > >> +++ b/hw/virtio/vhost-user-vsock.c
> > >> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
> > >>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (vvc->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> > >> index 0338de892f..8031c164a5 100644
> > >> --- a/hw/virtio/vhost-vsock.c
> > >> +++ b/hw/virtio/vhost-vsock.c
> > >> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
> > >>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>      int ret;
> > >>  
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> -
> > >>      if (vvc->vhost_dev.started == should_start) {
> > >>          return;
> > >>      }
> > >> -- 
> > >> 2.30.2
> > 
> > 
> > -- 
> > Alex Bennée

Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Thu, Nov 03, 2022 at 04:30:48PM -0400, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote:
> > 
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > 
> > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> > >> All the boilerplate virtio code does the same thing (or should at
> > >> least) of checking to see if the VM is running before attempting to
> > >> start VirtIO. Push the logic up to the common function to avoid
> > >> getting a copy and paste wrong.
> > >> 
> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > >
> > > How bad is it if we drop this?
> > 
> > 
> > I assume it will break gpio. Why do we want to drop this now? It has
> > been merged awhile. However there was a follow-up patch which tweaked
> > the order of checks in virtio_device_started.
> 
> And that follow up patch trips up UBSAN:
> 
> https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327
> 


And more specifically this

https://gitlab.com/mitsirkin/qemu/-/jobs/3269957848

> 
> 
> > >
> > >> ---
> > >>  include/hw/virtio/virtio.h   | 5 +++++
> > >>  hw/virtio/vhost-user-fs.c    | 6 +-----
> > >>  hw/virtio/vhost-user-i2c.c   | 6 +-----
> > >>  hw/virtio/vhost-user-rng.c   | 6 +-----
> > >>  hw/virtio/vhost-user-vsock.c | 6 +-----
> > >>  hw/virtio/vhost-vsock.c      | 6 +-----
> > >>  6 files changed, 10 insertions(+), 25 deletions(-)
> > >> 
> > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > >> index 9bb2485415..74e7ad5a92 100644
> > >> --- a/include/hw/virtio/virtio.h
> > >> +++ b/include/hw/virtio/virtio.h
> > >> @@ -100,6 +100,7 @@ struct VirtIODevice
> > >>      VirtQueue *vq;
> > >>      MemoryListener listener;
> > >>      uint16_t device_id;
> > >> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
> > >>      bool vm_running;
> > >>      bool broken; /* device in invalid state, needs reset */
> > >>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> > >> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> > >>          return vdev->started;
> > >>      }
> > >>  
> > >> +    if (!vdev->vm_running) {
> > >> +        return false;
> > >> +    }
> > >> +
> > >>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >>  }
> > >>  
> > >> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > >> index e513e4fdda..d2bebba785 100644
> > >> --- a/hw/virtio/vhost-user-fs.c
> > >> +++ b/hw/virtio/vhost-user-fs.c
> > >> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
> > >>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (fs->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > >> index 6020eee093..b930cf6d5e 100644
> > >> --- a/hw/virtio/vhost-user-i2c.c
> > >> +++ b/hw/virtio/vhost-user-i2c.c
> > >> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> > >>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (i2c->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > >> index 3a7bf8e32d..a9c1c4bc79 100644
> > >> --- a/hw/virtio/vhost-user-rng.c
> > >> +++ b/hw/virtio/vhost-user-rng.c
> > >> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> > >>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (rng->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> > >> index 0f8ff99f85..22c1616ebd 100644
> > >> --- a/hw/virtio/vhost-user-vsock.c
> > >> +++ b/hw/virtio/vhost-user-vsock.c
> > >> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
> > >>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> -
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>  
> > >>      if (vvc->vhost_dev.started == should_start) {
> > >>          return;
> > >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> > >> index 0338de892f..8031c164a5 100644
> > >> --- a/hw/virtio/vhost-vsock.c
> > >> +++ b/hw/virtio/vhost-vsock.c
> > >> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
> > >>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> > >>  {
> > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > >> +    bool should_start = virtio_device_started(vdev, status);
> > >>      int ret;
> > >>  
> > >> -    if (!vdev->vm_running) {
> > >> -        should_start = false;
> > >> -    }
> > >> -
> > >>      if (vvc->vhost_dev.started == should_start) {
> > >>          return;
> > >>      }
> > >> -- 
> > >> 2.30.2
> > 
> > 
> > -- 
> > Alex Bennée

Re: [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Thu, Nov 03, 2022 at 05:35:57PM -0400, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2022 at 04:30:48PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote:
> > > 
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > 
> > > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> > > >> All the boilerplate virtio code does the same thing (or should at
> > > >> least) of checking to see if the VM is running before attempting to
> > > >> start VirtIO. Push the logic up to the common function to avoid
> > > >> getting a copy and paste wrong.
> > > >> 
> > > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > >
> > > > How bad is it if we drop this?
> > > 
> > > 
> > > I assume it will break gpio. Why do we want to drop this now? It has
> > > been merged awhile. However there was a follow-up patch which tweaked
> > > the order of checks in virtio_device_started.
> > 
> > And that follow up patch trips up UBSAN:
> > 
> > https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327
> > 
> 
> 
> And more specifically this
> 
> https://gitlab.com/mitsirkin/qemu/-/jobs/3269957848

I'm sorry, I misread the results. Triggers without this
patch too so it's not to blame. Pls ignore.


> > 
> > 
> > > >
> > > >> ---
> > > >>  include/hw/virtio/virtio.h   | 5 +++++
> > > >>  hw/virtio/vhost-user-fs.c    | 6 +-----
> > > >>  hw/virtio/vhost-user-i2c.c   | 6 +-----
> > > >>  hw/virtio/vhost-user-rng.c   | 6 +-----
> > > >>  hw/virtio/vhost-user-vsock.c | 6 +-----
> > > >>  hw/virtio/vhost-vsock.c      | 6 +-----
> > > >>  6 files changed, 10 insertions(+), 25 deletions(-)
> > > >> 
> > > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > >> index 9bb2485415..74e7ad5a92 100644
> > > >> --- a/include/hw/virtio/virtio.h
> > > >> +++ b/include/hw/virtio/virtio.h
> > > >> @@ -100,6 +100,7 @@ struct VirtIODevice
> > > >>      VirtQueue *vq;
> > > >>      MemoryListener listener;
> > > >>      uint16_t device_id;
> > > >> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
> > > >>      bool vm_running;
> > > >>      bool broken; /* device in invalid state, needs reset */
> > > >>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> > > >> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> > > >>          return vdev->started;
> > > >>      }
> > > >>  
> > > >> +    if (!vdev->vm_running) {
> > > >> +        return false;
> > > >> +    }
> > > >> +
> > > >>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >>  }
> > > >>  
> > > >> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > >> index e513e4fdda..d2bebba785 100644
> > > >> --- a/hw/virtio/vhost-user-fs.c
> > > >> +++ b/hw/virtio/vhost-user-fs.c
> > > >> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
> > > >>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (fs->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > > >> index 6020eee093..b930cf6d5e 100644
> > > >> --- a/hw/virtio/vhost-user-i2c.c
> > > >> +++ b/hw/virtio/vhost-user-i2c.c
> > > >> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> > > >>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (i2c->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > > >> index 3a7bf8e32d..a9c1c4bc79 100644
> > > >> --- a/hw/virtio/vhost-user-rng.c
> > > >> +++ b/hw/virtio/vhost-user-rng.c
> > > >> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> > > >>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (rng->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> > > >> index 0f8ff99f85..22c1616ebd 100644
> > > >> --- a/hw/virtio/vhost-user-vsock.c
> > > >> +++ b/hw/virtio/vhost-user-vsock.c
> > > >> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
> > > >>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (vvc->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> > > >> index 0338de892f..8031c164a5 100644
> > > >> --- a/hw/virtio/vhost-vsock.c
> > > >> +++ b/hw/virtio/vhost-vsock.c
> > > >> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
> > > >>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>      int ret;
> > > >>  
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> -
> > > >>      if (vvc->vhost_dev.started == should_start) {
> > > >>          return;
> > > >>      }
> > > >> -- 
> > > >> 2.30.2
> > > 
> > > 
> > > -- 
> > > Alex Bennée

Re: [Virtio-fs] [PATCH v4 10/22] hw/virtio: move vm_running check to virtio_device_started

[Michael S. Tsirkin]

On Thu, Nov 03, 2022 at 05:35:57PM -0400, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2022 at 04:30:48PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Nov 03, 2022 at 07:18:30PM +0000, Alex Bennée wrote:
> > > 
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > 
> > > > On Tue, Aug 02, 2022 at 10:49:58AM +0100, Alex Bennée wrote:
> > > >> All the boilerplate virtio code does the same thing (or should at
> > > >> least) of checking to see if the VM is running before attempting to
> > > >> start VirtIO. Push the logic up to the common function to avoid
> > > >> getting a copy and paste wrong.
> > > >> 
> > > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > >
> > > > How bad is it if we drop this?
> > > 
> > > 
> > > I assume it will break gpio. Why do we want to drop this now? It has
> > > been merged awhile. However there was a follow-up patch which tweaked
> > > the order of checks in virtio_device_started.
> > 
> > And that follow up patch trips up UBSAN:
> > 
> > https://gitlab.com/mitsirkin/qemu/-/pipelines/684763327
> > 
> 
> 
> And more specifically this
> 
> https://gitlab.com/mitsirkin/qemu/-/jobs/3269957848

I'm sorry, I misread the results. Triggers without this
patch too so it's not to blame. Pls ignore.


> > 
> > 
> > > >
> > > >> ---
> > > >>  include/hw/virtio/virtio.h   | 5 +++++
> > > >>  hw/virtio/vhost-user-fs.c    | 6 +-----
> > > >>  hw/virtio/vhost-user-i2c.c   | 6 +-----
> > > >>  hw/virtio/vhost-user-rng.c   | 6 +-----
> > > >>  hw/virtio/vhost-user-vsock.c | 6 +-----
> > > >>  hw/virtio/vhost-vsock.c      | 6 +-----
> > > >>  6 files changed, 10 insertions(+), 25 deletions(-)
> > > >> 
> > > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > >> index 9bb2485415..74e7ad5a92 100644
> > > >> --- a/include/hw/virtio/virtio.h
> > > >> +++ b/include/hw/virtio/virtio.h
> > > >> @@ -100,6 +100,7 @@ struct VirtIODevice
> > > >>      VirtQueue *vq;
> > > >>      MemoryListener listener;
> > > >>      uint16_t device_id;
> > > >> +    /* @vm_running: current VM running state via virtio_vmstate_change() */
> > > >>      bool vm_running;
> > > >>      bool broken; /* device in invalid state, needs reset */
> > > >>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
> > > >> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> > > >>          return vdev->started;
> > > >>      }
> > > >>  
> > > >> +    if (!vdev->vm_running) {
> > > >> +        return false;
> > > >> +    }
> > > >> +
> > > >>      return status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >>  }
> > > >>  
> > > >> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > >> index e513e4fdda..d2bebba785 100644
> > > >> --- a/hw/virtio/vhost-user-fs.c
> > > >> +++ b/hw/virtio/vhost-user-fs.c
> > > >> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
> > > >>  static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (fs->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > > >> index 6020eee093..b930cf6d5e 100644
> > > >> --- a/hw/virtio/vhost-user-i2c.c
> > > >> +++ b/hw/virtio/vhost-user-i2c.c
> > > >> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> > > >>  static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (i2c->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > > >> index 3a7bf8e32d..a9c1c4bc79 100644
> > > >> --- a/hw/virtio/vhost-user-rng.c
> > > >> +++ b/hw/virtio/vhost-user-rng.c
> > > >> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> > > >>  static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostUserRNG *rng = VHOST_USER_RNG(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (rng->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> > > >> index 0f8ff99f85..22c1616ebd 100644
> > > >> --- a/hw/virtio/vhost-user-vsock.c
> > > >> +++ b/hw/virtio/vhost-user-vsock.c
> > > >> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
> > > >>  static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> -
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>  
> > > >>      if (vvc->vhost_dev.started == should_start) {
> > > >>          return;
> > > >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> > > >> index 0338de892f..8031c164a5 100644
> > > >> --- a/hw/virtio/vhost-vsock.c
> > > >> +++ b/hw/virtio/vhost-vsock.c
> > > >> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
> > > >>  static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> > > >>  {
> > > >>      VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> > > >> -    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
> > > >> +    bool should_start = virtio_device_started(vdev, status);
> > > >>      int ret;
> > > >>  
> > > >> -    if (!vdev->vm_running) {
> > > >> -        should_start = false;
> > > >> -    }
> > > >> -
> > > >>      if (vvc->vhost_dev.started == should_start) {
> > > >>          return;
> > > >>      }
> > > >> -- 
> > > >> 2.30.2
> > > 
> > > 
> > > -- 
> > > Alex Bennée