[PATCH v3 0/5] vhost-user-blk: live resize additional APIs

[PATCH v3 0/5] vhost-user-blk: live resize additional APIs

[Vladimir Sementsov-Ogievskiy]

v3:
02: add r-b by Markus
03: improve commit message
04: improve documentation, merge race-fix here (which was v2:05),
    rebase on master (migration_is_running() now without arguments)
05: improve documentation

Vladimir Sementsov-Ogievskiy (5):
  vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change
  qdev-monitor: fix error message in find_device_state()
  qdev-monitor: add option to report GenericError from find_device_state
  qapi: introduce device-sync-config
  qapi: introduce CONFIG_READ event

 hw/block/vhost-user-blk.c | 32 +++++++++++-------
 hw/virtio/virtio-pci.c    | 18 ++++++++++
 include/hw/qdev-core.h    |  3 ++
 include/monitor/qdev.h    |  2 ++
 include/sysemu/runstate.h |  1 +
 monitor/monitor.c         |  1 +
 qapi/qdev.json            | 54 ++++++++++++++++++++++++++++++
 stubs/qdev.c              |  6 ++++
 system/qdev-monitor.c     | 70 ++++++++++++++++++++++++++++++++++++---
 system/runstate.c         |  5 +++
 10 files changed, 175 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change

[Vladimir Sementsov-Ogievskiy]

Let's not care about what was changed and update the whole config,
reasons:

1. config->geometry should be updated together with capacity, so we fix
   a bug.

2. Vhost-user protocol doesn't say anything about config change
   limitation. Silent ignore of changes doesn't seem to be correct.

3. vhost-user-vsock reads the whole config

4. on realize we don't do any checks on retrieved config, so no reason
   to care here

Comment "valid for resize only" exists since introduction the whole
hw/block/vhost-user-blk.c in commit
   00343e4b54ba0685e9ebe928ec5713b0cf7f1d1c
    "vhost-user-blk: introduce a new vhost-user-blk host device",
seems it was just an extra limitation.

Also, let's notify guest unconditionally:

1. So does vhost-user-vsock

2. We are going to reuse the functionality in new cases when we do want
   to notify the guest unconditionally. So, no reason to create extra
   branches in the logic.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 hw/block/vhost-user-blk.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 6a856ad51a..9e6bbc6950 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -91,7 +91,6 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
 {
     int ret;
-    struct virtio_blk_config blkcfg;
     VirtIODevice *vdev = dev->vdev;
     VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
     Error *local_err = NULL;
@@ -100,19 +99,15 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
         return 0;
     }
 
-    ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
+    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
                                vdev->config_len, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
     }
 
-    /* valid for resize only */
-    if (blkcfg.capacity != s->blkcfg.capacity) {
-        s->blkcfg.capacity = blkcfg.capacity;
-        memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
-        virtio_notify_config(dev->vdev);
-    }
+    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
+    virtio_notify_config(dev->vdev);
 
     return 0;
 }
-- 
2.34.1

[PATCH v3 2/5] qdev-monitor: fix error message in find_device_state()

[Vladimir Sementsov-Ogievskiy]

This "hotpluggable" here is misleading. Actually we check is object a
device or not. Let's drop the word.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 system/qdev-monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index c1243891c3..840177d19f 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -891,7 +891,7 @@ static DeviceState *find_device_state(const char *id, Error **errp)
 
     dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
     if (!dev) {
-        error_setg(errp, "%s is not a hotpluggable device", id);
+        error_setg(errp, "%s is not a device", id);
         return NULL;
     }
 
-- 
2.34.1

[PATCH v3 3/5] qdev-monitor: add option to report GenericError from find_device_state

[Vladimir Sementsov-Ogievskiy]

Here we just prepare for the following patch, making possible to report
GenericError as recommended.

This patch doesn't aim to prevent further use of DeviceNotFound by
future interfaces:

 - find_device_state() is used in blk_by_qdev_id() and qmp_get_blk()
   functions, which may lead to spread of DeviceNotFound anyway
 - also, nothing prevent simply copy-pasting find_device_state() calls
   with false argument

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 system/qdev-monitor.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 840177d19f..7e075d91c1 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -878,13 +878,20 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     object_unref(OBJECT(dev));
 }
 
-static DeviceState *find_device_state(const char *id, Error **errp)
+/*
+ * Note that creating new APIs using error classes other than GenericError is
+ * not recommended. Set use_generic_error=true for new interfaces.
+ */
+static DeviceState *find_device_state(const char *id, bool use_generic_error,
+                                      Error **errp)
 {
     Object *obj = object_resolve_path_at(qdev_get_peripheral(), id);
     DeviceState *dev;
 
     if (!obj) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+        error_set(errp,
+                  (use_generic_error ?
+                   ERROR_CLASS_GENERIC_ERROR : ERROR_CLASS_DEVICE_NOT_FOUND),
                   "Device '%s' not found", id);
         return NULL;
     }
@@ -948,7 +955,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
 void qmp_device_del(const char *id, Error **errp)
 {
-    DeviceState *dev = find_device_state(id, errp);
+    DeviceState *dev = find_device_state(id, false, errp);
     if (dev != NULL) {
         if (dev->pending_deleted_event &&
             (dev->pending_deleted_expires_ms == 0 ||
@@ -1068,7 +1075,7 @@ BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
 
     GLOBAL_STATE_CODE();
 
-    dev = find_device_state(id, errp);
+    dev = find_device_state(id, false, errp);
     if (dev == NULL) {
         return NULL;
     }
-- 
2.34.1

[PATCH v3 4/5] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

Add command to sync config from vhost-user backend to the device. It
may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
triggered interrupt to the guest or just not available (not supported
by vhost-user server).

Command result is racy if allow it during migration. Let's allow the
sync only in RUNNING state.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/block/vhost-user-blk.c | 27 ++++++++++++++++------
 hw/virtio/virtio-pci.c    |  9 ++++++++
 include/hw/qdev-core.h    |  3 +++
 include/sysemu/runstate.h |  1 +
 qapi/qdev.json            | 21 +++++++++++++++++
 system/qdev-monitor.c     | 47 +++++++++++++++++++++++++++++++++++++++
 system/runstate.c         |  5 +++++
 7 files changed, 106 insertions(+), 7 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9e6bbc6950..2f301f380c 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
     s->blkcfg.wce = blkcfg->wce;
 }
 
+static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
+{
+    int ret;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBlk *s = VHOST_USER_BLK(vdev);
+
+    ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
+                               vdev->config_len, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    memcpy(vdev->config, &s->blkcfg, vdev->config_len);
+    virtio_notify_config(vdev);
+
+    return 0;
+}
+
 static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
 {
     int ret;
-    VirtIODevice *vdev = dev->vdev;
-    VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
     Error *local_err = NULL;
 
     if (!dev->started) {
         return 0;
     }
 
-    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
-                               vdev->config_len, &local_err);
+    ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
     }
 
-    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
-    virtio_notify_config(dev->vdev);
-
     return 0;
 }
 
@@ -576,6 +588,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
 
     device_class_set_props(dc, vhost_user_blk_properties);
     dc->vmsd = &vmstate_vhost_user_blk;
+    dc->sync_config = vhost_user_blk_sync_config;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     vdc->realize = vhost_user_blk_device_realize;
     vdc->unrealize = vhost_user_blk_device_unrealize;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index eaaf86402c..92afbae71c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2501,6 +2501,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
     vpciklass->parent_dc_realize(qdev, errp);
 }
 
+static int virtio_pci_sync_config(DeviceState *dev, Error **errp)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+
+    return qdev_sync_config(DEVICE(vdev), errp);
+}
+
 static void virtio_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -2517,6 +2525,7 @@ static void virtio_pci_class_init(ObjectClass *klass, void *data)
     device_class_set_parent_realize(dc, virtio_pci_dc_realize,
                                     &vpciklass->parent_dc_realize);
     rc->phases.hold = virtio_pci_bus_reset_hold;
