[PATCH for 7.2-rc3 v3 0/7] fix vhost-user issues with CI

[PATCH for 7.2-rc3 v3 0/7] fix vhost-user issues with CI

[Alex Bennée]

Hi,

Hopefully this is the final iteration to fix the vhost-user issues
that are currently plaguing the release. I've prevented the circular
closing for the vhost_dev structure by generalising the solution used
by virtio-user-blk which punts the close off to an aio instance.

The memory leak from:

  gpio->vhost_dev.vqs = g_new0(struct vhost_virtqueue, gpio->vhost_dev.nvqs);

still occurs which is because we never call vu_gpio_device_unrealize()
in the test. However its unclear why this is the case. We don't seem
to unrealize the vhost-user-network tests either and clang doesn't
complain about that. However as its not triggered by CI I guess we can
live with it for now.

I've included Stefano's:

  vhost: enable vrings in vhost_dev_start() for vhost-user devices

in this series as it makes sense and improves the vring state errors.
However it's up to you if you want to include it in the eventual PR.

I have had at least one green run on CI now:

  https://gitlab.com/stsquad/qemu/-/pipelines/707015814

Please review.

Alex Bennée (6):
  include/hw: attempt to document VirtIO feature variables
  include/hw: VM state takes precedence in virtio_device_should_start
  tests/qtests: override "force-legacy" for gpio virtio-mmio tests
  hw/virtio: ensure a valid host_feature set for virtio-user-gpio
  hw/virtio: add started_vu status field to vhost-user-gpio
  hw/virtio: generalise CHR_EVENT_CLOSED handling

Stefano Garzarella (1):
  vhost: enable vrings in vhost_dev_start() for vhost-user devices

 include/hw/virtio/vhost-user-gpio.h | 10 ++++
 include/hw/virtio/vhost-user.h      | 18 ++++++++
 include/hw/virtio/vhost.h           | 31 +++++++++++--
 include/hw/virtio/virtio.h          | 42 ++++++++++++++---
 backends/cryptodev-vhost.c          |  4 +-
 backends/vhost-user.c               |  4 +-
 hw/block/vhost-user-blk.c           | 45 +++---------------
 hw/net/vhost_net.c                  |  8 ++--
 hw/scsi/vhost-scsi-common.c         |  4 +-
 hw/virtio/vhost-user-fs.c           |  4 +-
 hw/virtio/vhost-user-gpio.c         | 32 +++++++++----
 hw/virtio/vhost-user-i2c.c          |  4 +-
 hw/virtio/vhost-user-rng.c          |  4 +-
 hw/virtio/vhost-user.c              | 71 +++++++++++++++++++++++++++++
 hw/virtio/vhost-vsock-common.c      |  4 +-
 hw/virtio/vhost.c                   | 44 ++++++++++++++++--
 tests/qtest/libqos/virtio-gpio.c    |  3 +-
 hw/virtio/trace-events              |  4 +-
 18 files changed, 251 insertions(+), 85 deletions(-)

- 
2.34.1

[PATCH v3 1/7] include/hw: attempt to document VirtIO feature variables

[Alex Bennée]

We have a bunch of variables associated with the device and the vhost
backend which are used inconsistently throughout the code base. Lets
start trying to bring some order by agreeing what each variable is
for.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Stefano Garzarella <sgarzare@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@gmail.com>

---
v2
  - dropped DISCUSS and commentary
  - separated protocol section for clarity
  - updated working on vhost->backend_features
  - made clear guest_features was the written state
