[PATCH v2 0/6] vhost-user-blk: live resize additional APIs

[PATCH v2 0/6] vhost-user-blk: live resize additional APIs

[Vladimir Sementsov-Ogievskiy]

v2:
- now based on master
- drop x- prefixes, still keep new APIs "unstable"

Also:

01: add a-b by Raphael
    add note about removed comment "valid for resize only"
02-03: new patches, following review of old "02", which is now 04
04: use GenericError
    wording
    keep short name device-sync-config still
06: rename event to VIRTIO_CONFIG_READ

===

In vhost-user protocol we have VHOST_USER_BACKEND_CONFIG_CHANGE_MSG,
which backend may send to notify Qemu, that we should re-read the
config, and notify the guest.

Still that's not always convenient: backend may not support this
message. Also, having QMP command to force config sync is more reliable
than waiting for notification from external program. It also may be
helpful for debug/restore: if we have changed disk size, but guest
doesn't see that, it's good to have a separate QMP command to trigger
resync of the config.

So, the series proposes two experimental APIs:

1. device-sync-config command, to trigger config synchronization

2. CONFIG_READ event, which notify management tool that guest read the
updated config. Of course, that can't guarantee that the guest correctly
handled the updated config, but it's still better than nothing: for sure
guest will not show new disk size until it read the updated config. So,
management tool may wait for this event to report success to the user.

Vladimir Sementsov-Ogievskiy (6):
  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: device-sync-config: check runstate
  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            | 43 ++++++++++++++++++++++++
 stubs/qdev.c              |  6 ++++
 system/qdev-monitor.c     | 71 ++++++++++++++++++++++++++++++++++++---
 system/runstate.c         |  5 +++
 10 files changed, 165 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] 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 v2 2/6] 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>
---
 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 a13db763e5..9febb743f1 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -890,7 +890,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 v2 3/6] qdev-monitor: add option to report GenericError from find_device_state

[Vladimir Sementsov-Ogievskiy]

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 9febb743f1..cf7481e416 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -877,13 +877,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;
     }
@@ -947,7 +954,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 ||
@@ -1067,7 +1074,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 v2 4/6] 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).

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 +++
 qapi/qdev.json            | 17 +++++++++++++++++
 system/qdev-monitor.c     | 23 +++++++++++++++++++++++
 5 files changed, 72 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 1a7039fb0c..1197341a3c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2318,6 +2318,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);
@@ -2334,6 +2342,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/qapi/qdev.json b/qapi/qdev.json
index 32ffaee644..6ece164172 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -162,3 +162,20 @@
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @device-sync-config:
+#
+# Synchronize config from backend to the guest.
+#
+# @id: the device's ID or QOM path
+#
+# Features:
+#
+# @unstable: The command is experimental.
+#
+# Since: 9.0
+##
+{ 'command': 'device-sync-config',
+  'features': [ 'unstable' ],
+  'data': {'id': 'str'} }
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index cf7481e416..e3107a12d7 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -968,6 +968,29 @@ 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 = 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;
-- 
2.34.1

[PATCH v2 5/6] qapi: device-sync-config: check runstate

[Vladimir Sementsov-Ogievskiy]

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>
---
 include/sysemu/runstate.h |  1 +
 system/qdev-monitor.c     | 27 ++++++++++++++++++++++++++-
 system/runstate.c         |  5 +++++
 3 files changed, 32 insertions(+), 1 deletion(-)

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/system/qdev-monitor.c b/system/qdev-monitor.c
index e3107a12d7..b83b5d23c9 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"
@@ -983,7 +984,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
 
 void qmp_device_sync_config(const char *id, Error **errp)
 {
-    DeviceState *dev = find_device_state(id, true, errp);
+    MigrationState *s = migrate_get_current();
+    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(s->state)) {
+        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;
     }
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

[PATCH v2 6/6] 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         | 26 ++++++++++++++++++++++++++
 stubs/qdev.c           |  6 ++++++
 system/qdev-monitor.c  |  6 ++++++
 6 files changed, 50 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 1197341a3c..fab49a2410 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);