+    dc->sync_config = virtio_pci_sync_config;
 }
 
 static const TypeInfo virtio_pci_info = {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9228e96c87..87135bdcdf 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
 typedef void (*DeviceReset)(DeviceState *dev);
 typedef void (*BusRealize)(BusState *bus, Error **errp);
 typedef void (*BusUnrealize)(BusState *bus);
+typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
 
 /**
  * struct DeviceClass - The base class for all devices.
@@ -162,6 +163,7 @@ struct DeviceClass {
     DeviceReset reset;
     DeviceRealize realize;
     DeviceUnrealize unrealize;
+    DeviceSyncConfig sync_config;
 
     /**
      * @vmsd: device state serialisation description for
@@ -546,6 +548,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
  */
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
+int qdev_sync_config(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp);
 void qdev_machine_creation_done(void);
diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 0117d243c4..296af52322 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -5,6 +5,7 @@
 #include "qemu/notify.h"
 
 bool runstate_check(RunState state);
+const char *current_run_state_str(void);
 void runstate_set(RunState new_state);
 RunState runstate_get(void);
 bool runstate_is_running(void);
diff --git a/qapi/qdev.json b/qapi/qdev.json
index facaa0bc6a..e8be79c3d5 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -161,3 +161,24 @@
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @device-sync-config:
+#
+# Synchronize config from backend to the guest. The command notifies
+# re-read the device config from the backend and notifies the guest
+# to re-read the config. The command may be used to notify the guest
+# about block device capcity change. Currently only vhost-user-blk
+# device supports this.
+#
+# @id: the device's ID or QOM path
+#
+# Features:
+#
+# @unstable: The command is experimental.
+#
+# Since: 9.1
+##
+{ 'command': 'device-sync-config',
+  'features': [ 'unstable' ],
+  'data': {'id': 'str'} }
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 7e075d91c1..cb35ea0b86 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -23,6 +23,7 @@
 #include "monitor/monitor.h"
 #include "monitor/qdev.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
 #include "qapi/qmp/dispatch.h"
@@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
     }
 }
 
+int qdev_sync_config(DeviceState *dev, Error **errp)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+    if (!dc->sync_config) {
+        error_setg(errp, "device-sync-config is not supported for '%s'",
+                   object_get_typename(OBJECT(dev)));
+        return -ENOTSUP;
+    }
+
+    return dc->sync_config(dev, errp);
+}
+
+void qmp_device_sync_config(const char *id, Error **errp)
+{
+    DeviceState *dev;
+
+    /*
+     * During migration there is a race between syncing`config and
+     * migrating it, so let's just not allow it.
+     *
+     * Moreover, let's not rely on setting up interrupts in paused
+     * state, which may be a part of migration process.
+     */
+
+    if (migration_is_running()) {
+        error_setg(errp, "Config synchronization is not allowed "
+                   "during migration.");
+        return;
+    }
+
+    if (!runstate_is_running()) {
+        error_setg(errp, "Config synchronization allowed only in '%s' state, "
+                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
+                   current_run_state_str());
+        return;
+    }
+
+    dev = find_device_state(id, true, errp);
+    if (!dev) {
+        return;
+    }
+
+    qdev_sync_config(dev, errp);
+}
+
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
diff --git a/system/runstate.c b/system/runstate.c
index d6ab860eca..8fd89172ae 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -189,6 +189,11 @@ bool runstate_check(RunState state)
     return current_run_state == state;
 }
 
+const char *current_run_state_str(void)
+{
+    return RunState_str(current_run_state);
+}
+
 static void runstate_init(void)
 {
     const RunStateTransition *p;
-- 
2.34.1

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/block/vhost-user-blk.c | 27 ++++++++++++++++------
>  hw/virtio/virtio-pci.c    |  9 ++++++++
>  include/hw/qdev-core.h    |  3 +++
>  include/sysemu/runstate.h |  1 +
>  qapi/qdev.json            | 21 +++++++++++++++++
>  system/qdev-monitor.c     | 47 +++++++++++++++++++++++++++++++++++++++
>  system/runstate.c         |  5 +++++
>  7 files changed, 106 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9e6bbc6950..2f301f380c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
>      s->blkcfg.wce = blkcfg->wce;
>  }
>  
> +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
> +{
> +    int ret;
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +
> +    ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> +                               vdev->config_len, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    memcpy(vdev->config, &s->blkcfg, vdev->config_len);
> +    virtio_notify_config(vdev);
> +
> +    return 0;
> +}
> +
>  static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>  {
>      int ret;
> -    VirtIODevice *vdev = dev->vdev;
> -    VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>      Error *local_err = NULL;
>  
>      if (!dev->started) {
>          return 0;
>      }
>  
> -    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
> -                               vdev->config_len, &local_err);
> +    ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
>          return ret;
>      }
>  
> -    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
> -    virtio_notify_config(dev->vdev);
> -
>      return 0;
>  }

This factors vhost_user_blk_sync_config() out of
vhost_user_blk_handle_config_change() for reuse.  Correct?

>  
> @@ -576,6 +588,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_props(dc, vhost_user_blk_properties);
>      dc->vmsd = &vmstate_vhost_user_blk;
> +    dc->sync_config = vhost_user_blk_sync_config;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      vdc->realize = vhost_user_blk_device_realize;
>      vdc->unrealize = vhost_user_blk_device_unrealize;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index eaaf86402c..92afbae71c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2501,6 +2501,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>      vpciklass->parent_dc_realize(qdev, errp);
>  }
>  
> +static int virtio_pci_sync_config(DeviceState *dev, Error **errp)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    return qdev_sync_config(DEVICE(vdev), errp);
> +}
> +
>  static void virtio_pci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -2517,6 +2525,7 @@ static void virtio_pci_class_init(ObjectClass *klass, void *data)
>      device_class_set_parent_realize(dc, virtio_pci_dc_realize,
>                                      &vpciklass->parent_dc_realize);
>      rc->phases.hold = virtio_pci_bus_reset_hold;
> +    dc->sync_config = virtio_pci_sync_config;
>  }
>  

I tried to follow the callbacks, but quickly gave up.  Leaving to a
reviewer who understands virtio.

>  static const TypeInfo virtio_pci_info = {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9228e96c87..87135bdcdf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
>  typedef void (*DeviceReset)(DeviceState *dev);
>  typedef void (*BusRealize)(BusState *bus, Error **errp);
>  typedef void (*BusUnrealize)(BusState *bus);
> +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
>  
>  /**
>   * struct DeviceClass - The base class for all devices.
> @@ -162,6 +163,7 @@ struct DeviceClass {
>      DeviceReset reset;
>      DeviceRealize realize;
>      DeviceUnrealize unrealize;
> +    DeviceSyncConfig sync_config;
>  
>      /**
>       * @vmsd: device state serialisation description for
> @@ -546,6 +548,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
>   */
>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>  void qdev_unplug(DeviceState *dev, Error **errp);
> +int qdev_sync_config(DeviceState *dev, Error **errp);
>  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                    DeviceState *dev, Error **errp);
>  void qdev_machine_creation_done(void);
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 0117d243c4..296af52322 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -5,6 +5,7 @@
>  #include "qemu/notify.h"
>  
>  bool runstate_check(RunState state);
> +const char *current_run_state_str(void);
>  void runstate_set(RunState new_state);
>  RunState runstate_get(void);
>  bool runstate_is_running(void);
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index facaa0bc6a..e8be79c3d5 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -161,3 +161,24 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @device-sync-config:
> +#
> +# Synchronize config from backend to the guest. The command notifies
> +# re-read the device config from the backend and notifies the guest
> +# to re-read the config. The command may be used to notify the guest
> +# about block device capcity change. Currently only vhost-user-blk
> +# device supports this.

I'm not sure I understand this.  To work towards an understanding, I
rephrase it, and you point out the errors.

     Synchronize device configuration from host to guest part.  First,
     copy the configuration from the host part (backend) to the guest
     part (frontend).  Then notify guest software that device
     configuration changed.

I wonder how configuration can get out of sync.  Can you explain?