---
 include/hw/virtio/vhost.h  | 25 ++++++++++++++++++++++---
 include/hw/virtio/virtio.h | 19 ++++++++++++++++++-
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 353252ac3e..eaf628f656 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -88,13 +88,32 @@ struct vhost_dev {
     int vq_index_end;
     /* if non-zero, minimum required value for max_queues */
     int num_queues;
+    /**
+     * vhost feature handling requires matching the feature set
+     * offered by a backend which may be a subset of the total
+     * features eventually offered to the guest.
+     *
+     * @features: available features provided by the backend
+     * @acked_features: final negotiated features with front-end driver
+     *
+     * @backend_features: this is used in a couple of places to either
+     * store VHOST_USER_F_PROTOCOL_FEATURES to apply to
+     * VHOST_USER_SET_FEATURES or VHOST_NET_F_VIRTIO_NET_HDR. Its
+     * future use should be discouraged and the variable retired as
+     * its easy to confuse with the VirtIO backend_features.
+     */
     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 */
+
+    /**
+     * @protocol_features: is the vhost-user only feature set by
+     * VHOST_USER_SET_PROTOCOL_FEATURES. Protocol features are only
+     * negotiated if VHOST_USER_F_PROTOCOL_FEATURES has been offered
+     * by the backend (see @features).
+     */
     uint64_t protocol_features;
+
     uint64_t max_queues;
     uint64_t backend_cap;
     /* @started: is the vhost device started? */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a973811cbf..0f612067f7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -93,6 +93,12 @@ enum virtio_device_endian {
     VIRTIO_DEVICE_ENDIAN_BIG,
 };
 
+/**
+ * struct VirtIODevice - common VirtIO structure
+ * @name: name of the device
+ * @status: VirtIO Device Status field
+ *
+ */
 struct VirtIODevice
 {
     DeviceState parent_obj;
@@ -100,9 +106,20 @@ struct VirtIODevice
     uint8_t status;
     uint8_t isr;
     uint16_t queue_sel;
-    uint64_t guest_features;
+    /**
+     * These fields represent a set of VirtIO features at various
+     * levels of the stack. @host_features indicates the complete
+     * feature set the VirtIO device can offer to the driver.
+     * @guest_features indicates which features the VirtIO driver has
+     * selected by writing to the feature register. Finally
+     * @backend_features represents everything supported by the
+     * backend (e.g. vhost) and could potentially be a subset of the
+     * total feature set offered by QEMU.
+     */
     uint64_t host_features;
+    uint64_t guest_features;
     uint64_t backend_features;
+
     size_t config_len;
     void *config;
     uint16_t config_vector;
-- 
2.34.1

Re: [PATCH v3 1/7] include/hw: attempt to document VirtIO feature variables

[Stefano Garzarella]

On Mon, Nov 28, 2022 at 04:40:59PM +0000, Alex Bennée wrote:
>We have a bunch of variables associated with the device and the vhost
>backend which are used inconsistently throughout the code base. Lets
>start trying to bring some order by agreeing what each variable is
>for.
>
>Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>Cc: Stefano Garzarella <sgarzare@redhat.com>
>Cc: "Michael S. Tsirkin" <mst@redhat.com>
>Cc: Stefan Hajnoczi <stefanha@gmail.com>
>
>---
>v2
>  - dropped DISCUSS and commentary
>  - separated protocol section for clarity
>  - updated working on vhost->backend_features
>  - made clear guest_features was the written state
>---
> include/hw/virtio/vhost.h  | 25 ++++++++++++++++++++++---
> include/hw/virtio/virtio.h | 19 ++++++++++++++++++-
> 2 files changed, 40 insertions(+), 4 deletions(-)

Thanks for this effort to improve our documentation!

Acked-by: Stefano Garzarella <sgarzare@redhat.com>

[PATCH v3 2/7] include/hw: VM state takes precedence in virtio_device_should_start

[Alex Bennée]

The VM status should always preempt the device status for these
checks. This ensures the device is in the correct state when we
suspend the VM prior to migrations. This restores the checks to the
order they where in before the refactoring moved things around.

While we are at it lets improve our documentation of the various
fields involved and document the two functions.

Fixes: 9f6bcfd99f (hw/virtio: move vm_running check to virtio_device_started)
Fixes: 259d69c00b (hw/virtio: introduce virtio_device_should_start)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com>

---
v3
  - rm extra line
  - fix fn name in comment for virtio_device_started()
---
 include/hw/virtio/virtio.h | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 0f612067f7..24561e933a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -133,6 +133,13 @@ struct VirtIODevice
     bool broken; /* device in invalid state, needs reset */
     bool use_disabled_flag; /* allow use of 'disable' flag when needed */
     bool disabled; /* device in temporarily disabled state */
+    /**
+     * @use_started: true if the @started flag should be used to check the
+     * current state of the VirtIO device. Otherwise status bits
+     * should be checked for a current status of the device.
+     * @use_started is only set via QMP and defaults to true for all
+     * modern machines (since 4.1).
+     */
     bool use_started;
     bool started;
     bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
@@ -408,6 +415,16 @@ static inline bool virtio_is_big_endian(VirtIODevice *vdev)
     return false;
 }
 
+/**
+ * virtio_device_started() - check if device started
+ * @vdev - the VirtIO device
+ * @status - the devices status bits
+ *
+ * Check if the device is started. For most modern machines this is
+ * tracked via the @vdev->started field (to support migration),
+ * otherwise we check for the final negotiated status bit that
+ * indicates everything is ready.
+ */
 static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
 {
     if (vdev->use_started) {
@@ -428,15 +445,11 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
  */
 static inline bool virtio_device_should_start(VirtIODevice *vdev, uint8_t status)
 {
-    if (vdev->use_started) {
-        return vdev->started;
-    }
-
     if (!vdev->vm_running) {
         return false;
     }
 
-    return status & VIRTIO_CONFIG_S_DRIVER_OK;
+    return virtio_device_started(vdev, status);
 }
 
 static inline void virtio_set_started(VirtIODevice *vdev, bool started)
-- 
2.34.1

Re: [PATCH v3 2/7] include/hw: VM state takes precedence in virtio_device_should_start

[Michael S. Tsirkin]

On Mon, Nov 28, 2022 at 04:41:00PM +0000, Alex Bennée wrote:
> The VM status should always preempt the device status for these
> checks. This ensures the device is in the correct state when we
> suspend the VM prior to migrations. This restores the checks to the
> order they where in before the refactoring moved things around.
> 
> While we are at it lets improve our documentation of the various
> fields involved and document the two functions.
> 
> Fixes: 9f6bcfd99f (hw/virtio: move vm_running check to virtio_device_started)
> Fixes: 259d69c00b (hw/virtio: introduce virtio_device_should_start)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com>


Doesn't this need to be like the last patch in the series?
Otherwise bisect will break on CI, right?

> ---
> v3
>   - rm extra line
>   - fix fn name in comment for virtio_device_started()
> ---
>  include/hw/virtio/virtio.h | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 0f612067f7..24561e933a 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -133,6 +133,13 @@ struct VirtIODevice
>      bool broken; /* device in invalid state, needs reset */
>      bool use_disabled_flag; /* allow use of 'disable' flag when needed */
>      bool disabled; /* device in temporarily disabled state */
> +    /**
> +     * @use_started: true if the @started flag should be used to check the
> +     * current state of the VirtIO device. Otherwise status bits
> +     * should be checked for a current status of the device.
> +     * @use_started is only set via QMP and defaults to true for all
> +     * modern machines (since 4.1).
> +     */
>      bool use_started;
>      bool started;
>      bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
> @@ -408,6 +415,16 @@ static inline bool virtio_is_big_endian(VirtIODevice *vdev)
>      return false;
>  }
>  
> +/**
> + * virtio_device_started() - check if device started
> + * @vdev - the VirtIO device
> + * @status - the devices status bits
> + *
> + * Check if the device is started. For most modern machines this is
> + * tracked via the @vdev->started field (to support migration),
> + * otherwise we check for the final negotiated status bit that
> + * indicates everything is ready.
> + */
>  static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>  {
>      if (vdev->use_started) {
> @@ -428,15 +445,11 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>   */
>  static inline bool virtio_device_should_start(VirtIODevice *vdev, uint8_t status)
>  {
> -    if (vdev->use_started) {
> -        return vdev->started;
> -    }
> -
>      if (!vdev->vm_running) {
>          return false;
>      }
>  
> -    return status & VIRTIO_CONFIG_S_DRIVER_OK;
> +    return virtio_device_started(vdev, status);
>  }
>  
>  static inline void virtio_set_started(VirtIODevice *vdev, bool started)
> -- 
> 2.34.1

[PATCH v3 3/7] tests/qtests: override "force-legacy" for gpio virtio-mmio tests

[Alex Bennée]

The GPIO device is a VIRTIO_F_VERSION_1 devices but running with a
legacy MMIO interface we miss out that feature bit causing confusion.
For the GPIO test force the mmio bus to support non-legacy so we can
properly test it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1333
---
 tests/qtest/libqos/virtio-gpio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/virtio-gpio.c b/tests/qtest/libqos/virtio-gpio.c
index 762aa6695b..f22d7b5eb5 100644
--- a/tests/qtest/libqos/virtio-gpio.c
+++ b/tests/qtest/libqos/virtio-gpio.c
@@ -154,7 +154,8 @@ static void virtio_gpio_register_nodes(void)
     QOSGraphEdgeOptions edge_opts = { };
 
     /* vhost-user-gpio-device */
-    edge_opts.extra_device_opts = "id=gpio0,chardev=chr-vhost-user-test";
+    edge_opts.extra_device_opts = "id=gpio0,chardev=chr-vhost-user-test "
+        "-global virtio-mmio.force-legacy=false";
     qos_node_create_driver("vhost-user-gpio-device",
                             virtio_gpio_device_create);
     qos_node_consumes("vhost-user-gpio-device", "virtio-bus", &edge_opts);
-- 
2.34.1

[PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Alex Bennée]

There was a disconnect here because vdev->host_features was set to
random rubbish. This caused a weird negotiation between the driver and
device that took no account of the features provided by the backend.
To fix this we must set vdev->host_features once we have initialised
the vhost backend.

[AJB: however this is confusing because AFAICT none of the other
vhost-user devices do this.]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/virtio/vhost-user-gpio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 5851cb3bc9..b2496c824c 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
         return ret;
     }
 
+    /*
+     * Once we have initialised the vhost backend we can finally set
+     * the what host features are available for this device.
+     */
+    vdev->host_features = vhost_dev->features;
+
     /* restore vhost state */
     if (virtio_device_started(vdev, vdev->status)) {
         vu_gpio_start(vdev);
-- 
2.34.1

Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Stefan Hajnoczi]

On Mon, 28 Nov 2022 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> There was a disconnect here because vdev->host_features was set to
> random rubbish. This caused a weird negotiation between the driver and
> device that took no account of the features provided by the backend.
> To fix this we must set vdev->host_features once we have initialised
> the vhost backend.
>
> [AJB: however this is confusing because AFAICT none of the other
> vhost-user devices do this.]
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/virtio/vhost-user-gpio.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 5851cb3bc9..b2496c824c 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>          return ret;
>      }
>
> +    /*
> +     * Once we have initialised the vhost backend we can finally set
> +     * the what host features are available for this device.
> +     */
> +    vdev->host_features = vhost_dev->features;

vdev->host_feature is already set by virtio_bus_device_plugged ->
vu_gpio_get_features.

Something is still wrong.

My understanding is: ->realize() performs a blocking connect so when
it returns and virtio_bus_device_plugged() runs, we'll be able to
fetch the backend features from ->get_features(). The assumption is
that the backend features don't change across reconnection (I think).

vhost-user-gpio seems to follow the same flow as the other vhost-user
devices (vhost-user-net is special, so let's ignore it), so I don't
understand why it's necessary to manually assign ->host_features here?

Stefan

Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Alex Bennée]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, 28 Nov 2022 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> There was a disconnect here because vdev->host_features was set to
>> random rubbish. This caused a weird negotiation between the driver and
>> device that took no account of the features provided by the backend.
>> To fix this we must set vdev->host_features once we have initialised
>> the vhost backend.
>>
>> [AJB: however this is confusing because AFAICT none of the other
>> vhost-user devices do this.]
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/virtio/vhost-user-gpio.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>> index 5851cb3bc9..b2496c824c 100644
>> --- a/hw/virtio/vhost-user-gpio.c
>> +++ b/hw/virtio/vhost-user-gpio.c
>> @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>>          return ret;
>>      }
>>
>> +    /*
>> +     * Once we have initialised the vhost backend we can finally set
>> +     * the what host features are available for this device.
>> +     */
>> +    vdev->host_features = vhost_dev->features;
>
> vdev->host_feature is already set by virtio_bus_device_plugged ->
> vu_gpio_get_features.
>
> Something is still wrong.
>
> My understanding is: ->realize() performs a blocking connect so when
> it returns and virtio_bus_device_plugged() runs, we'll be able to
> fetch the backend features from ->get_features(). The assumption is
> that the backend features don't change across reconnection (I think).
>
> vhost-user-gpio seems to follow the same flow as the other vhost-user
> devices (vhost-user-net is special, so let's ignore it), so I don't
> understand why it's necessary to manually assign ->host_features here?

I think you are right. I suspect I got confused while chasing down the
missing high bits (due to the legacy VirtIO negotiation). Also slightly
thrown off by the appearance of virtio-net (I assume because of a
machine default?):

  ➜  env QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-aarch64 G_TEST_DBUS_DAEMON=/home/alex/
  lsrc/qemu.git/tests/dbus-vmstate-daemon.sh MALLOC_PERTURB_=177 ./tests/qtest/qos-test --tap -p /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
  # random seed: R02S235ee9d31e87e0159f810979be62563e
  # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-1276289.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276289.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest
  # Start of aarch64 tests
  # Start of virt tests
  # Start of generic-pcihost tests
  # Start of pci-bus-generic tests
  # Start of pci-bus tests
  # Start of vhost-user-gpio-pci tests
  # Start of vhost-user-gpio tests
  # Start of vhost-user-gpio-tests tests
  # Start of read-guest-mem tests
  virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064
  vu_gpio_connect: pre host_features = 10039000000
  vu_gpio_connect: post host_features = 140000001
  virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001
  qemu-system-aarch64: Failed to set msg fds.
  qemu-system-aarch64: vhost VQ 0 ring restore failed: -22: Invalid argument (22)
  qemu-system-aarch64: Failed to set msg fds.
  qemu-system-aarch64: vhost VQ 1 ring restore failed: -22: Invalid argument (22)
  qemu-system-aarch64: Failed to set msg fds.
  qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22)
  qemu-system-aarch64: Failed to set msg fds.
  qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22)
  # qos_test running single test in subprocess
  # set_protocol_features: 0x200
  # set_owner: start of session
  # vhost-user: un-handled message: 14
  # vhost-user: un-handled message: 14
  # set_vring_num: 0/256
  # set_vring_addr: 0x7f55b0000000/0x7f55affff000/0x7f55b0001000
  ok 1 /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
  # Start of memfile tests
  # End of memfile tests
  # End of read-guest-mem tests
  # End of vhost-user-gpio-tests tests
  # End of vhost-user-gpio tests
  # End of vhost-user-gpio-pci tests
  # End of pci-bus tests
  # End of pci-bus-generic tests
  # End of generic-pcihost tests
  # End of virt tests
  # End of aarch64 tests
  1..1

and for mmio:

  env MALLOC_PERTURB_=235 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY="./qemu-system-arm" QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh ./tests/qtest/qos-test --tap -p /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
  # random seed: R02Sac48bb073367f77c1c1a1db6b5245c9e
  # starting QEMU: exec ./qemu-system-arm -qtest unix:/tmp/qtest-1276963.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276963.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest
  # Start of arm tests
  # Start of virt tests
  # Start of virtio-mmio tests
  # Start of virtio-bus tests
  # Start of vhost-user-gpio-device tests
  # Start of vhost-user-gpio tests
  # Start of vhost-user-gpio-tests tests
  # Start of read-guest-mem tests
  virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064
  vu_gpio_connect: pre host_features = 10039000000
  vu_gpio_connect: post host_features = 140000001
  virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001
  qemu-system-arm: Failed to set msg fds.
  qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22)
  qemu-system-arm: Failed to set msg fds.
  qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22)
  # qos_test running single test in subprocess
  # set_protocol_features: 0x200
  # set_owner: start of session
  # vhost-user: un-handled message: 14
  # vhost-user: un-handled message: 14
  ok 1 /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
  # Start of memfile tests
  # End of memfile tests
  # End of read-guest-mem tests
  # End of vhost-user-gpio-tests tests
  # End of vhost-user-gpio tests
  # End of vhost-user-gpio-device tests
  # End of virtio-bus tests
  # End of virtio-mmio tests
  # End of virt tests
  # End of arm tests
  1..1

I'll drop this patch for now and re-test.

>
> Stefan


-- 
Alex Bennée

Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Michael S. Tsirkin]