@@ -1735,6 +1740,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 6ece164172..ffc5e3be18 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -179,3 +179,29 @@
 { 'command': 'device-sync-config',
   'features': [ 'unstable' ],
   'data': {'id': 'str'} }
+
+##
+# @VIRTIO_CONFIG_READ:
+#
+# Emitted whenever guest reads virtio device config after config change.
+#
+# @device: device name
+#
+# @path: device path
+#
+# Features:
+#
+# @unstable: The event is experimental.
+#
+# Since: 9.0
+#
+# 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 b83b5d23c9..29b8242a2d 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 v2 2/6] qdev-monitor: fix error message in find_device_state()

[Markus Armbruster]

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

> 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>

"Suggested"

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  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 a13db763e5..9febb743f1 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -890,7 +890,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;
>      }

Perhaps including what @id resolved to could be useful, but that's
out of scope for this patch.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state

[Markus Armbruster]

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

> 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 9febb743f1..cf7481e416 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -877,13 +877,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;
>      }
> @@ -947,7 +954,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 ||
> @@ -1067,7 +1074,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;
>      }

I appreciate the attempt to curb the spread of DeviceNotFound errors.
Two issues:

* Copy-pasting find_device_state() with a false argument is an easy
  error to make.

* Most uses of find_device_state() are via blk_by_qdev_id() and
  qmp_get_blk().  Any new uses of qemu_get_blk() will still produce
  DeviceNotFound.

Hmm.

Re: [PATCH v2 4/6] 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).
>
> 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 +++
>  qapi/qdev.json            | 17 +++++++++++++++++
>  system/qdev-monitor.c     | 23 +++++++++++++++++++++++
>  5 files changed, 72 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 1a7039fb0c..1197341a3c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2318,6 +2318,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);
> @@ -2334,6 +2342,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/qapi/qdev.json b/qapi/qdev.json
> index 32ffaee644..6ece164172 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -162,3 +162,20 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @device-sync-config:
> +#
> +# Synchronize config from backend to the guest.
> +#
> +# @id: the device's ID or QOM path
> +#
> +# Features:
> +#
> +# @unstable: The command is experimental.
> +#
> +# Since: 9.0
> +##

Even for an experimental command a bit more guidance on intended would
be good.

The commit message describes this as a "command to sync config from
vhost-user backend to the device".  Here, there's no mention of
vhost-user.

Could the command make sense for devices other than vhost-user?

> +{ 'command': 'device-sync-config',
> +  'features': [ 'unstable' ],
> +  'data': {'id': 'str'} }
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index cf7481e416..e3107a12d7 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -968,6 +968,29 @@ 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 = 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 v2 5/6] qapi: device-sync-config: check runstate

[Markus Armbruster]

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

> 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>

If I understand this correctly, the previous commit introduces a race,
and this one fixes it.

We normally avoid such temporary bugs.  When avoidance is impractical,
we point them out in commit message and FIXME comment.