> +#
> +# @id: the device's ID or QOM path
> +#
> +# Features:
> +#
> +# @unstable: The command is experimental.
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'device-sync-config',
> +  'features': [ 'unstable' ],
> +  'data': {'id': 'str'} }
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 7e075d91c1..cb35ea0b86 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -23,6 +23,7 @@
>  #include "monitor/monitor.h"
>  #include "monitor/qdev.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/runstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
>  #include "qapi/qmp/dispatch.h"
> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>      }
>  }
>  
> +int qdev_sync_config(DeviceState *dev, Error **errp)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    if (!dc->sync_config) {
> +        error_setg(errp, "device-sync-config is not supported for '%s'",
> +                   object_get_typename(OBJECT(dev)));
> +        return -ENOTSUP;
> +    }
> +
> +    return dc->sync_config(dev, errp);
> +}
> +
> +void qmp_device_sync_config(const char *id, Error **errp)
> +{
> +    DeviceState *dev;
> +
> +    /*
> +     * During migration there is a race between syncing`config and
> +     * migrating it, so let's just not allow it.

Can you briefly explain the race?

> +     *
> +     * Moreover, let's not rely on setting up interrupts in paused
> +     * state, which may be a part of migration process.

What dependence exactly are you avoiding?  Config synchronization
depending on guest interrupt delivery?

> +     */
> +
> +    if (migration_is_running()) {
> +        error_setg(errp, "Config synchronization is not allowed "
> +                   "during migration.");

qapi/error.h:

     * The resulting message should be a single phrase, with no newline or
     * trailing punctuation.

Drop the period, please.

> +        return;
> +    }
> +
> +    if (!runstate_is_running()) {
> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
> +                   current_run_state_str());
> +        return;
> +    }
> +
> +    dev = find_device_state(id, true, errp);
> +    if (!dev) {
> +        return;
> +    }
> +
> +    qdev_sync_config(dev, errp);
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> diff --git a/system/runstate.c b/system/runstate.c
> index d6ab860eca..8fd89172ae 100644
> --- a/system/runstate.c
> +++ b/system/runstate.c
> @@ -189,6 +189,11 @@ bool runstate_check(RunState state)
>      return current_run_state == state;
>  }
>  
> +const char *current_run_state_str(void)
> +{
> +    return RunState_str(current_run_state);
> +}
> +

This helper is used just once.  Suggest to use
RunState_str(runstate_get()) instead.

>  static void runstate_init(void)
>  {
>      const RunStateTransition *p;

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 24.04.24 14:48, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Add command to sync config from vhost-user backend to the device. It
>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>> triggered interrupt to the guest or just not available (not supported
>> by vhost-user server).
>>
>> Command result is racy if allow it during migration. Let's allow the
>> sync only in RUNNING state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/block/vhost-user-blk.c | 27 ++++++++++++++++------
>>   hw/virtio/virtio-pci.c    |  9 ++++++++
>>   include/hw/qdev-core.h    |  3 +++
>>   include/sysemu/runstate.h |  1 +
>>   qapi/qdev.json            | 21 +++++++++++++++++
>>   system/qdev-monitor.c     | 47 +++++++++++++++++++++++++++++++++++++++
>>   system/runstate.c         |  5 +++++
>>   7 files changed, 106 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 9e6bbc6950..2f301f380c 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
>>       s->blkcfg.wce = blkcfg->wce;
>>   }
>>   
>> +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
>> +{
>> +    int ret;
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> +    VHostUserBlk *s = VHOST_USER_BLK(vdev);
>> +
>> +    ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
>> +                               vdev->config_len, errp);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    memcpy(vdev->config, &s->blkcfg, vdev->config_len);
>> +    virtio_notify_config(vdev);
>> +
>> +    return 0;
>> +}
>> +
>>   static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>>   {
>>       int ret;
>> -    VirtIODevice *vdev = dev->vdev;
>> -    VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>>       Error *local_err = NULL;
>>   
>>       if (!dev->started) {
>>           return 0;
>>       }
>>   
>> -    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
>> -                               vdev->config_len, &local_err);
>> +    ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), &local_err);
>>       if (ret < 0) {
>>           error_report_err(local_err);
>>           return ret;
>>       }
>>   
>> -    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
>> -    virtio_notify_config(dev->vdev);
>> -
>>       return 0;
>>   }
> 
> This factors vhost_user_blk_sync_config() out of
> vhost_user_blk_handle_config_change() for reuse.  Correct?

Yes. Will split to a separate patch in v4

> 
>>   
>> @@ -576,6 +588,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
>>   
>>       device_class_set_props(dc, vhost_user_blk_properties);
>>       dc->vmsd = &vmstate_vhost_user_blk;
>> +    dc->sync_config = vhost_user_blk_sync_config;
>>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>       vdc->realize = vhost_user_blk_device_realize;
>>       vdc->unrealize = vhost_user_blk_device_unrealize;
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index eaaf86402c..92afbae71c 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2501,6 +2501,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>>       vpciklass->parent_dc_realize(qdev, errp);
>>   }
>>   
>> +static int virtio_pci_sync_config(DeviceState *dev, Error **errp)
>> +{
>> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
>> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> +
>> +    return qdev_sync_config(DEVICE(vdev), errp);
>> +}
>> +
>>   static void virtio_pci_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -2517,6 +2525,7 @@ static void virtio_pci_class_init(ObjectClass *klass, void *data)
>>       device_class_set_parent_realize(dc, virtio_pci_dc_realize,
>>                                       &vpciklass->parent_dc_realize);
>>       rc->phases.hold = virtio_pci_bus_reset_hold;
>> +    dc->sync_config = virtio_pci_sync_config;
>>   }
>>   
> 
> I tried to follow the callbacks, but quickly gave up.  Leaving to a
> reviewer who understands virtio.
> 
>>   static const TypeInfo virtio_pci_info = {
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 9228e96c87..87135bdcdf 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
>>   typedef void (*DeviceReset)(DeviceState *dev);
>>   typedef void (*BusRealize)(BusState *bus, Error **errp);
>>   typedef void (*BusUnrealize)(BusState *bus);
>> +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
>>   
>>   /**
>>    * struct DeviceClass - The base class for all devices.
>> @@ -162,6 +163,7 @@ struct DeviceClass {
>>       DeviceReset reset;
>>       DeviceRealize realize;
>>       DeviceUnrealize unrealize;
>> +    DeviceSyncConfig sync_config;
>>   
>>       /**
>>        * @vmsd: device state serialisation description for
>> @@ -546,6 +548,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
>>    */
>>   HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>>   void qdev_unplug(DeviceState *dev, Error **errp);
>> +int qdev_sync_config(DeviceState *dev, Error **errp);
>>   void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>>                                     DeviceState *dev, Error **errp);
>>   void qdev_machine_creation_done(void);
>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>> index 0117d243c4..296af52322 100644
>> --- a/include/sysemu/runstate.h
>> +++ b/include/sysemu/runstate.h
>> @@ -5,6 +5,7 @@
>>   #include "qemu/notify.h"
>>   
>>   bool runstate_check(RunState state);
>> +const char *current_run_state_str(void);
>>   void runstate_set(RunState new_state);
>>   RunState runstate_get(void);
>>   bool runstate_is_running(void);
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index facaa0bc6a..e8be79c3d5 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -161,3 +161,24 @@
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>     'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @device-sync-config:
>> +#
>> +# Synchronize config from backend to the guest. The command notifies
>> +# re-read the device config from the backend and notifies the guest
>> +# to re-read the config. The command may be used to notify the guest
>> +# about block device capcity change. Currently only vhost-user-blk
>> +# device supports this.
> 
> I'm not sure I understand this.  To work towards an understanding, I
> rephrase it, and you point out the errors.
> 
>       Synchronize device configuration from host to guest part.  First,
>       copy the configuration from the host part (backend) to the guest
>       part (frontend).  Then notify guest software that device
>       configuration changed.

Correct, thanks

> 
> I wonder how configuration can get out of sync.  Can you explain?
> 

The example (and the original feature, which triggered developing this) is vhost disk resize. If vhost-server (backend) doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that disk capacity changed.