On Mon, Nov 28, 2022 at 07:39:29PM +0000, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Mon, 28 Nov 2022 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> There was a disconnect here because vdev->host_features was set to
> >> random rubbish. This caused a weird negotiation between the driver and
> >> device that took no account of the features provided by the backend.
> >> To fix this we must set vdev->host_features once we have initialised
> >> the vhost backend.
> >>
> >> [AJB: however this is confusing because AFAICT none of the other
> >> vhost-user devices do this.]
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  hw/virtio/vhost-user-gpio.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> >> index 5851cb3bc9..b2496c824c 100644
> >> --- a/hw/virtio/vhost-user-gpio.c
> >> +++ b/hw/virtio/vhost-user-gpio.c
> >> @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
> >>          return ret;
> >>      }
> >>
> >> +    /*
> >> +     * Once we have initialised the vhost backend we can finally set
> >> +     * the what host features are available for this device.
> >> +     */
> >> +    vdev->host_features = vhost_dev->features;
> >
> > vdev->host_feature is already set by virtio_bus_device_plugged ->
> > vu_gpio_get_features.
> >
> > Something is still wrong.
> >
> > My understanding is: ->realize() performs a blocking connect so when
> > it returns and virtio_bus_device_plugged() runs, we'll be able to
> > fetch the backend features from ->get_features(). The assumption is
> > that the backend features don't change across reconnection (I think).
> >
> > vhost-user-gpio seems to follow the same flow as the other vhost-user
> > devices (vhost-user-net is special, so let's ignore it), so I don't
> > understand why it's necessary to manually assign ->host_features here?
> 
> I think you are right. I suspect I got confused while chasing down the
> missing high bits (due to the legacy VirtIO negotiation). Also slightly
> thrown off by the appearance of virtio-net (I assume because of a
> machine default?):
> 
>   ➜  env QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-aarch64 G_TEST_DBUS_DAEMON=/home/alex/
>   lsrc/qemu.git/tests/dbus-vmstate-daemon.sh MALLOC_PERTURB_=177 ./tests/qtest/qos-test --tap -p /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
>   # random seed: R02S235ee9d31e87e0159f810979be62563e
>   # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-1276289.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276289.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest
>   # Start of aarch64 tests
>   # Start of virt tests
>   # Start of generic-pcihost tests
>   # Start of pci-bus-generic tests
>   # Start of pci-bus tests
>   # Start of vhost-user-gpio-pci tests
>   # Start of vhost-user-gpio tests
>   # Start of vhost-user-gpio-tests tests
>   # Start of read-guest-mem tests
>   virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064
>   vu_gpio_connect: pre host_features = 10039000000
>   vu_gpio_connect: post host_features = 140000001
>   virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001
>   qemu-system-aarch64: Failed to set msg fds.
>   qemu-system-aarch64: vhost VQ 0 ring restore failed: -22: Invalid argument (22)
>   qemu-system-aarch64: Failed to set msg fds.
>   qemu-system-aarch64: vhost VQ 1 ring restore failed: -22: Invalid argument (22)
>   qemu-system-aarch64: Failed to set msg fds.
>   qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22)
>   qemu-system-aarch64: Failed to set msg fds.
>   qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22)
>   # qos_test running single test in subprocess
>   # set_protocol_features: 0x200
>   # set_owner: start of session
>   # vhost-user: un-handled message: 14
>   # vhost-user: un-handled message: 14
>   # set_vring_num: 0/256
>   # set_vring_addr: 0x7f55b0000000/0x7f55affff000/0x7f55b0001000
>   ok 1 /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
>   # Start of memfile tests
>   # End of memfile tests
>   # End of read-guest-mem tests
>   # End of vhost-user-gpio-tests tests
>   # End of vhost-user-gpio tests
>   # End of vhost-user-gpio-pci tests
>   # End of pci-bus tests
>   # End of pci-bus-generic tests
>   # End of generic-pcihost tests
>   # End of virt tests
>   # End of aarch64 tests
>   1..1
> 
> and for mmio:
> 
>   env MALLOC_PERTURB_=235 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY="./qemu-system-arm" QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh ./tests/qtest/qos-test --tap -p /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
>   # random seed: R02Sac48bb073367f77c1c1a1db6b5245c9e
>   # starting QEMU: exec ./qemu-system-arm -qtest unix:/tmp/qtest-1276963.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276963.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest
>   # Start of arm tests
>   # Start of virt tests
>   # Start of virtio-mmio tests
>   # Start of virtio-bus tests
>   # Start of vhost-user-gpio-device tests
>   # Start of vhost-user-gpio tests
>   # Start of vhost-user-gpio-tests tests
>   # Start of read-guest-mem tests
>   virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064
>   vu_gpio_connect: pre host_features = 10039000000
>   vu_gpio_connect: post host_features = 140000001
>   virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001
>   qemu-system-arm: Failed to set msg fds.
>   qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22)
>   qemu-system-arm: Failed to set msg fds.
>   qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22)
>   # qos_test running single test in subprocess
>   # set_protocol_features: 0x200
>   # set_owner: start of session
>   # vhost-user: un-handled message: 14
>   # vhost-user: un-handled message: 14
>   ok 1 /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile
>   # Start of memfile tests
>   # End of memfile tests
>   # End of read-guest-mem tests
>   # End of vhost-user-gpio-tests tests
>   # End of vhost-user-gpio tests
>   # End of vhost-user-gpio-device tests
>   # End of virtio-bus tests
>   # End of virtio-mmio tests
>   # End of virt tests
>   # End of arm tests
>   1..1
> 
> I'll drop this patch for now and re-test.

So I should expect v4? And I guess we can split out documentation things then?

> >
> > Stefan
> 
> 
> -- 
> Alex Bennée

Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Stefan Hajnoczi]

Hi Alex,
I'm waiting for a v4 or a confirmation that you've retested and I can
just drop this patch.

Thanks!

Stefan

Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Michael S. Tsirkin]

On Tue, Nov 29, 2022 at 04:01:25PM -0500, Stefan Hajnoczi wrote:
> Hi Alex,
> I'm waiting for a v4 or a confirmation that you've retested and I can
> just drop this patch.
> 
> Thanks!
> 
> Stefan

Note things need to be reordered, patch 2 should come last.
So I'd really like to see v4 if possible.

-- 
MST

Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Alex Bennée]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> Hi Alex,
> I'm waiting for a v4 or a confirmation that you've retested and I can
> just drop this patch.

I've re-ordered and I'll post the up to date series with the dropped
patch tomorrow. I was hoping for r-b's for the other patches.

>
> Thanks!
>
> Stefan


-- 
Alex Bennée

[PATCH v3 5/7] vhost: enable vrings in vhost_dev_start() for vhost-user devices

[Alex Bennée]

From: Stefano Garzarella <sgarzare@redhat.com>

Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
backend, but we forgot to enable vrings as specified in
docs/interop/vhost-user.rst:

    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
    ring starts directly in the enabled state.

    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
    initialized in a disabled state and is enabled by
    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.

Some vhost-user front-ends already did this by calling
vhost_ops.vhost_set_vring_enable() directly:
- backends/cryptodev-vhost.c
- hw/net/virtio-net.c
- hw/virtio/vhost-user-gpio.c

But most didn't do that, so we would leave the vrings disabled and some
backends would not work. We observed this issue with the rust version of
virtiofsd [1], which uses the event loop [2] provided by the
vhost-user-backend crate where requests are not processed if vring is
not enabled.

Let's fix this issue by enabling the vrings in vhost_dev_start() for
vhost-user front-ends that don't already do this directly. Same thing
also in vhost_dev_stop() where we disable vrings.

[1] https://gitlab.com/virtio-fs/virtiofsd
[2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217

Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
Reported-by: German Maglione <gmaglione@redhat.com>
Tested-by: German Maglione <gmaglione@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Message-Id: <20221123131630.52020-1-sgarzare@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost.h      |  6 +++--
 backends/cryptodev-vhost.c     |  4 ++--
 backends/vhost-user.c          |  4 ++--
 hw/block/vhost-user-blk.c      |  4 ++--
 hw/net/vhost_net.c             |  8 +++----
 hw/scsi/vhost-scsi-common.c    |  4 ++--
 hw/virtio/vhost-user-fs.c      |  4 ++--
 hw/virtio/vhost-user-gpio.c    |  4 ++--
 hw/virtio/vhost-user-i2c.c     |  4 ++--
 hw/virtio/vhost-user-rng.c     |  4 ++--
 hw/virtio/vhost-vsock-common.c |  4 ++--
 hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
 hw/virtio/trace-events         |  4 ++--
 13 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index eaf628f656..1cafa0d776 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -203,24 +203,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
  * vhost_dev_start() - start the vhost device
  * @hdev: common vhost_dev structure
  * @vdev: the VirtIODevice structure
+ * @vrings: true to have vrings enabled in this call
  *
  * Starts the vhost device. From this point VirtIO feature negotiation
  * can start and the device can start processing VirtIO transactions.
  *
  * Return: 0 on success, < 0 on error.
  */
-int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
+int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
 
 /**
  * vhost_dev_stop() - stop the vhost device
  * @hdev: common vhost_dev structure
  * @vdev: the VirtIODevice structure
+ * @vrings: true to have vrings disabled in this call
  *
  * Stop the vhost device. After the device is stopped the notifiers
  * can be disabled (@vhost_dev_disable_notifiers) and the device can
  * be torn down (@vhost_dev_cleanup).
  */
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
+void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
 
 /**
  * DOC: vhost device configuration handling
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index bc13e466b4..572f87b3be 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
         goto fail_notifiers;
     }
 
-    r = vhost_dev_start(&crypto->dev, dev);
+    r = vhost_dev_start(&crypto->dev, dev, false);
     if (r < 0) {
         goto fail_start;
     }
@@ -111,7 +111,7 @@ static void
 cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
                                  VirtIODevice *dev)
 {
-    vhost_dev_stop(&crypto->dev, dev);
+    vhost_dev_stop(&crypto->dev, dev, false);
     vhost_dev_disable_notifiers(&crypto->dev, dev);
 }
 
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 5dedb2d987..7bfcaef976 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
     }
 
     b->dev.acked_features = b->vdev->guest_features;
-    ret = vhost_dev_start(&b->dev, b->vdev);
+    ret = vhost_dev_start(&b->dev, b->vdev, true);
     if (ret < 0) {
         error_report("Error start vhost dev");
         goto err_guest_notifiers;
@@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
         return;
     }
 
-    vhost_dev_stop(&b->dev, b->vdev);
+    vhost_dev_stop(&b->dev, b->vdev, true);
 
     if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent,
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 0d5190accf..1177064631 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
     }
 
     s->dev.vq_index_end = s->dev.nvqs;
-    ret = vhost_dev_start(&s->dev, vdev);
+    ret = vhost_dev_start(&s->dev, vdev, true);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Error starting vhost");
         goto err_guest_notifiers;
@@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&s->dev, vdev);
+    vhost_dev_stop(&s->dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 26e4930676..043058ff43 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
         goto fail_notifiers;
     }
 
-    r = vhost_dev_start(&net->dev, dev);
+    r = vhost_dev_start(&net->dev, dev, false);
     if (r < 0) {
         goto fail_start;
     }
@@ -308,7 +308,7 @@ fail:
     if (net->nc->info->poll) {
         net->nc->info->poll(net->nc, true);
     }
-    vhost_dev_stop(&net->dev, dev);
+    vhost_dev_stop(&net->dev, dev, false);
 fail_start:
     vhost_dev_disable_notifiers(&net->dev, dev);
 fail_notifiers:
@@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
     if (net->nc->info->poll) {
         net->nc->info->poll(net->nc, true);
     }
-    vhost_dev_stop(&net->dev, dev);
+    vhost_dev_stop(&net->dev, dev, false);
     if (net->nc->info->stop) {
         net->nc->info->stop(net->nc);
     }
@@ -606,7 +606,7 @@ err_start:
         assert(r >= 0);
     }
 
-    vhost_dev_stop(&net->dev, vdev);
+    vhost_dev_stop(&net->dev, vdev, false);
 
     return r;
 }
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index 767f827e55..18ea5dcfa1 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
         goto err_guest_notifiers;
     }
 
-    ret = vhost_dev_start(&vsc->dev, vdev);
+    ret = vhost_dev_start(&vsc->dev, vdev, true);
     if (ret < 0) {
         error_report("Error start vhost dev");
         goto err_guest_notifiers;
@@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     int ret = 0;
 
-    vhost_dev_stop(&vsc->dev, vdev);
+    vhost_dev_stop(&vsc->dev, vdev, true);
 
     if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index dc4014cdef..d97b179e6f 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
     }
 
     fs->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&fs->vhost_dev, vdev);
+    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost: %d", -ret);
         goto err_guest_notifiers;
@@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&fs->vhost_dev, vdev);
+    vhost_dev_stop(&fs->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index b2496c824c..b38e4d4cf0 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
      */
     vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
 
-    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
+    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
     if (ret < 0) {
         error_report("Error starting vhost-user-gpio: %d", ret);
         goto err_guest_notifiers;
@@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(vhost_dev, vdev);
+    vhost_dev_stop(vhost_dev, vdev, false);
 
     ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 1c9f3d20dc..dc5c828ba6 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
 
     i2c->vhost_dev.acked_features = vdev->guest_features;
 
-    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
+    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost-user-i2c: %d", -ret);
         goto err_guest_notifiers;
@@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&i2c->vhost_dev, vdev);
+    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index f9084cde58..201a39e220 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
     }
 
     rng->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&rng->vhost_dev, vdev);