> ---
>  include/sysemu/runstate.h |  1 +
>  system/qdev-monitor.c     | 27 ++++++++++++++++++++++++++-
>  system/runstate.c         |  5 +++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> 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/system/qdev-monitor.c b/system/qdev-monitor.c
> index e3107a12d7..b83b5d23c9 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"
> @@ -983,7 +984,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
>  
>  void qmp_device_sync_config(const char *id, Error **errp)
>  {
> -    DeviceState *dev = find_device_state(id, true, errp);
> +    MigrationState *s = migrate_get_current();
> +    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.

Wrap your comment lines around column 70, please.

> +     */
> +
> +    if (migration_is_running(s->state)) {
> +        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;
>      }
> 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;

Re: [PATCH v2 6/6] 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>

[...]

> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 6ece164172..ffc5e3be18 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -179,3 +179,29 @@
>  { 'command': 'device-sync-config',
>    'features': [ 'unstable' ],
>    'data': {'id': 'str'} }
> +
> +##
> +# @VIRTIO_CONFIG_READ:
> +#
> +# Emitted whenever guest reads virtio device config after config change.

Let's not abbreviate "configuration" to "config".

> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Features:
> +#
> +# @unstable: The event is experimental.
> +#
> +# Since: 9.0
> +#
> +# Example:
> +#
> +#     <- { "event": "VIRTIO_CONFIG_READ",
> +#          "data": { "device": "virtio-net-pci-0",
> +#                    "path": "/machine/peripheral/virtio-net-pci-0" },
> +#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +##

As for PATCH 4, I'd like to see some guidance on intended use.

> +{ 'event': 'VIRTIO_CONFIG_READ',
> +  'features': [ 'unstable' ],
> +  'data': { '*device': 'str', 'path': 'str' } }

[...]

Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state

[Vladimir Sementsov-Ogievskiy]

On 07.03.24 12:46, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> 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 9febb743f1..cf7481e416 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -877,13 +877,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;
>>       }
>> @@ -947,7 +954,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 ||
>> @@ -1067,7 +1074,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;
>>       }
> 
> I appreciate the attempt to curb the spread of DeviceNotFound errors.
> Two issues:
> 
> * Copy-pasting find_device_state() with a false argument is an easy
>    error to make.
> 
> * Most uses of find_device_state() are via blk_by_qdev_id() and
>    qmp_get_blk().  Any new uses of qemu_get_blk() will still produce
>    DeviceNotFound.
> 
> Hmm.

Hmm. Right. Wait a bit, I can make the change stricter.

Could you still explain (or give a link), why and when we decided to use only GenericError? I think, having different "error-codes" is a good thing, why we are trying to get rid of it?

-- 
Best regards,
Vladimir

Re: [PATCH v2 4/6] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 07.03.24 12:54, 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).
>>
>> 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 +++
>>   qapi/qdev.json            | 17 +++++++++++++++++
>>   system/qdev-monitor.c     | 23 +++++++++++++++++++++++
>>   5 files changed, 72 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 1a7039fb0c..1197341a3c 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2318,6 +2318,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);
>> @@ -2334,6 +2342,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/qapi/qdev.json b/qapi/qdev.json
>> index 32ffaee644..6ece164172 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -162,3 +162,20 @@
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>     'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @device-sync-config:
>> +#
>> +# Synchronize config from backend to the guest.
>> +#
>> +# @id: the device's ID or QOM path
>> +#
>> +# Features:
>> +#
>> +# @unstable: The command is experimental.
>> +#
>> +# Since: 9.0
>> +##
> 
> Even for an experimental command a bit more guidance on intended would
> be good.

Will add

> 
> The commit message describes this as a "command to sync config from
> vhost-user backend to the device".  Here, there's no mention of
> vhost-user.
> 
> Could the command make sense for devices other than vhost-user?

Yes it should work for any kind of backend, which can't notify Qemu about changed configuration (like disk size).

> 
>> +{ 'command': 'device-sync-config',
>> +  'features': [ 'unstable' ],
>> +  'data': {'id': 'str'} }
>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> index cf7481e416..e3107a12d7 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -968,6 +968,29 @@ 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 = 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 v2 5/6] qapi: device-sync-config: check runstate

[Vladimir Sementsov-Ogievskiy]

On 07.03.24 12:57, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> 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>
> 
> If I understand this correctly, the previous commit introduces a race,
> and this one fixes it.

Sure, I will merge it. I recently argued against such things, bad for me to break the rule myself)