>> +#
>> +# @id: the device's ID or QOM path
>> +#
>> +# Features:
>> +#
>> +# @unstable: The command is experimental.
>> +#
>> +# Since: 9.1
>> +##
>> +{ 'command': 'device-sync-config',
>> +  'features': [ 'unstable' ],
>> +  'data': {'id': 'str'} }
>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> index 7e075d91c1..cb35ea0b86 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -23,6 +23,7 @@
>>   #include "monitor/monitor.h"
>>   #include "monitor/qdev.h"
>>   #include "sysemu/arch_init.h"
>> +#include "sysemu/runstate.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-qdev.h"
>>   #include "qapi/qmp/dispatch.h"
>> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>>       }
>>   }
>>   
>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>> +{
>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> +
>> +    if (!dc->sync_config) {
>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>> +                   object_get_typename(OBJECT(dev)));
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    return dc->sync_config(dev, errp);
>> +}
>> +
>> +void qmp_device_sync_config(const char *id, Error **errp)
>> +{
>> +    DeviceState *dev;
>> +
>> +    /*
>> +     * During migration there is a race between syncing`config and
>> +     * migrating it, so let's just not allow it.
> 
> Can you briefly explain the race?

If at the moment of qmp command, corresponding config already migrated to the target, we'll change only the config on source, but on the target we'll still have outdated config.

> 
>> +     *
>> +     * Moreover, let's not rely on setting up interrupts in paused
>> +     * state, which may be a part of migration process.
> 
> What dependence exactly are you avoiding?  Config synchronization
> depending on guest interrupt delivery?

Right, guest is notified by pci_set_irq.

> 
>> +     */
>> +
>> +    if (migration_is_running()) {
>> +        error_setg(errp, "Config synchronization is not allowed "
>> +                   "during migration.");
> 
> qapi/error.h:
> 
>       * The resulting message should be a single phrase, with no newline or
>       * trailing punctuation.
> 
> Drop the period, please.

Will do

> 
>> +        return;
>> +    }
>> +
>> +    if (!runstate_is_running()) {
>> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
>> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
>> +                   current_run_state_str());
>> +        return;
>> +    }
>> +
>> +    dev = find_device_state(id, true, errp);
>> +    if (!dev) {
>> +        return;
>> +    }
>> +
>> +    qdev_sync_config(dev, errp);
>> +}
>> +
>>   void hmp_device_add(Monitor *mon, const QDict *qdict)
>>   {
>>       Error *err = NULL;
>> diff --git a/system/runstate.c b/system/runstate.c
>> index d6ab860eca..8fd89172ae 100644
>> --- a/system/runstate.c
>> +++ b/system/runstate.c
>> @@ -189,6 +189,11 @@ bool runstate_check(RunState state)
>>       return current_run_state == state;
>>   }
>>   
>> +const char *current_run_state_str(void)
>> +{
>> +    return RunState_str(current_run_state);
>> +}
>> +
> 
> This helper is used just once.  Suggest to use
> RunState_str(runstate_get()) instead.

OK

> 
>>   static void runstate_init(void)
>>   {
>>       const RunStateTransition *p;
> 

-- 
Best regards,
Vladimir

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 24.04.24 14:48, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> Add command to sync config from vhost-user backend to the device. It
>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>> triggered interrupt to the guest or just not available (not supported
>>> by vhost-user server).
>>>
>>> Command result is racy if allow it during migration. Let's allow the
>>> sync only in RUNNING state.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[...]

>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>> index 0117d243c4..296af52322 100644
>>> --- a/include/sysemu/runstate.h
>>> +++ b/include/sysemu/runstate.h
>>> @@ -5,6 +5,7 @@
>>>   #include "qemu/notify.h"
>>>   
>>>   bool runstate_check(RunState state);
>>> +const char *current_run_state_str(void);
>>>   void runstate_set(RunState new_state);
>>>   RunState runstate_get(void);
>>>   bool runstate_is_running(void);
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index facaa0bc6a..e8be79c3d5 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -161,3 +161,24 @@
>>>   ##
>>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>     'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>> +##
>>> +# @device-sync-config:
>>> +#
>>> +# Synchronize config from backend to the guest. The command notifies
>>> +# re-read the device config from the backend and notifies the guest
>>> +# to re-read the config. The command may be used to notify the guest
>>> +# about block device capcity change. Currently only vhost-user-blk
>>> +# device supports this.
>> 
>> I'm not sure I understand this.  To work towards an understanding, I
>> rephrase it, and you point out the errors.
>> 
>>       Synchronize device configuration from host to guest part.  First,
>>       copy the configuration from the host part (backend) to the guest
>>       part (frontend).  Then notify guest software that device
>>       configuration changed.
>
> Correct, thanks

Perhaps

  Synchronize guest-visible device configuration with the backend's
  configuration, and notify guest software that device configuration
  changed.

  This may be useful to notify the guest of a block device capacity
  change.  Currenrly, only vhost-user-blk devices support this.

Next question: what happens when the device *doesn't* support this?

>> I wonder how configuration can get out of sync.  Can you explain?
>> 
>
> The example (and the original feature, which triggered developing this) is vhost disk resize. If vhost-server (backend) doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that disk capacity changed.

Sounds like we wouldn't need this command if we could make the
vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG.  Is making it
support it impractical?  Or are there other uses for this command?

>>> +#
>>> +# @id: the device's ID or QOM path
>>> +#
>>> +# Features:
>>> +#
>>> +# @unstable: The command is experimental.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'command': 'device-sync-config',
>>> +  'features': [ 'unstable' ],
>>> +  'data': {'id': 'str'} }
>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>> index 7e075d91c1..cb35ea0b86 100644
>>> --- a/system/qdev-monitor.c
>>> +++ b/system/qdev-monitor.c
>>> @@ -23,6 +23,7 @@
>>>  #include "monitor/monitor.h"
>>>  #include "monitor/qdev.h"
>>>  #include "sysemu/arch_init.h"
>>> +#include "sysemu/runstate.h"
>>>  #include "qapi/error.h"
>>>  #include "qapi/qapi-commands-qdev.h"
>>>  #include "qapi/qmp/dispatch.h"
>>> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>>>       }
>>>   }
>>>   
>>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>>> +{
>>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>> +
>>> +    if (!dc->sync_config) {
>>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>>> +                   object_get_typename(OBJECT(dev)));
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    return dc->sync_config(dev, errp);
>>> +}
>>> +
>>> +void qmp_device_sync_config(const char *id, Error **errp)
>>> +{
>>> +    DeviceState *dev;
>>> +
>>> +    /*
>>> +     * During migration there is a race between syncing`config and
>>> +     * migrating it, so let's just not allow it.
>> 
>> Can you briefly explain the race?
>
> If at the moment of qmp command, corresponding config already migrated to the target, we'll change only the config on source, but on the target we'll still have outdated config.

For RAM, dirty tracking ensures the change gets sent.  But this is
device memory.  Correct?

>>> +     *
>>> +     * Moreover, let's not rely on setting up interrupts in paused
>>> +     * state, which may be a part of migration process.
>> 
>> What dependence exactly are you avoiding?  Config synchronization
>> depending on guest interrupt delivery?
>
> Right, guest is notified by pci_set_irq.

If we allowed it in paused state, the delivery of the interrupt would be
delayed until the guest resumes running.  Correct?

>>> +     */
>>> +
>>> +    if (migration_is_running()) {
>>> +        error_setg(errp, "Config synchronization is not allowed "
>>> +                   "during migration.");
>> 
>> qapi/error.h:
>> 
>>       * The resulting message should be a single phrase, with no newline or
>>       * trailing punctuation.
>> 
>> Drop the period, please.
>
> Will do
>
>> 
>>> +        return;
>>> +    }
>>> +
>>> +    if (!runstate_is_running()) {
>>> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
>>> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
>>> +                   current_run_state_str());
>>> +        return;
>>> +    }
>>> +
>>> +    dev = find_device_state(id, true, errp);
>>> +    if (!dev) {
>>> +        return;
>>> +    }
>>> +
>>> +    qdev_sync_config(dev, errp);
>>> +}
>>> +
>>>   void hmp_device_add(Monitor *mon, const QDict *qdict)
>>>   {
>>>       Error *err = NULL;

[...]

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 29.04.24 13:51, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 24.04.24 14:48, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>
>>>> Add command to sync config from vhost-user backend to the device. It
>>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>>> triggered interrupt to the guest or just not available (not supported
>>>> by vhost-user server).
>>>>
>>>> Command result is racy if allow it during migration. Let's allow the
>>>> sync only in RUNNING state.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> [...]
> 
>>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>>> index 0117d243c4..296af52322 100644
>>>> --- a/include/sysemu/runstate.h
>>>> +++ b/include/sysemu/runstate.h
>>>> @@ -5,6 +5,7 @@
>>>>    #include "qemu/notify.h"
>>>>    
>>>>    bool runstate_check(RunState state);
>>>> +const char *current_run_state_str(void);
>>>>    void runstate_set(RunState new_state);
>>>>    RunState runstate_get(void);
>>>>    bool runstate_is_running(void);
>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>> index facaa0bc6a..e8be79c3d5 100644
>>>> --- a/qapi/qdev.json
>>>> +++ b/qapi/qdev.json
>>>> @@ -161,3 +161,24 @@
>>>>    ##
>>>>    { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>>      'data': { '*device': 'str', 'path': 'str' } }
>>>> +
>>>> +##
>>>> +# @device-sync-config:
>>>> +#
>>>> +# Synchronize config from backend to the guest. The command notifies
>>>> +# re-read the device config from the backend and notifies the guest
>>>> +# to re-read the config. The command may be used to notify the guest
>>>> +# about block device capcity change. Currently only vhost-user-blk
>>>> +# device supports this.
>>>
>>> I'm not sure I understand this.  To work towards an understanding, I
>>> rephrase it, and you point out the errors.
>>>
>>>        Synchronize device configuration from host to guest part.  First,
>>>        copy the configuration from the host part (backend) to the guest
>>>        part (frontend).  Then notify guest software that device
>>>        configuration changed.
>>
>> Correct, thanks
> 
> Perhaps
> 
>    Synchronize guest-visible device configuration with the backend's
>    configuration, and notify guest software that device configuration
>    changed.
> 
>    This may be useful to notify the guest of a block device capacity
>    change.  Currenrly, only vhost-user-blk devices support this.

Sounds good

> 
> Next question: what happens when the device *doesn't* support this?

An error "device-sync-config is not supported ..."

> 
>>> I wonder how configuration can get out of sync.  Can you explain?
>>>
>>
>> The example (and the original feature, which triggered developing this) is vhost disk resize. If vhost-server (backend) doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that disk capacity changed.
> 
> Sounds like we wouldn't need this command if we could make the
> vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG.  Is making it
> support it impractical?  Or are there other uses for this command?

Qemu's internal vhost-server do support it. But that's not the only vhost-user server) So the command is useful for those servers which doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Note, that this message requires setting up additional channel of server -> client communication. That was the reason, why the "change-msg" solution was rejected in our downstream: it's safer to reuse existing channel (QMP), than to add and support an additional channel.