+    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost-user-rng: %d", -ret);
         goto err_guest_notifiers;
@@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&rng->vhost_dev, vdev);
+    vhost_dev_stop(&rng->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index a67a275de2..d21c72b401 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
     }
 
     vvc->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
+    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost: %d", -ret);
         goto err_guest_notifiers;
@@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&vvc->vhost_dev, vdev);
+    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d1c4c20b8c..7fb008bc9e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
     return 0;
 }
 
+static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
+{
+    if (!hdev->vhost_ops->vhost_set_vring_enable) {
+        return 0;
+    }
+
+    /*
+     * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
+     * been negotiated, the rings start directly in the enabled state, and
+     * .vhost_set_vring_enable callback will fail since
+     * VHOST_USER_SET_VRING_ENABLE is not supported.
+     */
+    if (hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER &&
+        !virtio_has_feature(hdev->backend_features,
+                            VHOST_USER_F_PROTOCOL_FEATURES)) {
+        return 0;
+    }
+
+    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
+}
+
 /* Host notifiers must be enabled at this point. */
-int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
+int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
 {
     int i, r;
 
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    trace_vhost_dev_start(hdev, vdev->name);
+    trace_vhost_dev_start(hdev, vdev->name, vrings);
 
     vdev->vhost_started = true;
     hdev->started = true;
@@ -1830,10 +1851,16 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
             goto fail_log;
         }
     }
+    if (vrings) {
+        r = vhost_dev_set_vring_enable(hdev, true);
+        if (r) {
+            goto fail_log;
+        }
+    }
     if (hdev->vhost_ops->vhost_dev_start) {
         r = hdev->vhost_ops->vhost_dev_start(hdev, true);
         if (r) {
-            goto fail_log;
+            goto fail_start;
         }
     }
     if (vhost_dev_has_iommu(hdev) &&
@@ -1848,6 +1875,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
     }
     return 0;
+fail_start:
+    if (vrings) {
+        vhost_dev_set_vring_enable(hdev, false);
+    }
 fail_log:
     vhost_log_put(hdev, false);
 fail_vq:
@@ -1866,18 +1897,21 @@ fail_features:
 }
 
 /* Host notifiers must be enabled at this point. */
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
+void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
 {
     int i;
 
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    trace_vhost_dev_stop(hdev, vdev->name);
+    trace_vhost_dev_stop(hdev, vdev->name, vrings);
 
     if (hdev->vhost_ops->vhost_dev_start) {
         hdev->vhost_ops->vhost_dev_start(hdev, false);
     }
+    if (vrings) {
+        vhost_dev_set_vring_enable(hdev, false);
+    }
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_stop(hdev,
                              vdev,
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 820dadc26c..14fc5b9bb2 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -9,8 +9,8 @@ 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_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
+vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
 
 
 # vhost-user.c
-- 
2.34.1

[PATCH v3 6/7] hw/virtio: add started_vu status field to vhost-user-gpio

[Alex Bennée]

As per the fix to vhost-user-blk in f5b22d06fb (vhost: recheck dev
state in the vhost_migration_log routine) we really should track the
connection and starting separately.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost-user-gpio.h | 10 ++++++++++
 hw/virtio/vhost-user-gpio.c         | 11 ++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/hw/virtio/vhost-user-gpio.h b/include/hw/virtio/vhost-user-gpio.h
index 4fe9aeecc0..a9305c5e6c 100644
--- a/include/hw/virtio/vhost-user-gpio.h
+++ b/include/hw/virtio/vhost-user-gpio.h
@@ -28,7 +28,17 @@ struct VHostUserGPIO {
     VhostUserState vhost_user;
     VirtQueue *command_vq;
     VirtQueue *interrupt_vq;
+    /**
+     * There are at least two steps of initialization of the
+     * vhost-user device. The first is a "connect" step and
+     * second is a "start" step. Make a separation between
+     * those initialization phases by using two fields.
+     *
+     * @connected: see vu_gpio_connect()/vu_gpio_disconnect()
+     * @started_vu: see vu_gpio_start()/vu_gpio_stop()
+     */
     bool connected;
+    bool started_vu;
     /*< public >*/
 };
 
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index b38e4d4cf0..75e28bcd3b 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -86,6 +86,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
         error_report("Error starting vhost-user-gpio: %d", ret);
         goto err_guest_notifiers;
     }
+    gpio->started_vu = true;
 
     /*
      * guest_notifier_mask/pending not used yet, so just unmask
@@ -126,16 +127,12 @@ static void vu_gpio_stop(VirtIODevice *vdev)
     struct vhost_dev *vhost_dev = &gpio->vhost_dev;
     int ret;
 
-    if (!k->set_guest_notifiers) {
+    if (!gpio->started_vu) {
         return;
     }
+    gpio->started_vu = false;
 
-    /*
-     * 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)) {
+    if (!k->set_guest_notifiers) {
         return;
     }
 
-- 
2.34.1

[PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Alex Bennée]

..and use for both virtio-user-blk and virtio-user-gpio. This avoids
the circular close by deferring shutdown due to disconnection until a
later point. virtio-user-blk already had this mechanism in place so
generalise it as a vhost-user helper function and use for both blk and
gpio devices.

While we are at it we also fix up vhost-user-gpio to re-establish the
event handler after close down so we can reconnect later.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost-user.h | 18 +++++++++
 hw/block/vhost-user-blk.c      | 41 +++-----------------
 hw/virtio/vhost-user-gpio.c    | 11 +++++-
 hw/virtio/vhost-user.c         | 71 ++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 37 deletions(-)

diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index c6e693cd3f..191216a74f 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
  */
 void vhost_user_cleanup(VhostUserState *user);
 
+/**
+ * vhost_user_async_close() - cleanup vhost-user post connection drop
+ * @d: DeviceState for the associated device (passed to callback)
+ * @chardev: the CharBackend associated with the connection
+ * @vhost: the common vhost device
+ * @cb: the user callback function to complete the clean-up
+ *
+ * This function is used to handle the shutdown of a vhost-user
+ * connection to a backend. We handle this centrally to make sure we
+ * do all the steps and handle potential races due to VM shutdowns.
+ * Once the connection is disabled we call a backhalf to ensure
+ */
+typedef void (*vu_async_close_fn)(DeviceState *cb);
+
+void vhost_user_async_close(DeviceState *d,
+                            CharBackend *chardev, struct vhost_dev *vhost,
+                            vu_async_close_fn cb);
+
 #endif
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1177064631..aff4d2b8cb 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
     vhost_user_blk_stop(vdev);
 
     vhost_dev_cleanup(&s->dev);
-}
 
-static void vhost_user_blk_chr_closed_bh(void *opaque)
-{
-    DeviceState *dev = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserBlk *s = VHOST_USER_BLK(vdev);
-
-    vhost_user_blk_disconnect(dev);
+    /* Re-instate the event handler for new connections */
     qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
-                             NULL, opaque, NULL, true);
+                             NULL, dev, NULL, true);
 }
 
 static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
@@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
         }
         break;
     case CHR_EVENT_CLOSED:
-        if (!runstate_check(RUN_STATE_SHUTDOWN)) {
-            /*
-             * A close event may happen during a read/write, but vhost
-             * code assumes the vhost_dev remains setup, so delay the
-             * stop & clear.
-             */
-            AioContext *ctx = qemu_get_current_aio_context();
-
-            qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
-                    NULL, NULL, false);
-            aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
-
-            /*
-             * Move vhost device to the stopped state. The vhost-user device
-             * will be clean up and disconnected in BH. This can be useful in
-             * 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;
-        }
+        /* defer close until later to avoid circular close */
+        vhost_user_async_close(dev, &s->chardev, &s->dev,
+                               vhost_user_blk_disconnect);
         break;
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 75e28bcd3b..cd76287766 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
     return 0;
 }
 
+static void vu_gpio_event(void *opaque, QEMUChrEvent event);
+
 static void vu_gpio_disconnect(DeviceState *dev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
 
     vu_gpio_stop(vdev);
     vhost_dev_cleanup(&gpio->vhost_dev);
+
+    /* Re-instate the event handler for new connections */
+    qemu_chr_fe_set_handlers(&gpio->chardev,
+                             NULL, NULL, vu_gpio_event,
+                             NULL, dev, NULL, true);
 }
 
 static void vu_gpio_event(void *opaque, QEMUChrEvent event)
@@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
         }
         break;
     case CHR_EVENT_CLOSED:
-        vu_gpio_disconnect(dev);
+        /* defer close until later to avoid circular close */
+        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
+                               vu_gpio_disconnect);
         break;
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index abe23d4ebe..8f635844af 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -21,6 +21,7 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
+#include "sysemu/runstate.h"
 #include "sysemu/cryptodev.h"
 #include "migration/migration.h"
 #include "migration/postcopy-ram.h"
@@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
     user->chr = NULL;
 }
 