> 
> We normally avoid such temporary bugs.  When avoidance is impractical,
> we point them out in commit message and FIXME comment.
> >> ---
>>   include/sysemu/runstate.h |  1 +
>>   system/qdev-monitor.c     | 27 ++++++++++++++++++++++++++-
>>   system/runstate.c         |  5 +++++
>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> 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/system/qdev-monitor.c b/system/qdev-monitor.c
>> index e3107a12d7..b83b5d23c9 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"
>> @@ -983,7 +984,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
>>   
>>   void qmp_device_sync_config(const char *id, Error **errp)
>>   {
>> -    DeviceState *dev = find_device_state(id, true, errp);
>> +    MigrationState *s = migrate_get_current();
>> +    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.
> 
> Wrap your comment lines around column 70, please.

OK.

I never remember this recommendation.. Probably we'd better have a check in checkpatch for it

> 
>> +     */
>> +
>> +    if (migration_is_running(s->state)) {
>> +        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;
>>       }
>> 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;
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 6/6] qapi: introduce CONFIG_READ event

[Vladimir Sementsov-Ogievskiy]

On 07.03.24 13:01, 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>
> 
> [...]
> 
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 6ece164172..ffc5e3be18 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -179,3 +179,29 @@
>>   { 'command': 'device-sync-config',
>>     'features': [ 'unstable' ],
>>     'data': {'id': 'str'} }
>> +
>> +##
>> +# @VIRTIO_CONFIG_READ:
>> +#
>> +# Emitted whenever guest reads virtio device config after config change.
> 
> Let's not abbreviate "configuration" to "config".

Oops, me again.

> 
>> +#
>> +# @device: device name
>> +#
>> +# @path: device path
>> +#
>> +# Features:
>> +#
>> +# @unstable: The event is experimental.
>> +#
>> +# Since: 9.0
>> +#
>> +# Example:
>> +#
>> +#     <- { "event": "VIRTIO_CONFIG_READ",
>> +#          "data": { "device": "virtio-net-pci-0",
>> +#                    "path": "/machine/peripheral/virtio-net-pci-0" },
>> +#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +##
> 
> As for PATCH 4, I'd like to see some guidance on intended use.

Will do

> 
>> +{ 'event': 'VIRTIO_CONFIG_READ',
>> +  'features': [ 'unstable' ],
>> +  'data': { '*device': 'str', 'path': 'str' } }
> 
> [...]
> 

Thanks for reviewing my patches!

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state

[Vladimir Sementsov-Ogievskiy]

On 07.03.24 12:46, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> 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 9febb743f1..cf7481e416 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -877,13 +877,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;
>>       }
>> @@ -947,7 +954,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 ||
>> @@ -1067,7 +1074,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;
>>       }
> 
> I appreciate the attempt to curb the spread of DeviceNotFound errors.
> Two issues:
> 
> * Copy-pasting find_device_state() with a false argument is an easy
>    error to make.
> 
> * Most uses of find_device_state() are via blk_by_qdev_id() and
>    qmp_get_blk().  Any new uses of qemu_get_blk() will still produce
>    DeviceNotFound.
> 

Hm. But what to do?

- rename all three functions to FOO_base(), and add a boolean parameter
- add FOO() wrappers, passing true (use generic error)
- add FOO_deprecated() wrappers, passing false (use device not found error)
- change existing callers to use FOO_deprecated()

Still, new generic-error-based blk_by_qdev_id() and qmp_get_blk() will be unused..

That seem too ugly for me. And that doesn't give 100% sure that nobody will simply duplicate call to _deprecated() function...

Maybe better don't care for now (and continue use device-not-found for new APIs, that reuse find_device_state()), and just declare the deprecation for ERROR_CLASS_DEVICE_NOT_FOUND? And drop it usage everywhere after 3-releases deprecation cycle.

Or do we want deprecate everything except for generic-error, and deprecate error/class field in qmp return value altogether?

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state

[Markus Armbruster]

Sorry for the late answer.

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

> On 07.03.24 12:46, Markus Armbruster wrote:

[...]

>> I appreciate the attempt to curb the spread of DeviceNotFound errors.
>> Two issues:
>>
>> * Copy-pasting find_device_state() with a false argument is an easy
>>   error to make.
>>
>> * Most uses of find_device_state() are via blk_by_qdev_id() and
>>   qmp_get_blk().  Any new uses of qemu_get_blk() will still produce
>>   DeviceNotFound.
>>
>> Hmm.
>
> Hmm. Right. Wait a bit, I can make the change stricter.
>
> Could you still explain (or give a link), why and when we decided to use only GenericError? I think, having different "error-codes" is a good thing, why we are trying to get rid of it?