Also, the command may help to debug the system, when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG doesn't work for some reason.

> 
>>>> +#
>>>> +# @id: the device's ID or QOM path
>>>> +#
>>>> +# Features:
>>>> +#
>>>> +# @unstable: The command is experimental.
>>>> +#
>>>> +# Since: 9.1
>>>> +##
>>>> +{ 'command': 'device-sync-config',
>>>> +  'features': [ 'unstable' ],
>>>> +  'data': {'id': 'str'} }
>>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>>> index 7e075d91c1..cb35ea0b86 100644
>>>> --- a/system/qdev-monitor.c
>>>> +++ b/system/qdev-monitor.c
>>>> @@ -23,6 +23,7 @@
>>>>   #include "monitor/monitor.h"
>>>>   #include "monitor/qdev.h"
>>>>   #include "sysemu/arch_init.h"
>>>> +#include "sysemu/runstate.h"
>>>>   #include "qapi/error.h"
>>>>   #include "qapi/qapi-commands-qdev.h"
>>>>   #include "qapi/qmp/dispatch.h"
>>>> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>>>>        }
>>>>    }
>>>>    
>>>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>>> +
>>>> +    if (!dc->sync_config) {
>>>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>>>> +                   object_get_typename(OBJECT(dev)));
>>>> +        return -ENOTSUP;
>>>> +    }
>>>> +
>>>> +    return dc->sync_config(dev, errp);
>>>> +}
>>>> +
>>>> +void qmp_device_sync_config(const char *id, Error **errp)
>>>> +{
>>>> +    DeviceState *dev;
>>>> +
>>>> +    /*
>>>> +     * During migration there is a race between syncing`config and
>>>> +     * migrating it, so let's just not allow it.
>>>
>>> Can you briefly explain the race?
>>
>> If at the moment of qmp command, corresponding config already migrated to the target, we'll change only the config on source, but on the target we'll still have outdated config.
> 
> For RAM, dirty tracking ensures the change gets sent.  But this is
> device memory.  Correct?

Yes. It's stored in malloced buffer VirtIIODevice::config, and accessed through handlers virtio_pci_config_read()/virtio_pci_config_write(). As I understand, no kind of dirty tracking here..

And I see, it's migrated in virtio_save():
...
     qemu_put_be32(f, vdev->config_len);
     qemu_put_buffer(f, vdev->config, vdev->config_len);
...

> 
>>>> +     *
>>>> +     * Moreover, let's not rely on setting up interrupts in paused
>>>> +     * state, which may be a part of migration process.
>>>
>>> What dependence exactly are you avoiding?  Config synchronization
>>> depending on guest interrupt delivery?
>>
>> Right, guest is notified by pci_set_irq.
> 
> If we allowed it in paused state, the delivery of the interrupt would be
> delayed until the guest resumes running.  Correct?

I think so. But this will not work, if we do offline migration like pause -> migrate -> resume on target. So I decided that better be more safe. The restrictions may be relaxed in future if needed.

> 
>>>> +     */
>>>> +
>>>> +    if (migration_is_running()) {
>>>> +        error_setg(errp, "Config synchronization is not allowed "
>>>> +                   "during migration.");
>>>
>>> qapi/error.h:
>>>
>>>        * The resulting message should be a single phrase, with no newline or
>>>        * trailing punctuation.
>>>
>>> Drop the period, please.
>>
>> Will do
>>
>>>
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (!runstate_is_running()) {
>>>> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
>>>> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
>>>> +                   current_run_state_str());
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    dev = find_device_state(id, true, errp);
>>>> +    if (!dev) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qdev_sync_config(dev, errp);
>>>> +}
>>>> +
>>>>    void hmp_device_add(Monitor *mon, const QDict *qdict)
>>>>    {
>>>>        Error *err = NULL;
> 
> [...]
> 

-- 
Best regards,
Vladimir

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 29.04.24 13:51, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> On 24.04.24 14:48, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>>
>>>>> Add command to sync config from vhost-user backend to the device. It
>>>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>>>> triggered interrupt to the guest or just not available (not supported
>>>>> by vhost-user server).
>>>>>
>>>>> Command result is racy if allow it during migration. Let's allow the
>>>>> sync only in RUNNING state.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> 
>> [...]
>> 
>>>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>>>> index 0117d243c4..296af52322 100644
>>>>> --- a/include/sysemu/runstate.h
>>>>> +++ b/include/sysemu/runstate.h
>>>>> @@ -5,6 +5,7 @@
>>>>>    #include "qemu/notify.h"
>>>>>    
>>>>>    bool runstate_check(RunState state);
>>>>> +const char *current_run_state_str(void);
>>>>>    void runstate_set(RunState new_state);
>>>>>    RunState runstate_get(void);
>>>>>    bool runstate_is_running(void);
>>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>>> index facaa0bc6a..e8be79c3d5 100644
>>>>> --- a/qapi/qdev.json
>>>>> +++ b/qapi/qdev.json
>>>>> @@ -161,3 +161,24 @@
>>>>>    ##
>>>>>    { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>>>      'data': { '*device': 'str', 'path': 'str' } }
>>>>> +
>>>>> +##
>>>>> +# @device-sync-config:
>>>>> +#
>>>>> +# Synchronize config from backend to the guest. The command notifies
>>>>> +# re-read the device config from the backend and notifies the guest
>>>>> +# to re-read the config. The command may be used to notify the guest
>>>>> +# about block device capcity change. Currently only vhost-user-blk
>>>>> +# device supports this.
>>>>
>>>> I'm not sure I understand this.  To work towards an understanding, I
>>>> rephrase it, and you point out the errors.
>>>>
>>>>        Synchronize device configuration from host to guest part.  First,
>>>>        copy the configuration from the host part (backend) to the guest
>>>>        part (frontend).  Then notify guest software that device
>>>>        configuration changed.
>>>
>>> Correct, thanks
>> 
>> Perhaps
>> 
>>    Synchronize guest-visible device configuration with the backend's
>>    configuration, and notify guest software that device configuration
>>    changed.
>> 
>>    This may be useful to notify the guest of a block device capacity
>>    change.  Currenrly, only vhost-user-blk devices support this.
>
> Sounds good

Except I fat-fingered "Currently".

>> 
>> Next question: what happens when the device *doesn't* support this?
>
> An error "device-sync-config is not supported ..."

Okay.

>>>> I wonder how configuration can get out of sync.  Can you explain?
>>>>
>>>
>>> The example (and the original feature, which triggered developing this) is vhost disk resize. If vhost-server (backend) doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that disk capacity changed.
>> 
>> Sounds like we wouldn't need this command if we could make the
>> vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG.  Is making it
>> support it impractical?  Or are there other uses for this command?
>
> Qemu's internal vhost-server do support it. But that's not the only vhost-user server) So the command is useful for those servers which doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Note, that this message requires setting up additional channel of server -> client communication. That was the reason, why the "change-msg" solution was rejected in our downstream: it's safer to reuse existing channel (QMP), than to add and support an additional channel.
>
> Also, the command may help to debug the system, when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG doesn't work for some reason.

Suggest to work this into the commit message.

>>>>> +#
>>>>> +# @id: the device's ID or QOM path
>>>>> +#
>>>>> +# Features:
>>>>> +#
>>>>> +# @unstable: The command is experimental.
>>>>> +#
>>>>> +# Since: 9.1
>>>>> +##
>>>>> +{ 'command': 'device-sync-config',
>>>>> +  'features': [ 'unstable' ],
>>>>> +  'data': {'id': 'str'} }
>>>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>>>> index 7e075d91c1..cb35ea0b86 100644
>>>>> --- a/system/qdev-monitor.c
>>>>> +++ b/system/qdev-monitor.c
>>>>> @@ -23,6 +23,7 @@
>>>>>   #include "monitor/monitor.h"
>>>>>   #include "monitor/qdev.h"
>>>>>   #include "sysemu/arch_init.h"
>>>>> +#include "sysemu/runstate.h"
>>>>>   #include "qapi/error.h"
>>>>>   #include "qapi/qapi-commands-qdev.h"
>>>>>   #include "qapi/qmp/dispatch.h"
>>>>> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>>>>>        }
>>>>>    }
>>>>>    
>>>>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>>>>> +{
>>>>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>>>> +
>>>>> +    if (!dc->sync_config) {
>>>>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>>>>> +                   object_get_typename(OBJECT(dev)));
>>>>> +        return -ENOTSUP;
>>>>> +    }
>>>>> +
>>>>> +    return dc->sync_config(dev, errp);
>>>>> +}
>>>>> +
>>>>> +void qmp_device_sync_config(const char *id, Error **errp)
>>>>> +{
>>>>> +    DeviceState *dev;
>>>>> +
>>>>> +    /*
>>>>> +     * During migration there is a race between syncing`config and
>>>>> +     * migrating it, so let's just not allow it.
>>>>
>>>> Can you briefly explain the race?
>>>
>>> If at the moment of qmp command, corresponding config already migrated to the target, we'll change only the config on source, but on the target we'll still have outdated config.
>> 
>> For RAM, dirty tracking ensures the change gets sent.  But this is
>> device memory.  Correct?
>
> Yes. It's stored in malloced buffer VirtIIODevice::config, and accessed through handlers virtio_pci_config_read()/virtio_pci_config_write(). As I understand, no kind of dirty tracking here..
>
> And I see, it's migrated in virtio_save():
> ...
>      qemu_put_be32(f, vdev->config_len);
>      qemu_put_buffer(f, vdev->config, vdev->config_len);
> ...

Suggest to explain the race in the comment.  Perhaps like this:

  Guest-visible configuration is stored in device memory.  There is a
  race between updating and migrating it: if we update it before we
  migrate it, it's migrated fine, but if any later updates are lost.

>>>>> +     *
>>>>> +     * Moreover, let's not rely on setting up interrupts in paused
>>>>> +     * state, which may be a part of migration process.
>>>>
>>>> What dependence exactly are you avoiding?  Config synchronization
>>>> depending on guest interrupt delivery?
>>>
>>> Right, guest is notified by pci_set_irq.
>> 
>> If we allowed it in paused state, the delivery of the interrupt would be
>> delayed until the guest resumes running.  Correct?
>
> I think so. But this will not work, if we do offline migration like pause -> migrate -> resume on target. So I decided that better be more safe. The restrictions may be relaxed in future if needed.

Sounds like we'd make an interrupt pending on the source, but migration
failed to make it pending on the target as well, so it gets lost.

Is that the case?

If yes, question for migration experts: why isn't this a problem
elsewhere, too?

[...]

Thanks!

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 29.04.24 16:04, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 29.04.24 13:51, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>
>>>> On 24.04.24 14:48, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>>>
>>>>>> Add command to sync config from vhost-user backend to the device. It
>>>>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>>>>> triggered interrupt to the guest or just not available (not supported
>>>>>> by vhost-user server).
>>>>>>
>>>>>> Command result is racy if allow it during migration. Let's allow the
>>>>>> sync only in RUNNING state.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>
>>> [...]
>>>
>>>>>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>>>>>> index 0117d243c4..296af52322 100644
>>>>>> --- a/include/sysemu/runstate.h
>>>>>> +++ b/include/sysemu/runstate.h
>>>>>> @@ -5,6 +5,7 @@
>>>>>>     #include "qemu/notify.h"
>>>>>>     
>>>>>>     bool runstate_check(RunState state);
>>>>>> +const char *current_run_state_str(void);
>>>>>>     void runstate_set(RunState new_state);
>>>>>>     RunState runstate_get(void);
>>>>>>     bool runstate_is_running(void);
>>>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>>>> index facaa0bc6a..e8be79c3d5 100644
>>>>>> --- a/qapi/qdev.json
>>>>>> +++ b/qapi/qdev.json
>>>>>> @@ -161,3 +161,24 @@
>>>>>>     ##
>>>>>>     { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>>>>       'data': { '*device': 'str', 'path': 'str' } }
>>>>>> +
>>>>>> +##
>>>>>> +# @device-sync-config:
>>>>>> +#
>>>>>> +# Synchronize config from backend to the guest. The command notifies
>>>>>> +# re-read the device config from the backend and notifies the guest
>>>>>> +# to re-read the config. The command may be used to notify the guest
>>>>>> +# about block device capcity change. Currently only vhost-user-blk
>>>>>> +# device supports this.
>>>>>
>>>>> I'm not sure I understand this.  To work towards an understanding, I
>>>>> rephrase it, and you point out the errors.
>>>>>
>>>>>         Synchronize device configuration from host to guest part.  First,
>>>>>         copy the configuration from the host part (backend) to the guest
>>>>>         part (frontend).  Then notify guest software that device
>>>>>         configuration changed.
>>>>
>>>> Correct, thanks
>>>
>>> Perhaps
>>>
>>>     Synchronize guest-visible device configuration with the backend's
>>>     configuration, and notify guest software that device configuration
>>>     changed.
>>>
>>>     This may be useful to notify the guest of a block device capacity
>>>     change.  Currenrly, only vhost-user-blk devices support this.
>>
>> Sounds good
> 
> Except I fat-fingered "Currently".
> 
>>>
>>> Next question: what happens when the device *doesn't* support this?
>>
>> An error "device-sync-config is not supported ..."
> 
> Okay.
> 
>>>>> I wonder how configuration can get out of sync.  Can you explain?
>>>>>
>>>>
>>>> The example (and the original feature, which triggered developing this) is vhost disk resize. If vhost-server (backend) doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG, neither QEMU nor guest will know that disk capacity changed.
>>>
>>> Sounds like we wouldn't need this command if we could make the
>>> vhost-server support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG.  Is making it
>>> support it impractical?  Or are there other uses for this command?
>>
>> Qemu's internal vhost-server do support it. But that's not the only vhost-user server) So the command is useful for those servers which doesn't support VHOST_USER_SLAVE_CONFIG_CHANGE_MSG. Note, that this message requires setting up additional channel of server -> client communication. That was the reason, why the "change-msg" solution was rejected in our downstream: it's safer to reuse existing channel (QMP), than to add and support an additional channel.
>>
>> Also, the command may help to debug the system, when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG doesn't work for some reason.
> 
> Suggest to work this into the commit message.
> 
>>>>>> +#
>>>>>> +# @id: the device's ID or QOM path
>>>>>> +#
>>>>>> +# Features:
>>>>>> +#
>>>>>> +# @unstable: The command is experimental.
>>>>>> +#
>>>>>> +# Since: 9.1
>>>>>> +##
>>>>>> +{ 'command': 'device-sync-config',
>>>>>> +  'features': [ 'unstable' ],
>>>>>> +  'data': {'id': 'str'} }
>>>>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>>>>>> index 7e075d91c1..cb35ea0b86 100644
>>>>>> --- a/system/qdev-monitor.c
>>>>>> +++ b/system/qdev-monitor.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>    #include "monitor/monitor.h"
>>>>>>    #include "monitor/qdev.h"
>>>>>>    #include "sysemu/arch_init.h"
>>>>>> +#include "sysemu/runstate.h"
>>>>>>    #include "qapi/error.h"
>>>>>>    #include "qapi/qapi-commands-qdev.h"
>>>>>>    #include "qapi/qmp/dispatch.h"
>>>>>> @@ -969,6 +970,52 @@ void qmp_device_del(const char *id, Error **errp)
>>>>>>         }
>>>>>>     }
>>>>>>     
>>>>>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>>>>>> +{
>>>>>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>>>>> +
>>>>>> +    if (!dc->sync_config) {
>>>>>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>>>>>> +                   object_get_typename(OBJECT(dev)));
>>>>>> +        return -ENOTSUP;
>>>>>> +    }
>>>>>> +
>>>>>> +    return dc->sync_config(dev, errp);
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_device_sync_config(const char *id, Error **errp)
>>>>>> +{
>>>>>> +    DeviceState *dev;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * During migration there is a race between syncing`config and
>>>>>> +     * migrating it, so let's just not allow it.
>>>>>
>>>>> Can you briefly explain the race?
>>>>
>>>> If at the moment of qmp command, corresponding config already migrated to the target, we'll change only the config on source, but on the target we'll still have outdated config.
>>>
>>> For RAM, dirty tracking ensures the change gets sent.  But this is
>>> device memory.  Correct?
>>
>> Yes. It's stored in malloced buffer VirtIIODevice::config, and accessed through handlers virtio_pci_config_read()/virtio_pci_config_write(). As I understand, no kind of dirty tracking here..
>>
>> And I see, it's migrated in virtio_save():
>> ...
>>       qemu_put_be32(f, vdev->config_len);
>>       qemu_put_buffer(f, vdev->config, vdev->config_len);
>> ...
> 
> Suggest to explain the race in the comment.  Perhaps like this:
> 
>    Guest-visible configuration is stored in device memory.  There is a
>    race between updating and migrating it: if we update it before we
>    migrate it, it's migrated fine, but if any later updates are lost.
> 
>>>>>> +     *
>>>>>> +     * Moreover, let's not rely on setting up interrupts in paused
>>>>>> +     * state, which may be a part of migration process.
>>>>>
>>>>> What dependence exactly are you avoiding?  Config synchronization
>>>>> depending on guest interrupt delivery?
>>>>
>>>> Right, guest is notified by pci_set_irq.
>>>
>>> If we allowed it in paused state, the delivery of the interrupt would be
>>> delayed until the guest resumes running.  Correct?
>>
>> I think so. But this will not work, if we do offline migration like pause -> migrate -> resume on target. So I decided that better be more safe. The restrictions may be relaxed in future if needed.
> 
> Sounds like we'd make an interrupt pending on the source, but migration
> failed to make it pending on the target as well, so it gets lost.
> 
> Is that the case?

Yes, I mean exactly this thing. But probably I was wrong with my doubt, as queued interrupts do migrate:

virtio_save() {
...
qemu_put_8s(f, &vdev->isr);
...


Hmm. And I don't remember now, was it just theoretical doubt, or a real problem. I'll check.

> 
> If yes, question for migration experts: why isn't this a problem
> elsewhere, too?
> 
> [...]
> 
> Thanks!
> 

-- 
Best regards,
Vladimir

Re: [PATCH v3 4/5] qapi: introduce device-sync-config

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[...]

> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9228e96c87..87135bdcdf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
>  typedef void (*DeviceReset)(DeviceState *dev);
>  typedef void (*BusRealize)(BusState *bus, Error **errp);
>  typedef void (*BusUnrealize)(BusState *bus);
> +typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
>  
>  /**
>   * struct DeviceClass - The base class for all devices.
> @@ -162,6 +163,7 @@ struct DeviceClass {
>      DeviceReset reset;
>      DeviceRealize realize;
>      DeviceUnrealize unrealize;
> +    DeviceSyncConfig sync_config;

I get

    include/hw/qdev-core.h:179: warning: Function parameter or member 'sync_config' not described in 'DeviceClass'

To fix this, cover the new member in the doc comment.

>  
>      /**
>       * @vmsd: device state serialisation description for

[...]

[PATCH v3 5/5] qapi: introduce CONFIG_READ event

[Vladimir Sementsov-Ogievskiy]

Send a new event when guest reads virtio-pci config after
virtio_notify_config() call.

That's useful to check that guest fetched modified config, for example
after resizing disk backend.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/virtio/virtio-pci.c |  9 +++++++++
 include/monitor/qdev.h |  2 ++
 monitor/monitor.c      |  1 +
 qapi/qdev.json         | 33 +++++++++++++++++++++++++++++++++
 stubs/qdev.c           |  6 ++++++
 system/qdev-monitor.c  |  6 ++++++
 6 files changed, 57 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 92afbae71c..c0c158dae2 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -23,6 +23,7 @@
 #include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
+#include "monitor/qdev.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/qdev-properties.h"
@@ -530,6 +531,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
     }
     addr -= config;
 
+    if (vdev->generation > 0) {
+        qdev_virtio_config_read_event(DEVICE(proxy));
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_readb(vdev, addr);
@@ -1884,6 +1889,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
         return UINT64_MAX;
     }
 
+    if (vdev->generation > 0) {
+        qdev_virtio_config_read_event(DEVICE(proxy));
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_modern_readb(vdev, addr);
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 1d57bf6577..fc9a834dca 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
  */
 const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
+void qdev_virtio_config_read_event(DeviceState *dev);
+
 #endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 01ede1babd..5b06146503 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -316,6 +316,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
     [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
     [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
+    [QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },
 };
 
 /*
diff --git a/qapi/qdev.json b/qapi/qdev.json
index e8be79c3d5..29a4f47360 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -182,3 +182,36 @@
 { 'command': 'device-sync-config',
   'features': [ 'unstable' ],
   'data': {'id': 'str'} }
+
+##
+# @VIRTIO_CONFIG_READ:
+#
+# Emitted whenever guest reads virtio device configuration after
+# configuration change.
+#
+# The event may be used in pair with device-sync-config. It shows
+# that guest has re-read updated configuration. It doesn't
+# guarantee that guest successfully handled it and updated the
+# view of the device for the user, but still it's a kind of
+# success indicator.
+#
+# @device: device name
+#
+# @path: device path
+#
+# Features:
+#
+# @unstable: The event is experimental.
+#
+# Since: 9.1
+#
+# Example:
+#
+#     <- { "event": "VIRTIO_CONFIG_READ",
+#          "data": { "device": "virtio-net-pci-0",
+#                    "path": "/machine/peripheral/virtio-net-pci-0" },
+#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+##
+{ 'event': 'VIRTIO_CONFIG_READ',
+  'features': [ 'unstable' ],
+  'data': { '*device': 'str', 'path': 'str' } }
diff --git a/stubs/qdev.c b/stubs/qdev.c
index 6869f6f90a..ab6c4afe0b 100644
--- a/stubs/qdev.c
+++ b/stubs/qdev.c
@@ -26,3 +26,9 @@ void qapi_event_send_device_unplug_guest_error(const char *device,
 {
     /* Nothing to do. */
 }
+
+void qapi_event_send_virtio_config_read(const char *device,
+                                        const char *path)
+{
+    /* Nothing to do. */
+}
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index cb35ea0b86..8a2ca77fde 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -26,6 +26,7 @@
 #include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
+#include "qapi/qapi-events-qdev.h"
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
@@ -1206,3 +1207,8 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
     }
     return true;
 }