+
+typedef struct {
+    vu_async_close_fn cb;
+    DeviceState *dev;
+    CharBackend *cd;
+    struct vhost_dev *vhost;
+} VhostAsyncCallback;
+
+static void vhost_user_async_close_bh(void *opaque)
+{
+    VhostAsyncCallback *data = opaque;
+    struct vhost_dev *vhost = data->vhost;
+
+    /*
+     * If the vhost_dev has been cleared in the meantime there is
+     * nothing left to do as some other path has completed the
+     * cleanup.
+     */
+    if (vhost->vdev) {
+        data->cb(data->dev);
+    }
+
+    g_free(data);
+}
+
+/*
+ * We only schedule the work if the machine is running. If suspended
+ * we want to keep all the in-flight data as is for migration
+ * purposes.
+ */
+void vhost_user_async_close(DeviceState *d,
+                            CharBackend *chardev, struct vhost_dev *vhost,
+                            vu_async_close_fn cb)
+{
+    if (!runstate_check(RUN_STATE_SHUTDOWN)) {
+        /*
+         * A close event may happen during a read/write, but vhost
+         * code assumes the vhost_dev remains setup, so delay the
+         * stop & clear.
+         */
+        AioContext *ctx = qemu_get_current_aio_context();
+        VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
+
+        /* Save data for the callback */
+        data->cb = cb;
+        data->dev = d;
+        data->cd = chardev;
+        data->vhost = vhost;
+
+        /* Disable any further notifications on the chardev */
+        qemu_chr_fe_set_handlers(chardev,
+                                 NULL, NULL, NULL, NULL, NULL, NULL,
+                                 false);
+
+        aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
+
+        /*
+         * Move vhost device to the stopped state. The vhost-user device
+         * will be clean up and disconnected in BH. This can be useful in
+         * 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).
+         *
+         * Note if the vhost device is fully cleared by the time we
+         * execute the bottom half we won't continue with the cleanup.
+         */
+        vhost->started = false;
+    }
+}
+
 static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
 {
     if (!virtio_has_feature(dev->protocol_features,
-- 
2.34.1

Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Raphael Norwitz]

> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
> the circular close by deferring shutdown due to disconnection until a
> later point. virtio-user-blk already had this mechanism in place so

The mechanism was originally copied from virtio-net so we should probably fix it there too. AFAICT calling vhost_user_async_close() should work in net_vhost_user_event().

Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.


> generalise it as a vhost-user helper function and use for both blk and
> gpio devices.
> 
> While we are at it we also fix up vhost-user-gpio to re-establish the
> event handler after close down so we can reconnect later.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/hw/virtio/vhost-user.h | 18 +++++++++
> hw/block/vhost-user-blk.c      | 41 +++-----------------
> hw/virtio/vhost-user-gpio.c    | 11 +++++-
> hw/virtio/vhost-user.c         | 71 ++++++++++++++++++++++++++++++++++
> 4 files changed, 104 insertions(+), 37 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index c6e693cd3f..191216a74f 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
>  */
> void vhost_user_cleanup(VhostUserState *user);
> 
> +/**
> + * vhost_user_async_close() - cleanup vhost-user post connection drop
> + * @d: DeviceState for the associated device (passed to callback)
> + * @chardev: the CharBackend associated with the connection
> + * @vhost: the common vhost device
> + * @cb: the user callback function to complete the clean-up
> + *
> + * This function is used to handle the shutdown of a vhost-user
> + * connection to a backend. We handle this centrally to make sure we
> + * do all the steps and handle potential races due to VM shutdowns.
> + * Once the connection is disabled we call a backhalf to ensure
> + */
> +typedef void (*vu_async_close_fn)(DeviceState *cb);
> +
> +void vhost_user_async_close(DeviceState *d,
> +                            CharBackend *chardev, struct vhost_dev *vhost,
> +                            vu_async_close_fn cb);
> +
> #endif
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 1177064631..aff4d2b8cb 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>     vhost_user_blk_stop(vdev);
> 
>     vhost_dev_cleanup(&s->dev);
> -}
> 
> -static void vhost_user_blk_chr_closed_bh(void *opaque)
> -{
> -    DeviceState *dev = opaque;
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -
> -    vhost_user_blk_disconnect(dev);
> +    /* Re-instate the event handler for new connections */
>     qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> -                             NULL, opaque, NULL, true);
> +                             NULL, dev, NULL, true);
> }
> 
> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>         }
>         break;
>     case CHR_EVENT_CLOSED:
> -        if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> -            /*
> -             * A close event may happen during a read/write, but vhost
> -             * code assumes the vhost_dev remains setup, so delay the
> -             * stop & clear.
> -             */
> -            AioContext *ctx = qemu_get_current_aio_context();
> -
> -            qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> -                    NULL, NULL, false);
> -            aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> -
> -            /*
> -             * Move vhost device to the stopped state. The vhost-user device
> -             * will be clean up and disconnected in BH. This can be useful in
> -             * 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;
> -        }
> +        /* defer close until later to avoid circular close */
> +        vhost_user_async_close(dev, &s->chardev, &s->dev,
> +                               vhost_user_blk_disconnect);
>         break;
>     case CHR_EVENT_BREAK:
>     case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 75e28bcd3b..cd76287766 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>     return 0;
> }
> 
> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
> +
> static void vu_gpio_disconnect(DeviceState *dev)
> {
>     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
> 
>     vu_gpio_stop(vdev);
>     vhost_dev_cleanup(&gpio->vhost_dev);
> +
> +    /* Re-instate the event handler for new connections */
> +    qemu_chr_fe_set_handlers(&gpio->chardev,
> +                             NULL, NULL, vu_gpio_event,
> +                             NULL, dev, NULL, true);
> }
> 
> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>         }
>         break;
>     case CHR_EVENT_CLOSED:
> -        vu_gpio_disconnect(dev);
> +        /* defer close until later to avoid circular close */
> +        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> +                               vu_gpio_disconnect);
>         break;
>     case CHR_EVENT_BREAK:
>     case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index abe23d4ebe..8f635844af 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -21,6 +21,7 @@
> #include "qemu/error-report.h"
> #include "qemu/main-loop.h"
> #include "qemu/sockets.h"
> +#include "sysemu/runstate.h"
> #include "sysemu/cryptodev.h"
> #include "migration/migration.h"
> #include "migration/postcopy-ram.h"
> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
>     user->chr = NULL;
> }
> 

nit: Extra space 