We actually got rid of most of them years ago :)

But you deserve a more complete answer.

QMP initially specified the following error response[1]:

    2.4.2 error
    -----------
    
    The error response is issued when the command execution could not be
    completed because of an error condition.
    
    The format is:
    
    { "error": { "class": json-string, "data": json-value }, "id": json-value }
    
     Where,
    
    - The "class" member contains the error class name (eg. "ServiceUnavailable")
    - The "data" member contains specific error data and is defined in a
      per-command basis, it will be an empty json-object if the error has no data
    - The "id" member contains the transaction identification associated with
      the command execution (if issued by the Client)

Note the structure of @data depends on @class.  We documented a
command's possible error classes (well, we tried), but never bothered to
document the @data it comes with.

Documentation deficits aside, this is looks quite expressive.  There are
issues, though:

1. Formatting errors in human-readable form is bothersome, and creates a
   tight coupling between QMP server and client.

   Consider:

    {"class": "DeviceNotFound", "data": {"device": "ide1-cd0"}}

   To format this in human-readable form, you need to know the error.

   The server does.  Fine print: it has a table mapping JSON templates
   to human-readable error message templates.

   The client needs to duplicate this somehow.  If it receives an error
   it doesn't know, all it can do is barf (possibly dressed up) JSON at
   the human.  To avoid that, clients need to track the server closely:
   tight coupling.

2. Errors have notational overhead, which leads to bad errors.

   To create a new error, you have to edit two source files (not
   counting clients).  Strong incentive to reuse existing errors.  Even
   when they don't quite fit.  When a name provided by the user couldn't
   be resolved, reusing DeviceNotFound is easier than creating a new
   error that is more precise.

3. The human-readable error message is hidden from the programmer's
   view, which leads to bad error messages.

   At the point in the source where the error is created, we see
   something like QERR_DEVICE_NOT_FOUND, name.  To see the
   human-readable message, we have to look up macro
   QERR_DEVICE_NOT_FOUND's error message template in the table, or
   actually test (*gasp*) the error.  People generally do neither, and
   bad error messages proliferate.

4. Too little gain for the pain

   Clients rarely want to handle different errors differently.  More
   often than not, all they do with @class and @data is log them.  Only
   occasionally do they switch on @class, and basically never on @data.

It me took a considerable fight to get the protocol amended to include a
human-readable message[2].  This addressed issue 1.

Over the next two years or so, issues 2. to 4. slowly sank in.  We
eventually tired of the complexity, ripped out @data, and dumbed down
all error classes to GenericError, except for the few clients actually
cared for[3].  We also mandated that new errors avoid the QERR_ macros.

Eliminating the existing QERR_ macros has been slow.  We're down to 13
in master, with patches deleting 7 on the list.

This has served us reasonably well.

Questions?


[1] Commit f544d174dfc
    QMP: Introduce specification
    Dec 2009

[2] Commit 77e595e7c61q
    QMP: add human-readable description to error response
    Dec 2009

[3] Commit de253f14912
    qmp: switch to the new error format on the wire
    Aug 2012

Re: [PATCH v2 3/6] qdev-monitor: add option to report GenericError from find_device_state

[Vladimir Sementsov-Ogievskiy]