+
+void qdev_virtio_config_read_event(DeviceState *dev)
+{
+    qapi_event_send_virtio_config_read(dev->id, dev->canonical_path);
+}
-- 
2.34.1

Re: [PATCH v3 5/5] qapi: introduce CONFIG_READ event

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Send a new event when guest reads virtio-pci config after
> virtio_notify_config() call.
>
> That's useful to check that guest fetched modified config, for example
> after resizing disk backend.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/virtio/virtio-pci.c |  9 +++++++++
>  include/monitor/qdev.h |  2 ++
>  monitor/monitor.c      |  1 +
>  qapi/qdev.json         | 33 +++++++++++++++++++++++++++++++++
>  stubs/qdev.c           |  6 ++++++
>  system/qdev-monitor.c  |  6 ++++++
>  6 files changed, 57 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 92afbae71c..c0c158dae2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -23,6 +23,7 @@
>  #include "hw/boards.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
> +#include "monitor/qdev.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/qdev-properties.h"
> @@ -530,6 +531,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>      }
>      addr -= config;
>  
> +    if (vdev->generation > 0) {
> +        qdev_virtio_config_read_event(DEVICE(proxy));
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_readb(vdev, addr);
> @@ -1884,6 +1889,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
>          return UINT64_MAX;
>      }
>  
> +    if (vdev->generation > 0) {
> +        qdev_virtio_config_read_event(DEVICE(proxy));
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_modern_readb(vdev, addr);
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 1d57bf6577..fc9a834dca 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>   */
>  const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>  
> +void qdev_virtio_config_read_event(DeviceState *dev);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 01ede1babd..5b06146503 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -316,6 +316,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>      [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
>      [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
> +    [QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },

All the other rate-limited events use 1s.  Why 0.3s for this one?

>  };
>  
>  /*
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index e8be79c3d5..29a4f47360 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -182,3 +182,36 @@
>  { 'command': 'device-sync-config',
>    'features': [ 'unstable' ],
>    'data': {'id': 'str'} }
> +
> +##
> +# @VIRTIO_CONFIG_READ:
> +#
> +# Emitted whenever guest reads virtio device configuration after
> +# configuration change.

Is it emitted whenever the guest reads, or only when it reads after a
configuration change?

> +#
> +# The event may be used in pair with device-sync-config. It shows
> +# that guest has re-read updated configuration. It doesn't
> +# guarantee that guest successfully handled it and updated the
> +# view of the device for the user, but still it's a kind of
> +# success indicator.

The event is virtio-only.  device-sync-config isn't.  Why?

> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Features:
> +#
> +# @unstable: The event is experimental.
> +#

Missing:

   # Note: This event is rate-limited.
   #

> +# Since: 9.1
> +#
> +# Example:
> +#
> +#     <- { "event": "VIRTIO_CONFIG_READ",
> +#          "data": { "device": "virtio-net-pci-0",
> +#                    "path": "/machine/peripheral/virtio-net-pci-0" },
> +#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +##
> +{ 'event': 'VIRTIO_CONFIG_READ',
> +  'features': [ 'unstable' ],
> +  'data': { '*device': 'str', 'path': 'str' } }
> diff --git a/stubs/qdev.c b/stubs/qdev.c
> index 6869f6f90a..ab6c4afe0b 100644
> --- a/stubs/qdev.c
> +++ b/stubs/qdev.c
> @@ -26,3 +26,9 @@ void qapi_event_send_device_unplug_guest_error(const char *device,
>  {
>      /* Nothing to do. */
>  }
> +
> +void qapi_event_send_virtio_config_read(const char *device,
> +                                        const char *path)
> +{
> +    /* Nothing to do. */
> +}
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index cb35ea0b86..8a2ca77fde 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -26,6 +26,7 @@
>  #include "sysemu/runstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
> @@ -1206,3 +1207,8 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>      }
>      return true;
>  }
> +
> +void qdev_virtio_config_read_event(DeviceState *dev)
> +{
> +    qapi_event_send_virtio_config_read(dev->id, dev->canonical_path);
> +}