> +
> +typedef struct {
> +    vu_async_close_fn cb;
> +    DeviceState *dev;
> +    CharBackend *cd;
> +    struct vhost_dev *vhost;
> +} VhostAsyncCallback;
> +
> +static void vhost_user_async_close_bh(void *opaque)
> +{
> +    VhostAsyncCallback *data = opaque;
> +    struct vhost_dev *vhost = data->vhost;
> +
> +    /*
> +     * If the vhost_dev has been cleared in the meantime there is
> +     * nothing left to do as some other path has completed the
> +     * cleanup.
> +     */
> +    if (vhost->vdev) {
> +        data->cb(data->dev);
> +    }
> +
> +    g_free(data);
> +}
> +
> +/*
> + * We only schedule the work if the machine is running. If suspended
> + * we want to keep all the in-flight data as is for migration
> + * purposes.
> + */
> +void vhost_user_async_close(DeviceState *d,
> +                            CharBackend *chardev, struct vhost_dev *vhost,
> +                            vu_async_close_fn cb)
> +{
> +    if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> +        /*
> +         * A close event may happen during a read/write, but vhost
> +         * code assumes the vhost_dev remains setup, so delay the
> +         * stop & clear.
> +         */
> +        AioContext *ctx = qemu_get_current_aio_context();
> +        VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
> +
> +        /* Save data for the callback */
> +        data->cb = cb;
> +        data->dev = d;
> +        data->cd = chardev;
> +        data->vhost = vhost;
> +
> +        /* Disable any further notifications on the chardev */
> +        qemu_chr_fe_set_handlers(chardev,
> +                                 NULL, NULL, NULL, NULL, NULL, NULL,
> +                                 false);
> +
> +        aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
> +
> +        /*
> +         * Move vhost device to the stopped state. The vhost-user device

Not this change’s fault but we should fix up the grammar here i.e. s/clean/cleaned/ and probably should be “disconnected in the BH”…etc.

> +         * will be clean up and disconnected in BH. This can be useful in
> +         * 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).
> +         *
> +         * Note if the vhost device is fully cleared by the time we
> +         * execute the bottom half we won't continue with the cleanup.
> +         */
> +        vhost->started = false;
> +    }
> +}
> +
> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> {
>     if (!virtio_has_feature(dev->protocol_features,
> -- 
> 2.34.1
> 

Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Michael S. Tsirkin]

On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
> > On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> > 
> > ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
> > the circular close by deferring shutdown due to disconnection until a
> > later point. virtio-user-blk already had this mechanism in place so
> 
> The mechanism was originally copied from virtio-net so we should probably fix it there too. AFAICT calling vhost_user_async_close() should work in net_vhost_user_event().
> 
> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.

If you do, separate patch pls and does not have to block this series.

> 
> > generalise it as a vhost-user helper function and use for both blk and
> > gpio devices.
> > 
> > While we are at it we also fix up vhost-user-gpio to re-establish the
> > event handler after close down so we can reconnect later.
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> > include/hw/virtio/vhost-user.h | 18 +++++++++
> > hw/block/vhost-user-blk.c      | 41 +++-----------------
> > hw/virtio/vhost-user-gpio.c    | 11 +++++-
> > hw/virtio/vhost-user.c         | 71 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 104 insertions(+), 37 deletions(-)
> > 
> > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > index c6e693cd3f..191216a74f 100644
> > --- a/include/hw/virtio/vhost-user.h
> > +++ b/include/hw/virtio/vhost-user.h
> > @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
> >  */
> > void vhost_user_cleanup(VhostUserState *user);
> > 
> > +/**
> > + * vhost_user_async_close() - cleanup vhost-user post connection drop
> > + * @d: DeviceState for the associated device (passed to callback)
> > + * @chardev: the CharBackend associated with the connection
> > + * @vhost: the common vhost device
> > + * @cb: the user callback function to complete the clean-up
> > + *
> > + * This function is used to handle the shutdown of a vhost-user
> > + * connection to a backend. We handle this centrally to make sure we
> > + * do all the steps and handle potential races due to VM shutdowns.
> > + * Once the connection is disabled we call a backhalf to ensure
> > + */
> > +typedef void (*vu_async_close_fn)(DeviceState *cb);
> > +
> > +void vhost_user_async_close(DeviceState *d,
> > +                            CharBackend *chardev, struct vhost_dev *vhost,
> > +                            vu_async_close_fn cb);
> > +
> > #endif
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 1177064631..aff4d2b8cb 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> >     vhost_user_blk_stop(vdev);
> > 
> >     vhost_dev_cleanup(&s->dev);
> > -}
> > 
> > -static void vhost_user_blk_chr_closed_bh(void *opaque)
> > -{
> > -    DeviceState *dev = opaque;
> > -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > -
> > -    vhost_user_blk_disconnect(dev);
> > +    /* Re-instate the event handler for new connections */
> >     qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> > -                             NULL, opaque, NULL, true);
> > +                             NULL, dev, NULL, true);
> > }
> > 
> > static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> > @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> >         }
> >         break;
> >     case CHR_EVENT_CLOSED:
> > -        if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> > -            /*
> > -             * A close event may happen during a read/write, but vhost
> > -             * code assumes the vhost_dev remains setup, so delay the
> > -             * stop & clear.
> > -             */
> > -            AioContext *ctx = qemu_get_current_aio_context();
> > -
> > -            qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> > -                    NULL, NULL, false);
> > -            aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> > -
> > -            /*
> > -             * Move vhost device to the stopped state. The vhost-user device
> > -             * will be clean up and disconnected in BH. This can be useful in
> > -             * 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;
> > -        }
> > +        /* defer close until later to avoid circular close */
> > +        vhost_user_async_close(dev, &s->chardev, &s->dev,
> > +                               vhost_user_blk_disconnect);
> >         break;
> >     case CHR_EVENT_BREAK:
> >     case CHR_EVENT_MUX_IN:
> > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> > index 75e28bcd3b..cd76287766 100644
> > --- a/hw/virtio/vhost-user-gpio.c
> > +++ b/hw/virtio/vhost-user-gpio.c
> > @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
> >     return 0;
> > }
> > 
> > +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
> > +
> > static void vu_gpio_disconnect(DeviceState *dev)
> > {
> >     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
> > 
> >     vu_gpio_stop(vdev);
> >     vhost_dev_cleanup(&gpio->vhost_dev);
> > +
> > +    /* Re-instate the event handler for new connections */
> > +    qemu_chr_fe_set_handlers(&gpio->chardev,
> > +                             NULL, NULL, vu_gpio_event,
> > +                             NULL, dev, NULL, true);
> > }
> > 
> > static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> > @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> >         }
> >         break;
> >     case CHR_EVENT_CLOSED:
> > -        vu_gpio_disconnect(dev);
> > +        /* defer close until later to avoid circular close */
> > +        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> > +                               vu_gpio_disconnect);
> >         break;
> >     case CHR_EVENT_BREAK:
> >     case CHR_EVENT_MUX_IN:
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index abe23d4ebe..8f635844af 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -21,6 +21,7 @@
> > #include "qemu/error-report.h"
> > #include "qemu/main-loop.h"
> > #include "qemu/sockets.h"
> > +#include "sysemu/runstate.h"
> > #include "sysemu/cryptodev.h"
> > #include "migration/migration.h"
> > #include "migration/postcopy-ram.h"
> > @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
> >     user->chr = NULL;
> > }
> > 
> 
> nit: Extra space 
> 
> > +
> > +typedef struct {
> > +    vu_async_close_fn cb;
> > +    DeviceState *dev;
> > +    CharBackend *cd;
> > +    struct vhost_dev *vhost;
> > +} VhostAsyncCallback;
> > +
> > +static void vhost_user_async_close_bh(void *opaque)
> > +{
> > +    VhostAsyncCallback *data = opaque;
> > +    struct vhost_dev *vhost = data->vhost;
> > +
> > +    /*
> > +     * If the vhost_dev has been cleared in the meantime there is
> > +     * nothing left to do as some other path has completed the
> > +     * cleanup.
> > +     */
> > +    if (vhost->vdev) {
> > +        data->cb(data->dev);
> > +    }
> > +
> > +    g_free(data);
> > +}
> > +
> > +/*
> > + * We only schedule the work if the machine is running. If suspended
> > + * we want to keep all the in-flight data as is for migration
> > + * purposes.
> > + */
> > +void vhost_user_async_close(DeviceState *d,
> > +                            CharBackend *chardev, struct vhost_dev *vhost,
> > +                            vu_async_close_fn cb)
> > +{
> > +    if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> > +        /*
> > +         * A close event may happen during a read/write, but vhost
> > +         * code assumes the vhost_dev remains setup, so delay the
> > +         * stop & clear.
> > +         */
> > +        AioContext *ctx = qemu_get_current_aio_context();
> > +        VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
> > +
> > +        /* Save data for the callback */
> > +        data->cb = cb;
> > +        data->dev = d;
> > +        data->cd = chardev;
> > +        data->vhost = vhost;
> > +
> > +        /* Disable any further notifications on the chardev */
> > +        qemu_chr_fe_set_handlers(chardev,
> > +                                 NULL, NULL, NULL, NULL, NULL, NULL,
> > +                                 false);
> > +
> > +        aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
> > +
> > +        /*
> > +         * Move vhost device to the stopped state. The vhost-user device
> 
> Not this change’s fault but we should fix up the grammar here i.e. s/clean/cleaned/ and probably should be “disconnected in the BH”…etc.
> 
> > +         * will be clean up and disconnected in BH. This can be useful in
> > +         * 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).
> > +         *
> > +         * Note if the vhost device is fully cleared by the time we
> > +         * execute the bottom half we won't continue with the cleanup.
> > +         */
> > +        vhost->started = false;
> > +    }
> > +}
> > +
> > static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> > {
> >     if (!virtio_has_feature(dev->protocol_features,
> > -- 
> > 2.34.1
> > 
> 

Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Raphael Norwitz]

> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> 
>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
>>> the circular close by deferring shutdown due to disconnection until a
>>> later point. virtio-user-blk already had this mechanism in place so
>> 
>> The mechanism was originally copied from virtio-net so we should probably fix it there too. AFAICT calling vhost_user_async_close() should work in net_vhost_user_event().
>> 
>> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
> 
> If you do, separate patch pls and does not have to block this series.

If the series is urgent my comments can be addressed later.

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

> 
>> 
>>> generalise it as a vhost-user helper function and use for both blk and
>>> gpio devices.
>>> 
>>> While we are at it we also fix up vhost-user-gpio to re-establish the
>>> event handler after close down so we can reconnect later.
>>> 
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>> include/hw/virtio/vhost-user.h | 18 +++++++++
>>> hw/block/vhost-user-blk.c      | 41 +++-----------------
>>> hw/virtio/vhost-user-gpio.c    | 11 +++++-
>>> hw/virtio/vhost-user.c         | 71 ++++++++++++++++++++++++++++++++++
>>> 4 files changed, 104 insertions(+), 37 deletions(-)
>>> 
>>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
>>> index c6e693cd3f..191216a74f 100644
>>> --- a/include/hw/virtio/vhost-user.h
>>> +++ b/include/hw/virtio/vhost-user.h
>>> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
>>> */
>>> void vhost_user_cleanup(VhostUserState *user);
>>> 
>>> +/**
>>> + * vhost_user_async_close() - cleanup vhost-user post connection drop
>>> + * @d: DeviceState for the associated device (passed to callback)
>>> + * @chardev: the CharBackend associated with the connection
>>> + * @vhost: the common vhost device
>>> + * @cb: the user callback function to complete the clean-up
>>> + *
>>> + * This function is used to handle the shutdown of a vhost-user
>>> + * connection to a backend. We handle this centrally to make sure we
>>> + * do all the steps and handle potential races due to VM shutdowns.
>>> + * Once the connection is disabled we call a backhalf to ensure
>>> + */
>>> +typedef void (*vu_async_close_fn)(DeviceState *cb);
>>> +
>>> +void vhost_user_async_close(DeviceState *d,
>>> +                            CharBackend *chardev, struct vhost_dev *vhost,
>>> +                            vu_async_close_fn cb);
>>> +
>>> #endif
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index 1177064631..aff4d2b8cb 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>>>    vhost_user_blk_stop(vdev);
>>> 
>>>    vhost_dev_cleanup(&s->dev);
>>> -}
>>> 
>>> -static void vhost_user_blk_chr_closed_bh(void *opaque)
>>> -{
>>> -    DeviceState *dev = opaque;
>>> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>> -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>> -
>>> -    vhost_user_blk_disconnect(dev);
>>> +    /* Re-instate the event handler for new connections */
>>>    qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
>>> -                             NULL, opaque, NULL, true);
>>> +                             NULL, dev, NULL, true);
>>> }
>>> 
>>> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>>        }
>>>        break;
>>>    case CHR_EVENT_CLOSED:
>>> -        if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>> -            /*
>>> -             * A close event may happen during a read/write, but vhost
>>> -             * code assumes the vhost_dev remains setup, so delay the
>>> -             * stop & clear.
>>> -             */
>>> -            AioContext *ctx = qemu_get_current_aio_context();
>>> -
>>> -            qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
>>> -                    NULL, NULL, false);
>>> -            aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
>>> -
>>> -            /*
>>> -             * Move vhost device to the stopped state. The vhost-user device
>>> -             * will be clean up and disconnected in BH. This can be useful in
>>> -             * 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;
>>> -        }
>>> +        /* defer close until later to avoid circular close */
>>> +        vhost_user_async_close(dev, &s->chardev, &s->dev,
>>> +                               vhost_user_blk_disconnect);
>>>        break;
>>>    case CHR_EVENT_BREAK:
>>>    case CHR_EVENT_MUX_IN:
>>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>>> index 75e28bcd3b..cd76287766 100644
>>> --- a/hw/virtio/vhost-user-gpio.c
>>> +++ b/hw/virtio/vhost-user-gpio.c
>>> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>>>    return 0;
>>> }
>>> 
>>> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
>>> +
>>> static void vu_gpio_disconnect(DeviceState *dev)
>>> {
>>>    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
>>> 
>>>    vu_gpio_stop(vdev);
>>>    vhost_dev_cleanup(&gpio->vhost_dev);
>>> +
>>> +    /* Re-instate the event handler for new connections */
>>> +    qemu_chr_fe_set_handlers(&gpio->chardev,
>>> +                             NULL, NULL, vu_gpio_event,
>>> +                             NULL, dev, NULL, true);
>>> }
>>> 
>>> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>>        }
>>>        break;
>>>    case CHR_EVENT_CLOSED:
>>> -        vu_gpio_disconnect(dev);
>>> +        /* defer close until later to avoid circular close */
>>> +        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
>>> +                               vu_gpio_disconnect);
>>>        break;
>>>    case CHR_EVENT_BREAK:
>>>    case CHR_EVENT_MUX_IN:
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index abe23d4ebe..8f635844af 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -21,6 +21,7 @@
>>> #include "qemu/error-report.h"
>>> #include "qemu/main-loop.h"
>>> #include "qemu/sockets.h"
>>> +#include "sysemu/runstate.h"
>>> #include "sysemu/cryptodev.h"
>>> #include "migration/migration.h"
>>> #include "migration/postcopy-ram.h"
>>> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
>>>    user->chr = NULL;
>>> }
>>> 
>> 
>> nit: Extra space 
>> 
>>> +
>>> +typedef struct {
>>> +    vu_async_close_fn cb;
>>> +    DeviceState *dev;
>>> +    CharBackend *cd;
>>> +    struct vhost_dev *vhost;
>>> +} VhostAsyncCallback;
>>> +
>>> +static void vhost_user_async_close_bh(void *opaque)
>>> +{
>>> +    VhostAsyncCallback *data = opaque;
>>> +    struct vhost_dev *vhost = data->vhost;
>>> +
>>> +    /*
>>> +     * If the vhost_dev has been cleared in the meantime there is
>>> +     * nothing left to do as some other path has completed the
>>> +     * cleanup.
>>> +     */
>>> +    if (vhost->vdev) {
>>> +        data->cb(data->dev);
>>> +    }
>>> +
>>> +    g_free(data);
>>> +}
>>> +
>>> +/*
>>> + * We only schedule the work if the machine is running. If suspended
>>> + * we want to keep all the in-flight data as is for migration
>>> + * purposes.
>>> + */
>>> +void vhost_user_async_close(DeviceState *d,
>>> +                            CharBackend *chardev, struct vhost_dev *vhost,
>>> +                            vu_async_close_fn cb)
>>> +{
>>> +    if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>> +        /*
>>> +         * A close event may happen during a read/write, but vhost
>>> +         * code assumes the vhost_dev remains setup, so delay the
>>> +         * stop & clear.
>>> +         */
>>> +        AioContext *ctx = qemu_get_current_aio_context();
>>> +        VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
>>> +
>>> +        /* Save data for the callback */
>>> +        data->cb = cb;
>>> +        data->dev = d;
>>> +        data->cd = chardev;
>>> +        data->vhost = vhost;
>>> +
>>> +        /* Disable any further notifications on the chardev */
>>> +        qemu_chr_fe_set_handlers(chardev,
>>> +                                 NULL, NULL, NULL, NULL, NULL, NULL,
>>> +                                 false);
>>> +
>>> +        aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
>>> +
>>> +        /*
>>> +         * Move vhost device to the stopped state. The vhost-user device
>> 
>> Not this change’s fault but we should fix up the grammar here i.e. s/clean/cleaned/ and probably should be “disconnected in the BH”…etc.
>> 
>>> +         * will be clean up and disconnected in BH. This can be useful in
>>> +         * 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).
>>> +         *
>>> +         * Note if the vhost device is fully cleared by the time we
>>> +         * execute the bottom half we won't continue with the cleanup.
>>> +         */
>>> +        vhost->started = false;
>>> +    }
>>> +}
>>> +
>>> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>>> {
>>>    if (!virtio_has_feature(dev->protocol_features,
>>> -- 
>>> 2.34.1
>>> 
>> 
> 

Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Alex Bennée]

Raphael Norwitz <raphael.norwitz@nutanix.com> writes:

>> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
>>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>> 
>>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
>>>> the circular close by deferring shutdown due to disconnection until a
>>>> later point. virtio-user-blk already had this mechanism in place so
>>> 
>>> The mechanism was originally copied from virtio-net so we should
>>> probably fix it there too. AFAICT calling vhost_user_async_close()
>>> should work in net_vhost_user_event().
>>> 
>>> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
>> 
>> If you do, separate patch pls and does not have to block this series.
>
> If the series is urgent my comments can be addressed later.

On the surface it looks similar but the vhost-net code doesn't deal in
DeviceState opaques and also has invented a s->watch variable for
manually removing gio sources. I'm not sure I'm confident I can
re-factor this code and not break something so close to release.

>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>
>> 
>>> 
>>>> generalise it as a vhost-user helper function and use for both blk and
>>>> gpio devices.
>>>> 
>>>> While we are at it we also fix up vhost-user-gpio to re-establish the
>>>> event handler after close down so we can reconnect later.
>>>> 
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>> include/hw/virtio/vhost-user.h | 18 +++++++++
>>>> hw/block/vhost-user-blk.c      | 41 +++-----------------
>>>> hw/virtio/vhost-user-gpio.c    | 11 +++++-
>>>> hw/virtio/vhost-user.c         | 71 ++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 104 insertions(+), 37 deletions(-)
>>>> 
>>>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
>>>> index c6e693cd3f..191216a74f 100644
>>>> --- a/include/hw/virtio/vhost-user.h
>>>> +++ b/include/hw/virtio/vhost-user.h
>>>> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
>>>> */
>>>> void vhost_user_cleanup(VhostUserState *user);
>>>> 
>>>> +/**
>>>> + * vhost_user_async_close() - cleanup vhost-user post connection drop
>>>> + * @d: DeviceState for the associated device (passed to callback)
>>>> + * @chardev: the CharBackend associated with the connection
>>>> + * @vhost: the common vhost device
>>>> + * @cb: the user callback function to complete the clean-up
>>>> + *
>>>> + * This function is used to handle the shutdown of a vhost-user
>>>> + * connection to a backend. We handle this centrally to make sure we
>>>> + * do all the steps and handle potential races due to VM shutdowns.
>>>> + * Once the connection is disabled we call a backhalf to ensure
>>>> + */
>>>> +typedef void (*vu_async_close_fn)(DeviceState *cb);
>>>> +
>>>> +void vhost_user_async_close(DeviceState *d,
>>>> +                            CharBackend *chardev, struct vhost_dev *vhost,
>>>> +                            vu_async_close_fn cb);
>>>> +
>>>> #endif
>>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>>> index 1177064631..aff4d2b8cb 100644
>>>> --- a/hw/block/vhost-user-blk.c
>>>> +++ b/hw/block/vhost-user-blk.c
>>>> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>>>>    vhost_user_blk_stop(vdev);
>>>> 
>>>>    vhost_dev_cleanup(&s->dev);
>>>> -}
>>>> 
>>>> -static void vhost_user_blk_chr_closed_bh(void *opaque)
>>>> -{
>>>> -    DeviceState *dev = opaque;
>>>> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>> -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>>> -
>>>> -    vhost_user_blk_disconnect(dev);
>>>> +    /* Re-instate the event handler for new connections */
>>>>    qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
>>>> -                             NULL, opaque, NULL, true);
>>>> +                             NULL, dev, NULL, true);
>>>> }
>>>> 
>>>> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>>> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>>>        }
>>>>        break;
>>>>    case CHR_EVENT_CLOSED:
>>>> -        if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>>> -            /*
>>>> -             * A close event may happen during a read/write, but vhost
>>>> -             * code assumes the vhost_dev remains setup, so delay the
>>>> -             * stop & clear.
>>>> -             */
>>>> -            AioContext *ctx = qemu_get_current_aio_context();
>>>> -
>>>> -            qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
>>>> -                    NULL, NULL, false);
>>>> -            aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
>>>> -
>>>> -            /*
>>>> -             * Move vhost device to the stopped state. The vhost-user device
>>>> -             * will be clean up and disconnected in BH. This can be useful in
>>>> -             * 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;
>>>> -        }
>>>> +        /* defer close until later to avoid circular close */
>>>> +        vhost_user_async_close(dev, &s->chardev, &s->dev,
>>>> +                               vhost_user_blk_disconnect);
>>>>        break;
>>>>    case CHR_EVENT_BREAK:
>>>>    case CHR_EVENT_MUX_IN:
>>>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>>>> index 75e28bcd3b..cd76287766 100644
>>>> --- a/hw/virtio/vhost-user-gpio.c
>>>> +++ b/hw/virtio/vhost-user-gpio.c
>>>> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>>>>    return 0;
>>>> }
>>>> 
>>>> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
>>>> +
>>>> static void vu_gpio_disconnect(DeviceState *dev)
>>>> {
>>>>    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
>>>> 
>>>>    vu_gpio_stop(vdev);
>>>>    vhost_dev_cleanup(&gpio->vhost_dev);
>>>> +
>>>> +    /* Re-instate the event handler for new connections */
>>>> +    qemu_chr_fe_set_handlers(&gpio->chardev,
>>>> +                             NULL, NULL, vu_gpio_event,
>>>> +                             NULL, dev, NULL, true);
>>>> }
>>>> 
>>>> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>>> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>>>        }
>>>>        break;
>>>>    case CHR_EVENT_CLOSED:
>>>> -        vu_gpio_disconnect(dev);
>>>> +        /* defer close until later to avoid circular close */
>>>> +        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
>>>> +                               vu_gpio_disconnect);
>>>>        break;
>>>>    case CHR_EVENT_BREAK:
>>>>    case CHR_EVENT_MUX_IN:
>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>> index abe23d4ebe..8f635844af 100644
>>>> --- a/hw/virtio/vhost-user.c
>>>> +++ b/hw/virtio/vhost-user.c
>>>> @@ -21,6 +21,7 @@
>>>> #include "qemu/error-report.h"
>>>> #include "qemu/main-loop.h"
>>>> #include "qemu/sockets.h"
>>>> +#include "sysemu/runstate.h"
>>>> #include "sysemu/cryptodev.h"
>>>> #include "migration/migration.h"
>>>> #include "migration/postcopy-ram.h"
>>>> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
>>>>    user->chr = NULL;
>>>> }
>>>> 
>>> 
>>> nit: Extra space 
>>> 
>>>> +
>>>> +typedef struct {
>>>> +    vu_async_close_fn cb;
>>>> +    DeviceState *dev;
>>>> +    CharBackend *cd;
>>>> +    struct vhost_dev *vhost;
>>>> +} VhostAsyncCallback;
>>>> +
>>>> +static void vhost_user_async_close_bh(void *opaque)
>>>> +{
>>>> +    VhostAsyncCallback *data = opaque;
>>>> +    struct vhost_dev *vhost = data->vhost;
>>>> +
>>>> +    /*
>>>> +     * If the vhost_dev has been cleared in the meantime there is
>>>> +     * nothing left to do as some other path has completed the
>>>> +     * cleanup.
>>>> +     */
>>>> +    if (vhost->vdev) {
>>>> +        data->cb(data->dev);
>>>> +    }
>>>> +
>>>> +    g_free(data);
>>>> +}
>>>> +
>>>> +/*
>>>> + * We only schedule the work if the machine is running. If suspended
>>>> + * we want to keep all the in-flight data as is for migration
>>>> + * purposes.
>>>> + */
>>>> +void vhost_user_async_close(DeviceState *d,
>>>> +                            CharBackend *chardev, struct vhost_dev *vhost,
>>>> +                            vu_async_close_fn cb)
>>>> +{
>>>> +    if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>>> +        /*
>>>> +         * A close event may happen during a read/write, but vhost
>>>> +         * code assumes the vhost_dev remains setup, so delay the
>>>> +         * stop & clear.
>>>> +         */
>>>> +        AioContext *ctx = qemu_get_current_aio_context();
>>>> +        VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
>>>> +
>>>> +        /* Save data for the callback */
>>>> +        data->cb = cb;
>>>> +        data->dev = d;
>>>> +        data->cd = chardev;
>>>> +        data->vhost = vhost;
>>>> +
>>>> +        /* Disable any further notifications on the chardev */
>>>> +        qemu_chr_fe_set_handlers(chardev,
>>>> +                                 NULL, NULL, NULL, NULL, NULL, NULL,
>>>> +                                 false);
>>>> +
>>>> +        aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
>>>> +
>>>> +        /*
>>>> +         * Move vhost device to the stopped state. The vhost-user device
>>> 
>>> Not this change’s fault but we should fix up the grammar here i.e.
>>> s/clean/cleaned/ and probably should be “disconnected in the
>>> BH”…etc.
>>> 
>>>> +         * will be clean up and disconnected in BH. This can be useful in
>>>> +         * 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).
>>>> +         *
>>>> +         * Note if the vhost device is fully cleared by the time we
>>>> +         * execute the bottom half we won't continue with the cleanup.
>>>> +         */
>>>> +        vhost->started = false;
>>>> +    }
>>>> +}
>>>> +
>>>> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>>>> {
>>>>    if (!virtio_has_feature(dev->protocol_features,
>>>> -- 
>>>> 2.34.1
>>>> 
>>> 
>> 


-- 
Alex Bennée

Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Michael S. Tsirkin]

On Wed, Nov 30, 2022 at 10:25:58AM +0000, Alex Bennée wrote:
> 
> Raphael Norwitz <raphael.norwitz@nutanix.com> writes:
> 
> >> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 
> >> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
> >>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>> 
> >>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
> >>>> the circular close by deferring shutdown due to disconnection until a
> >>>> later point. virtio-user-blk already had this mechanism in place so
> >>> 
> >>> The mechanism was originally copied from virtio-net so we should
> >>> probably fix it there too. AFAICT calling vhost_user_async_close()
> >>> should work in net_vhost_user_event().
> >>> 
> >>> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
> >> 
> >> If you do, separate patch pls and does not have to block this series.
> >
> > If the series is urgent my comments can be addressed later.
> 
> On the surface it looks similar but the vhost-net code doesn't deal in
> DeviceState opaques and also has invented a s->watch variable for
> manually removing gio sources. I'm not sure I'm confident I can
> re-factor this code and not break something so close to release.

OK, that's fair.

> >
> > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> >
> >> 
> >>> 
> >>>> generalise it as a vhost-user helper function and use for both blk and
> >>>> gpio devices.
> >>>> 
> >>>> While we are at it we also fix up vhost-user-gpio to re-establish the
> >>>> event handler after close down so we can reconnect later.
> >>>> 
> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >>>> ---
> >>>> include/hw/virtio/vhost-user.h | 18 +++++++++
> >>>> hw/block/vhost-user-blk.c      | 41 +++-----------------
> >>>> hw/virtio/vhost-user-gpio.c    | 11 +++++-
> >>>> hw/virtio/vhost-user.c         | 71 ++++++++++++++++++++++++++++++++++
> >>>> 4 files changed, 104 insertions(+), 37 deletions(-)
> >>>> 
> >>>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> >>>> index c6e693cd3f..191216a74f 100644
> >>>> --- a/include/hw/virtio/vhost-user.h
> >>>> +++ b/include/hw/virtio/vhost-user.h
> >>>> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
> >>>> */
> >>>> void vhost_user_cleanup(VhostUserState *user);
> >>>> 
> >>>> +/**
> >>>> + * vhost_user_async_close() - cleanup vhost-user post connection drop
> >>>> + * @d: DeviceState for the associated device (passed to callback)
> >>>> + * @chardev: the CharBackend associated with the connection
> >>>> + * @vhost: the common vhost device
> >>>> + * @cb: the user callback function to complete the clean-up
> >>>> + *
> >>>> + * This function is used to handle the shutdown of a vhost-user
> >>>> + * connection to a backend. We handle this centrally to make sure we
> >>>> + * do all the steps and handle potential races due to VM shutdowns.
> >>>> + * Once the connection is disabled we call a backhalf to ensure
> >>>> + */
> >>>> +typedef void (*vu_async_close_fn)(DeviceState *cb);
> >>>> +
> >>>> +void vhost_user_async_close(DeviceState *d,
> >>>> +                            CharBackend *chardev, struct vhost_dev *vhost,
> >>>> +                            vu_async_close_fn cb);
> >>>> +
> >>>> #endif
> >>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >>>> index 1177064631..aff4d2b8cb 100644
> >>>> --- a/hw/block/vhost-user-blk.c
> >>>> +++ b/hw/block/vhost-user-blk.c
> >>>> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> >>>>    vhost_user_blk_stop(vdev);
> >>>> 
> >>>>    vhost_dev_cleanup(&s->dev);
> >>>> -}
> >>>> 
> >>>> -static void vhost_user_blk_chr_closed_bh(void *opaque)
> >>>> -{
> >>>> -    DeviceState *dev = opaque;
> >>>> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>> -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> >>>> -
> >>>> -    vhost_user_blk_disconnect(dev);
> >>>> +    /* Re-instate the event handler for new connections */
> >>>>    qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> >>>> -                             NULL, opaque, NULL, true);
> >>>> +                             NULL, dev, NULL, true);
> >>>> }
> >>>> 
> >>>> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> >>>> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> >>>>        }
> >>>>        break;
> >>>>    case CHR_EVENT_CLOSED:
> >>>> -        if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> >>>> -            /*
> >>>> -             * A close event may happen during a read/write, but vhost
> >>>> -             * code assumes the vhost_dev remains setup, so delay the
> >>>> -             * stop & clear.
> >>>> -             */
> >>>> -            AioContext *ctx = qemu_get_current_aio_context();
> >>>> -
> >>>> -            qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> >>>> -                    NULL, NULL, false);
> >>>> -            aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> >>>> -
> >>>> -            /*
> >>>> -             * Move vhost device to the stopped state. The vhost-user device
> >>>> -             * will be clean up and disconnected in BH. This can be useful in
> >>>> -             * 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;
> >>>> -        }
> >>>> +        /* defer close until later to avoid circular close */
> >>>> +        vhost_user_async_close(dev, &s->chardev, &s->dev,
> >>>> +                               vhost_user_blk_disconnect);
> >>>>        break;
> >>>>    case CHR_EVENT_BREAK:
> >>>>    case CHR_EVENT_MUX_IN:
> >>>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> >>>> index 75e28bcd3b..cd76287766 100644
> >>>> --- a/hw/virtio/vhost-user-gpio.c
> >>>> +++ b/hw/virtio/vhost-user-gpio.c
> >>>> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
> >>>>    return 0;
> >>>> }
> >>>> 
> >>>> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
> >>>> +
> >>>> static void vu_gpio_disconnect(DeviceState *dev)
> >>>> {
> >>>>    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
> >>>> 
> >>>>    vu_gpio_stop(vdev);
> >>>>    vhost_dev_cleanup(&gpio->vhost_dev);
> >>>> +
> >>>> +    /* Re-instate the event handler for new connections */
> >>>> +    qemu_chr_fe_set_handlers(&gpio->chardev,
> >>>> +                             NULL, NULL, vu_gpio_event,
> >>>> +                             NULL, dev, NULL, true);
> >>>> }
> >>>> 
> >>>> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> >>>> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> >>>>        }
> >>>>        break;
> >>>>    case CHR_EVENT_CLOSED:
> >>>> -        vu_gpio_disconnect(dev);
> >>>> +        /* defer close until later to avoid circular close */
> >>>> +        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> >>>> +                               vu_gpio_disconnect);
> >>>>        break;
> >>>>    case CHR_EVENT_BREAK:
> >>>>    case CHR_EVENT_MUX_IN:
> >>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>>> index abe23d4ebe..8f635844af 100644
> >>>> --- a/hw/virtio/vhost-user.c
> >>>> +++ b/hw/virtio/vhost-user.c
> >>>> @@ -21,6 +21,7 @@
> >>>> #include "qemu/error-report.h"
> >>>> #include "qemu/main-loop.h"
> >>>> #include "qemu/sockets.h"
> >>>> +#include "sysemu/runstate.h"
> >>>> #include "sysemu/cryptodev.h"
> >>>> #include "migration/migration.h"
> >>>> #include "migration/postcopy-ram.h"
> >>>> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
> >>>>    user->chr = NULL;
> >>>> }
> >>>> 
> >>> 
> >>> nit: Extra space 
> >>> 
> >>>> +
> >>>> +typedef struct {
> >>>> +    vu_async_close_fn cb;
> >>>> +    DeviceState *dev;
> >>>> +    CharBackend *cd;
> >>>> +    struct vhost_dev *vhost;
> >>>> +} VhostAsyncCallback;
> >>>> +
> >>>> +static void vhost_user_async_close_bh(void *opaque)
> >>>> +{
> >>>> +    VhostAsyncCallback *data = opaque;
> >>>> +    struct vhost_dev *vhost = data->vhost;
> >>>> +
> >>>> +    /*
> >>>> +     * If the vhost_dev has been cleared in the meantime there is
> >>>> +     * nothing left to do as some other path has completed the
> >>>> +     * cleanup.
> >>>> +     */
> >>>> +    if (vhost->vdev) {
> >>>> +        data->cb(data->dev);
> >>>> +    }
> >>>> +
> >>>> +    g_free(data);
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * We only schedule the work if the machine is running. If suspended
> >>>> + * we want to keep all the in-flight data as is for migration
> >>>> + * purposes.
> >>>> + */
> >>>> +void vhost_user_async_close(DeviceState *d,
> >>>> +                            CharBackend *chardev, struct vhost_dev *vhost,
> >>>> +                            vu_async_close_fn cb)
> >>>> +{
> >>>> +    if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> >>>> +        /*
> >>>> +         * A close event may happen during a read/write, but vhost
> >>>> +         * code assumes the vhost_dev remains setup, so delay the
> >>>> +         * stop & clear.
> >>>> +         */
> >>>> +        AioContext *ctx = qemu_get_current_aio_context();
> >>>> +        VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
> >>>> +
> >>>> +        /* Save data for the callback */
> >>>> +        data->cb = cb;
> >>>> +        data->dev = d;
> >>>> +        data->cd = chardev;
> >>>> +        data->vhost = vhost;
> >>>> +
> >>>> +        /* Disable any further notifications on the chardev */
> >>>> +        qemu_chr_fe_set_handlers(chardev,
> >>>> +                                 NULL, NULL, NULL, NULL, NULL, NULL,
> >>>> +                                 false);
> >>>> +
> >>>> +        aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
> >>>> +
> >>>> +        /*
> >>>> +         * Move vhost device to the stopped state. The vhost-user device
> >>> 
> >>> Not this change’s fault but we should fix up the grammar here i.e.
> >>> s/clean/cleaned/ and probably should be “disconnected in the
> >>> BH”…etc.
> >>> 
> >>>> +         * will be clean up and disconnected in BH. This can be useful in
> >>>> +         * 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).
> >>>> +         *
> >>>> +         * Note if the vhost device is fully cleared by the time we
> >>>> +         * execute the bottom half we won't continue with the cleanup.
> >>>> +         */
> >>>> +        vhost->started = false;
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> >>>> {
> >>>>    if (!virtio_has_feature(dev->protocol_features,
> >>>> -- 
> >>>> 2.34.1
> >>>> 
> >>> 
> >> 
> 
> 
> -- 
> Alex Bennée

Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

[Alex Bennée]

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

> On Wed, Nov 30, 2022 at 10:25:58AM +0000, Alex Bennée wrote:
>> 
>> Raphael Norwitz <raphael.norwitz@nutanix.com> writes:
>> 
>> >> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> 
>> >> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
>> >>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >>>> 
>> >>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
>> >>>> the circular close by deferring shutdown due to disconnection until a
>> >>>> later point. virtio-user-blk already had this mechanism in place so
>> >>> 
>> >>> The mechanism was originally copied from virtio-net so we should
>> >>> probably fix it there too. AFAICT calling vhost_user_async_close()
>> >>> should work in net_vhost_user_event().
>> >>> 
>> >>> Otherwise the code looks good modulo a few nits. Happy to see
>> >>> the duplicated logic is being generalized.
>> >> 
>> >> If you do, separate patch pls and does not have to block this series.
>> >
>> > If the series is urgent my comments can be addressed later.
>> 
>> On the surface it looks similar but the vhost-net code doesn't deal in
>> DeviceState opaques and also has invented a s->watch variable for
>> manually removing gio sources. I'm not sure I'm confident I can
>> re-factor this code and not break something so close to release.
>
> OK, that's fair.

See 20221130112439.2527228-1-alex.bennee@linaro.org for the v4 series.

-- 
Alex Bennée

Re: [PATCH for 7.2-rc3 v3 0/7] fix vhost-user issues with CI

[Michael S. Tsirkin]

Patch 1 is good but inappropriate for 7.2
Patch 2 should be last in series.
Patch 4 we are dropping.
I thought hard about it, I think we should patch vhost user net too
because of the risk introduced by patch 2 (which affects everyone).
Can be a patch on top though.

Besides this, for series:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

-- 
MST