On 15.03.24 15:51, Markus Armbruster wrote:
> Sorry for the late answer.
> 
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 07.03.24 12:46, Markus Armbruster wrote:
> 
> [...]
> 
>>> I appreciate the attempt to curb the spread of DeviceNotFound errors.
>>> Two issues:
>>>
>>> * Copy-pasting find_device_state() with a false argument is an easy
>>>    error to make.
>>>
>>> * Most uses of find_device_state() are via blk_by_qdev_id() and
>>>    qmp_get_blk().  Any new uses of qemu_get_blk() will still produce
>>>    DeviceNotFound.
>>>
>>> Hmm.
>>
>> Hmm. Right. Wait a bit, I can make the change stricter.
>>
>> Could you still explain (or give a link), why and when we decided to use only GenericError? I think, having different "error-codes" is a good thing, why we are trying to get rid of it?
> 
> We actually got rid of most of them years ago :)
> 
> But you deserve a more complete answer.
> 
> QMP initially specified the following error response[1]:
> 
>      2.4.2 error
>      -----------
>      
>      The error response is issued when the command execution could not be
>      completed because of an error condition.
>      
>      The format is:
>      
>      { "error": { "class": json-string, "data": json-value }, "id": json-value }
>      
>       Where,
>      
>      - The "class" member contains the error class name (eg. "ServiceUnavailable")
>      - The "data" member contains specific error data and is defined in a
>        per-command basis, it will be an empty json-object if the error has no data
>      - The "id" member contains the transaction identification associated with
>        the command execution (if issued by the Client)
> 
> Note the structure of @data depends on @class.  We documented a
> command's possible error classes (well, we tried), but never bothered to
> document the @data it comes with.
> 
> Documentation deficits aside, this is looks quite expressive.  There are
> issues, though:
> 
> 1. Formatting errors in human-readable form is bothersome, and creates a
>     tight coupling between QMP server and client.
> 
>     Consider:
> 
>      {"class": "DeviceNotFound", "data": {"device": "ide1-cd0"}}
> 
>     To format this in human-readable form, you need to know the error.
> 
>     The server does.  Fine print: it has a table mapping JSON templates
>     to human-readable error message templates.
> 
>     The client needs to duplicate this somehow.  If it receives an error
>     it doesn't know, all it can do is barf (possibly dressed up) JSON at
>     the human.  To avoid that, clients need to track the server closely:
>     tight coupling.
> 
> 2. Errors have notational overhead, which leads to bad errors.
> 
>     To create a new error, you have to edit two source files (not
>     counting clients).  Strong incentive to reuse existing errors.  Even
>     when they don't quite fit.  When a name provided by the user couldn't
>     be resolved, reusing DeviceNotFound is easier than creating a new
>     error that is more precise.
> 
> 3. The human-readable error message is hidden from the programmer's
>     view, which leads to bad error messages.
> 
>     At the point in the source where the error is created, we see
>     something like QERR_DEVICE_NOT_FOUND, name.  To see the
>     human-readable message, we have to look up macro
>     QERR_DEVICE_NOT_FOUND's error message template in the table, or
>     actually test (*gasp*) the error.  People generally do neither, and
>     bad error messages proliferate.
> 
> 4. Too little gain for the pain
> 
>     Clients rarely want to handle different errors differently.  More
>     often than not, all they do with @class and @data is log them.  Only
>     occasionally do they switch on @class, and basically never on @data.
> 
> It me took a considerable fight to get the protocol amended to include a
> human-readable message[2].  This addressed issue 1.
> 
> Over the next two years or so, issues 2. to 4. slowly sank in.  We
> eventually tired of the complexity, ripped out @data, and dumbed down
> all error classes to GenericError, except for the few clients actually
> cared for[3].  We also mandated that new errors avoid the QERR_ macros.
> 
> Eliminating the existing QERR_ macros has been slow.  We're down to 13
> in master, with patches deleting 7 on the list.
> 
> This has served us reasonably well.
> 
> Questions?
> 
> 
> [1] Commit f544d174dfc
>      QMP: Introduce specification
>      Dec 2009
> 
> [2] Commit 77e595e7c61q
>      QMP: add human-readable description to error response
>      Dec 2009
> 
> [3] Commit de253f14912
>      qmp: switch to the new error format on the wire
>      Aug 2012
> 

Thanks for full-picture story! Now I'm all for it.

-- 
Best regards,
Vladimir