Which configuration needs the stub?

Re: [PATCH v3 5/5] qapi: introduce CONFIG_READ event

[Vladimir Sementsov-Ogievskiy]

On 24.04.24 15:11, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Send a new event when guest reads virtio-pci config after
>> virtio_notify_config() call.
>>
>> That's useful to check that guest fetched modified config, for example
>> after resizing disk backend.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/virtio/virtio-pci.c |  9 +++++++++
>>   include/monitor/qdev.h |  2 ++
>>   monitor/monitor.c      |  1 +
>>   qapi/qdev.json         | 33 +++++++++++++++++++++++++++++++++
>>   stubs/qdev.c           |  6 ++++++
>>   system/qdev-monitor.c  |  6 ++++++
>>   6 files changed, 57 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 92afbae71c..c0c158dae2 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/boards.h"
>>   #include "hw/virtio/virtio.h"
>>   #include "migration/qemu-file-types.h"
>> +#include "monitor/qdev.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/qdev-properties.h"
>> @@ -530,6 +531,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>>       }
>>       addr -= config;
>>   
>> +    if (vdev->generation > 0) {
>> +        qdev_virtio_config_read_event(DEVICE(proxy));
>> +    }
>> +
>>       switch (size) {
>>       case 1:
>>           val = virtio_config_readb(vdev, addr);
>> @@ -1884,6 +1889,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
>>           return UINT64_MAX;
>>       }
>>   
>> +    if (vdev->generation > 0) {
>> +        qdev_virtio_config_read_event(DEVICE(proxy));
>> +    }
>> +
>>       switch (size) {
>>       case 1:
>>           val = virtio_config_modern_readb(vdev, addr);
>> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
>> index 1d57bf6577..fc9a834dca 100644
>> --- a/include/monitor/qdev.h
>> +++ b/include/monitor/qdev.h
>> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>>    */
>>   const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>>   
>> +void qdev_virtio_config_read_event(DeviceState *dev);
>> +
>>   #endif
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 01ede1babd..5b06146503 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -316,6 +316,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>>       [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
>> +    [QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },
> 
> All the other rate-limited events use 1s.  Why 0.3s for this one?

No actual reason, just seemed to me that 1s is too much. Should be better to keep all limits to be the same, until no concrete reason to break it.

> 
>>   };
>>   
>>   /*
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index e8be79c3d5..29a4f47360 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -182,3 +182,36 @@
>>   { 'command': 'device-sync-config',
>>     'features': [ 'unstable' ],
>>     'data': {'id': 'str'} }
>> +
>> +##
>> +# @VIRTIO_CONFIG_READ:
>> +#
>> +# Emitted whenever guest reads virtio device configuration after
>> +# configuration change.
> 
> Is it emitted whenever the guest reads, or only when it reads after a
> configuration change?

Hmm, it's emitted only when vdev->generation > 0, which generally mean that there was at least one call to virtio_notify_config()... That's not the logic, which could be simply described here.


Actually, now I think that event was a premature improvement. In our final downstream solution only the command device-sync-config is used, not the event. I see that the concept of the event is objectionable, I think, I'll better just drop it in v4.

> 
>> +#
>> +# The event may be used in pair with device-sync-config. It shows
>> +# that guest has re-read updated configuration. It doesn't
>> +# guarantee that guest successfully handled it and updated the
>> +# view of the device for the user, but still it's a kind of
>> +# success indicator.
> 
> The event is virtio-only.  device-sync-config isn't.  Why?
> 
>> +#
>> +# @device: device name
>> +#
>> +# @path: device path
>> +#
>> +# Features:
>> +#
>> +# @unstable: The event is experimental.
>> +#
> 
> Missing:
> 
>     # Note: This event is rate-limited.
>     #
> 
>> +# Since: 9.1
>> +#
>> +# Example:
>> +#
>> +#     <- { "event": "VIRTIO_CONFIG_READ",
>> +#          "data": { "device": "virtio-net-pci-0",
>> +#                    "path": "/machine/peripheral/virtio-net-pci-0" },
>> +#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +##
>> +{ 'event': 'VIRTIO_CONFIG_READ',
>> +  'features': [ 'unstable' ],
>> +  'data': { '*device': 'str', 'path': 'str' } }
>> diff --git a/stubs/qdev.c b/stubs/qdev.c
>> index 6869f6f90a..ab6c4afe0b 100644
>> --- a/stubs/qdev.c
>> +++ b/stubs/qdev.c
>> @@ -26,3 +26,9 @@ void qapi_event_send_device_unplug_guest_error(const char *device,
>>   {
>>       /* Nothing to do. */
>>   }
>> +
>> +void qapi_event_send_virtio_config_read(const char *device,
>> +                                        const char *path)
>> +{
>> +    /* Nothing to do. */
>> +}
>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> index cb35ea0b86..8a2ca77fde 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -26,6 +26,7 @@
>>   #include "sysemu/runstate.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-qdev.h"
>> +#include "qapi/qapi-events-qdev.h"
>>   #include "qapi/qmp/dispatch.h"
>>   #include "qapi/qmp/qdict.h"
>>   #include "qapi/qmp/qerror.h"
>> @@ -1206,3 +1207,8 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>>       }
>>       return true;
>>   }
>> +
>> +void qdev_virtio_config_read_event(DeviceState *dev)
>> +{
>> +    qapi_event_send_virtio_config_read(dev->id, dev->canonical_path);
>> +}
> 
> Which configuration needs the stub?
> 

-- 
Best regards,
Vladimir