[PATCH v2 0/8] vhost-user-blk: dynamically resize config space based on features

[PATCH v2 0/8] vhost-user-blk: dynamically resize config space based on features

[Daniil Tatianin]

This patch set attempts to align vhost-user-blk with virtio-blk in
terms of backward compatibility and flexibility. It also improves
the virtio core by introducing new common code that can be used by
a virtio device to calculate its config space size.

In particular it adds the following things:
- Common virtio code for deducing the required device config size based
  on provided host features.
- Ability to disable modern virtio-blk features like
  discard/write-zeroes for vhost-user-blk.
- Dynamic configuration space resizing based on enabled features,
  by reusing the common code introduced in the earlier commits.
- Cleans up the VHostUserBlk structure by reusing parent fields.

Changes since v1 (mostly addresses Stefan's feedback):
- Introduce VirtIOConfigSizeParams & virtio_get_config_size
- Remove virtio_blk_set_config_size altogether, make virtio-blk-common.c
  only hold the virtio-blk config size parameters.
- Reuse parent fields in vhost-user-blk instead of introducing new ones.

Daniil Tatianin (8):
  virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size
  virtio-blk: utilize VirtIOConfigSizeParams & virtio_get_config_size
  virtio-net: utilize VirtIOConfigSizeParams & virtio_get_config_size
  virtio: remove the virtio_feature_get_config_size helper
  virtio-blk: move config size params to virtio-blk-common
  vhost-user-blk: make it possible to disable write-zeroes/discard
  vhost-user-blk: make 'config_wce' part of 'host_features'
  vhost-user-blk: dynamically resize config space based on features

 MAINTAINERS                           |  4 +++
 hw/block/meson.build                  |  4 +--
 hw/block/vhost-user-blk.c             | 29 +++++++++++---------
 hw/block/virtio-blk-common.c          | 39 +++++++++++++++++++++++++++
 hw/block/virtio-blk.c                 | 28 +++----------------
 hw/net/virtio-net.c                   |  8 ++++--
 hw/virtio/virtio.c                    | 10 ++++---
 include/hw/virtio/vhost-user-blk.h    |  1 -
 include/hw/virtio/virtio-blk-common.h | 20 ++++++++++++++
 include/hw/virtio/virtio.h            | 10 +++++--
 10 files changed, 104 insertions(+), 49 deletions(-)
 create mode 100644 hw/block/virtio-blk-common.c
 create mode 100644 include/hw/virtio/virtio-blk-common.h

-- 
2.25.1

[PATCH v2 1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size

[Daniil Tatianin]

This is the first step towards moving all device config size calculation
logic into the virtio core code. In particular, this adds a struct that
contains all the necessary information for common virtio code to be able
to calculate the final config size for a device. This is expected to be
used with the new virtio_get_config_size helper, which calculates the
final length based on the provided host features.

This builds on top of already existing code like VirtIOFeature and
virtio_feature_get_config_size(), but adds additional fields, as well as
sanity checking so that device-specifc code doesn't have to duplicate it.

An example usage would be:

    static const VirtIOFeature dev_features[] = {
        {.flags = 1ULL << FEATURE_1_BIT,
         .end = endof(struct virtio_dev_config, feature_1)},
        {.flags = 1ULL << FEATURE_2_BIT,
         .end = endof(struct virtio_dev_config, feature_2)},
        {}
    };

    static const VirtIOConfigSizeParams dev_cfg_size_params = {
        .min_size = DEV_BASE_CONFIG_SIZE,
        .max_size = sizeof(struct virtio_dev_config),
        .feature_sizes = dev_features
    };

    // code inside my_dev_device_realize()
    size_t config_size = virtio_get_config_size(&dev_cfg_size_params,
                                                host_features);
    virtio_init(vdev, VIRTIO_ID_MYDEV, config_size);

Currently every device is expected to write its own boilerplate from the
example above in device_realize(), however, the next step of this
transition is moving VirtIOConfigSizeParams into VirtioDeviceClass,
so that it can be done automatically by the virtio initialization code.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/virtio/virtio.c         | 17 +++++++++++++++++
 include/hw/virtio/virtio.h |  9 +++++++++
 2 files changed, 26 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5d607aeaa0..8518382025 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3014,6 +3014,23 @@ size_t virtio_feature_get_config_size(const VirtIOFeature *feature_sizes,
     return config_size;
 }
 
+size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
+                              uint64_t host_features)
+{
+    size_t config_size = params->min_size;
+    const VirtIOFeature *feature_sizes = params->feature_sizes;
+    size_t i;
+
+    for (i = 0; feature_sizes[i].flags != 0; i++) {
+        if (host_features & feature_sizes[i].flags) {
+            config_size = MAX(feature_sizes[i].end, config_size);
+        }
+    }
+
+    assert(config_size <= params->max_size);
+    return config_size;
+}
+
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 {
     int i, ret;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..1991c58d9b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -44,6 +44,15 @@ typedef struct VirtIOFeature {
     size_t end;
 } VirtIOFeature;
 
+typedef struct VirtIOConfigSizeParams {
+    size_t min_size;
+    size_t max_size;
+    const VirtIOFeature *feature_sizes;
+} VirtIOConfigSizeParams;
+
+size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
+                              uint64_t host_features);
+
 size_t virtio_feature_get_config_size(const VirtIOFeature *features,
                                       uint64_t host_features);
 
-- 
2.25.1

[PATCH v2 2/8] virtio-blk: utilize VirtIOConfigSizeParams & virtio_get_config_size

[Daniil Tatianin]

Use the common helper instead of duplicating the same logic.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/block/virtio-blk.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e9ba752f6b..10c47c2934 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -49,13 +49,11 @@ static const VirtIOFeature feature_sizes[] = {
     {}
 };
 
-static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
-{
-    s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
-        virtio_feature_get_config_size(feature_sizes, host_features));
-
-    assert(s->config_size <= sizeof(struct virtio_blk_config));
-}
+static const VirtIOConfigSizeParams cfg_size_params = {
+    .min_size = VIRTIO_BLK_CFG_SIZE,
+    .max_size = sizeof(struct virtio_blk_config),
+    .feature_sizes = feature_sizes
+};
 
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
@@ -1204,8 +1202,8 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_blk_set_config_size(s, s->host_features);
-
+    s->config_size = virtio_get_config_size(&cfg_size_params,
+                                            s->host_features);
     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
 
     s->blk = conf->conf.blk;
-- 
2.25.1

[PATCH v2 3/8] virtio-net: utilize VirtIOConfigSizeParams & virtio_get_config_size

[Daniil Tatianin]

Use the new common helper. As an added bonus this also makes use of
config size sanity checking via the 'max_size' field.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/net/virtio-net.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..dfc8dd8562 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -106,6 +106,11 @@ static const VirtIOFeature feature_sizes[] = {
     {}
 };
 
+static const VirtIOConfigSizeParams cfg_size_params = {
+    .max_size = sizeof(struct virtio_net_config),
+    .feature_sizes = feature_sizes
+};
+
 static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
 {
     VirtIONet *n = qemu_get_nic_opaque(nc);
@@ -3246,8 +3251,7 @@ static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
 {
     virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
 
-    n->config_size = virtio_feature_get_config_size(feature_sizes,
-                                                    host_features);
+    n->config_size = virtio_get_config_size(&cfg_size_params, host_features);
 }
 
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
-- 
2.25.1

[PATCH v2 4/8] virtio: remove the virtio_feature_get_config_size helper

[Daniil Tatianin]

This has no more users and is superseded by virtio_get_config_size.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/virtio/virtio.c         | 15 ---------------
 include/hw/virtio/virtio.h |  3 ---
 2 files changed, 18 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8518382025..c0bf8dd6fd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2999,21 +2999,6 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
     return ret;
 }
 
-size_t virtio_feature_get_config_size(const VirtIOFeature *feature_sizes,
-                                      uint64_t host_features)
-{
-    size_t config_size = 0;
-    int i;
-
-    for (i = 0; feature_sizes[i].flags != 0; i++) {
-        if (host_features & feature_sizes[i].flags) {
-            config_size = MAX(feature_sizes[i].end, config_size);
-        }
-    }
-
-    return config_size;
-}
-
 size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
                               uint64_t host_features)
 {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 1991c58d9b..f3ff6710d5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -53,9 +53,6 @@ typedef struct VirtIOConfigSizeParams {
 size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
                               uint64_t host_features);
 
-size_t virtio_feature_get_config_size(const VirtIOFeature *features,
-                                      uint64_t host_features);
-
 typedef struct VirtQueue VirtQueue;
 
 #define VIRTQUEUE_MAX_SIZE 1024
-- 
2.25.1

[PATCH v2 5/8] virtio-blk: move config size params to virtio-blk-common

[Daniil Tatianin]

This way we can reuse it for other virtio-blk devices, e.g
vhost-user-blk, which currently does not control its config space size
dynamically.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 MAINTAINERS                           |  4 +++
 hw/block/meson.build                  |  4 +--
 hw/block/virtio-blk-common.c          | 39 +++++++++++++++++++++++++++
 hw/block/virtio-blk.c                 | 24 ++---------------
 include/hw/virtio/virtio-blk-common.h | 20 ++++++++++++++
 5 files changed, 67 insertions(+), 24 deletions(-)
 create mode 100644 hw/block/virtio-blk-common.c
 create mode 100644 include/hw/virtio/virtio-blk-common.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 5ce4227ff6..a7d3914735 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2030,8 +2030,10 @@ virtio-blk
 M: Stefan Hajnoczi <stefanha@redhat.com>
 L: qemu-block@nongnu.org
 S: Supported
+F: hw/block/virtio-blk-common.c
 F: hw/block/virtio-blk.c
 F: hw/block/dataplane/*
+F: include/hw/virtio/virtio-blk-common.h
 F: tests/qtest/virtio-blk-test.c
 T: git https://github.com/stefanha/qemu.git block
 
@@ -2271,11 +2273,13 @@ S: Maintained
 F: contrib/vhost-user-blk/
 F: contrib/vhost-user-scsi/
 F: hw/block/vhost-user-blk.c
+F: hw/block/virtio-blk-common.c
 F: hw/scsi/vhost-user-scsi.c
 F: hw/virtio/vhost-user-blk-pci.c
 F: hw/virtio/vhost-user-scsi-pci.c
 F: include/hw/virtio/vhost-user-blk.h
 F: include/hw/virtio/vhost-user-scsi.h
+F: include/hw/virtio/virtio-blk-common.h
 
 vhost-user-gpu
 M: Marc-André Lureau <marcandre.lureau@redhat.com>
diff --git a/hw/block/meson.build b/hw/block/meson.build
index 2389326112..1908abd45c 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -16,7 +16,7 @@ softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
 softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
 softmmu_ss.add(when: 'CONFIG_TC58128', if_true: files('tc58128.c'))
 
-specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
-specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c'))
+specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c', 'virtio-blk-common.c'))
+specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c', 'virtio-blk-common.c'))
 
 subdir('dataplane')
diff --git a/hw/block/virtio-blk-common.c b/hw/block/virtio-blk-common.c
new file mode 100644
index 0000000000..ac52d7c176
--- /dev/null
+++ b/hw/block/virtio-blk-common.c
@@ -0,0 +1,39 @@
+/*
+ * Virtio Block Device common helpers
+ *
+ * Copyright IBM, Corp. 2007
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "standard-headers/linux/virtio_blk.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-blk-common.h"
+
+/* Config size before the discard support (hide associated config fields) */
+#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
+                                     max_discard_sectors)
+
+/*
+ * Starting from the discard feature, we can use this array to properly
+ * set the config size depending on the features enabled.
+ */
+static const VirtIOFeature feature_sizes[] = {
+    {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
+     .end = endof(struct virtio_blk_config, discard_sector_alignment)},
+    {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
+     .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
+    {}
+};
+
+const VirtIOConfigSizeParams virtio_blk_cfg_size_params = {
+    .min_size = VIRTIO_BLK_CFG_SIZE,
+    .max_size = sizeof(struct virtio_blk_config),
+    .feature_sizes = feature_sizes
+};
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 10c47c2934..8131ec2dbc 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -32,29 +32,9 @@
 #include "hw/virtio/virtio-bus.h"
 #include "migration/qemu-file-types.h"
 #include "hw/virtio/virtio-access.h"
+#include "hw/virtio/virtio-blk-common.h"
 #include "qemu/coroutine.h"
 
-/* Config size before the discard support (hide associated config fields) */
-#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
-                                     max_discard_sectors)
-/*
- * Starting from the discard feature, we can use this array to properly
- * set the config size depending on the features enabled.
- */
-static const VirtIOFeature feature_sizes[] = {
-    {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
-     .end = endof(struct virtio_blk_config, discard_sector_alignment)},
-    {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
-     .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
-    {}
-};
-
-static const VirtIOConfigSizeParams cfg_size_params = {
-    .min_size = VIRTIO_BLK_CFG_SIZE,
-    .max_size = sizeof(struct virtio_blk_config),
-    .feature_sizes = feature_sizes
-};
-
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
@@ -1202,7 +1182,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    s->config_size = virtio_get_config_size(&cfg_size_params,
+    s->config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
                                             s->host_features);
     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
 
diff --git a/include/hw/virtio/virtio-blk-common.h b/include/hw/virtio/virtio-blk-common.h
new file mode 100644
index 0000000000..31daada3e3
--- /dev/null
+++ b/include/hw/virtio/virtio-blk-common.h
@@ -0,0 +1,20 @@
+/*
+ * Virtio Block Device common helpers
+ *
+ * Copyright IBM, Corp. 2007
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef VIRTIO_BLK_COMMON_H
+#define VIRTIO_BLK_COMMON_H
+
+#include "hw/virtio/virtio.h"
+
+extern const VirtIOConfigSizeParams virtio_blk_cfg_size_params;
+
+#endif
-- 
2.25.1

[PATCH v2 6/8] vhost-user-blk: make it possible to disable write-zeroes/discard

[Daniil Tatianin]

It is useful to have the ability to disable these features for
compatibility with older VMs that don't have these implemented.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/block/vhost-user-blk.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..4c9727e08c 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -259,8 +259,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
     virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
     virtio_add_feature(&features, VIRTIO_BLK_F_RO);
-    virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
-    virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
 
     if (s->config_wce) {
         virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
@@ -592,6 +590,10 @@ static Property vhost_user_blk_properties[] = {
                        VHOST_USER_BLK_AUTO_NUM_QUEUES),
     DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
     DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
+    DEFINE_PROP_BIT64("discard", VHostUserBlk, parent_obj.host_features,
+                      VIRTIO_BLK_F_DISCARD, true),
+    DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, parent_obj.host_features,
+                      VIRTIO_BLK_F_WRITE_ZEROES, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.25.1

[PATCH v2 7/8] vhost-user-blk: make 'config_wce' part of 'host_features'

[Daniil Tatianin]

No reason to have this be a separate field. This also makes it more akin
to what the virtio-blk device does.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/block/vhost-user-blk.c          | 6 ++----
 include/hw/virtio/vhost-user-blk.h | 1 -
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 4c9727e08c..0d916edefa 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -260,9 +260,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
     virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
     virtio_add_feature(&features, VIRTIO_BLK_F_RO);
 
-    if (s->config_wce) {
-        virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-    }
     if (s->num_queues > 1) {
         virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
     }
@@ -589,7 +586,8 @@ static Property vhost_user_blk_properties[] = {
     DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
                        VHOST_USER_BLK_AUTO_NUM_QUEUES),
     DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
-    DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
+    DEFINE_PROP_BIT64("config-wce", VHostUserBlk, parent_obj.host_features,
+                      VIRTIO_BLK_F_CONFIG_WCE, true),
     DEFINE_PROP_BIT64("discard", VHostUserBlk, parent_obj.host_features,
                       VIRTIO_BLK_F_DISCARD, true),
     DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, parent_obj.host_features,
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 7c91f15040..ea085ee1ed 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -34,7 +34,6 @@ struct VHostUserBlk {
     struct virtio_blk_config blkcfg;
     uint16_t num_queues;
     uint32_t queue_size;
-    uint32_t config_wce;
     struct vhost_dev dev;
     struct vhost_inflight *inflight;
     VhostUserState vhost_user;
-- 
2.25.1

[PATCH v2 8/8] vhost-user-blk: dynamically resize config space based on features

[Daniil Tatianin]

Make vhost-user-blk backwards compatible when migrating from older VMs
running with modern features turned off, the same way it was done for
virtio-blk in 20764be0421c ("virtio-blk: set config size depending on the features enabled")

It's currently impossible to migrate from an older VM with
vhost-user-blk (with disable-legacy=off) because of errors like this:

qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: 41 device: 1 cmask: ff wmask: 80 w1cmask:0
qemu-system-x86_64: Failed to load PCIDevice:config
qemu-system-x86_64: Failed to load virtio-blk:virtio
qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:05.0:00.0:02.0/virtio-blk'
qemu-system-x86_64: load of migration failed: Invalid argument

This is caused by the newer (destination) VM requiring a bigger BAR0
alignment because it has to cover a bigger configuration space, which
isn't actually needed since those additional config fields are not
active (write-zeroes/discard).

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 hw/block/vhost-user-blk.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 0d916edefa..8dd063eb96 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -23,6 +23,7 @@
 #include "hw/qdev-core.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
+#include "hw/virtio/virtio-blk-common.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user-blk.h"
 #include "hw/virtio/virtio.h"
@@ -63,7 +64,7 @@ static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     /* Our num_queues overrides the device backend */
     virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);
 
-    memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
+    memcpy(config, &s->blkcfg, vdev->config_len);
 }
 
 static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -92,12 +93,12 @@ 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;
 
     ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
-                               sizeof(struct virtio_blk_config),
-                               &local_err);
+                               vdev->config_len, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
@@ -106,7 +107,7 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
     /* valid for resize only */
     if (blkcfg.capacity != s->blkcfg.capacity) {
         s->blkcfg.capacity = blkcfg.capacity;
-        memcpy(dev->vdev->config, &s->blkcfg, sizeof(struct virtio_blk_config));
+        memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
         virtio_notify_config(dev->vdev);
     }
 
@@ -442,7 +443,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
     assert(s->connected);
 
     ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
-                               sizeof(struct virtio_blk_config), errp);
+                               s->parent_obj.config_len, errp);
     if (ret < 0) {
         qemu_chr_fe_disconnect(&s->chardev);
         vhost_dev_cleanup(&s->dev);
@@ -457,6 +458,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
     ERRP_GUARD();
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostUserBlk *s = VHOST_USER_BLK(vdev);
+    size_t config_size;
     int retries;
     int i, ret;
 
@@ -487,8 +489,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_init(vdev, VIRTIO_ID_BLOCK,
-                sizeof(struct virtio_blk_config));
+    config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
+                                         vdev->host_features);
+    virtio_init(vdev, VIRTIO_ID_BLOCK, config_size);
 
     s->virtqs = g_new(VirtQueue *, s->num_queues);
     for (i = 0; i < s->num_queues; i++) {
-- 
2.25.1

Re: [PATCH v2 0/8] vhost-user-blk: dynamically resize config space based on features

[Raphael Norwitz]

> ping

Apologies for the late review - busy week. First pass looks good but will review
comprehensively tomorrow or over the weekend.

Re: [PATCH v2 1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size

[Raphael Norwitz]

I feel like it would be easier to review if the first 4 patches were
squashed together, but that’s not a big deal.

For this one:

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

On Fri, Aug 26, 2022 at 05:32:41PM +0300, Daniil Tatianin wrote:
> This is the first step towards moving all device config size calculation
> logic into the virtio core code. In particular, this adds a struct that
> contains all the necessary information for common virtio code to be able
> to calculate the final config size for a device. This is expected to be
> used with the new virtio_get_config_size helper, which calculates the
> final length based on the provided host features.
> 
> This builds on top of already existing code like VirtIOFeature and
> virtio_feature_get_config_size(), but adds additional fields, as well as
> sanity checking so that device-specifc code doesn't have to duplicate it.
> 
> An example usage would be:
> 
>     static const VirtIOFeature dev_features[] = {
>         {.flags = 1ULL << FEATURE_1_BIT,
>          .end = endof(struct virtio_dev_config, feature_1)},
>         {.flags = 1ULL << FEATURE_2_BIT,
>          .end = endof(struct virtio_dev_config, feature_2)},
>         {}
>     };
> 
>     static const VirtIOConfigSizeParams dev_cfg_size_params = {
>         .min_size = DEV_BASE_CONFIG_SIZE,
>         .max_size = sizeof(struct virtio_dev_config),
>         .feature_sizes = dev_features
>     };
> 
>     // code inside my_dev_device_realize()
>     size_t config_size = virtio_get_config_size(&dev_cfg_size_params,
>                                                 host_features);
>     virtio_init(vdev, VIRTIO_ID_MYDEV, config_size);
> 
> Currently every device is expected to write its own boilerplate from the
> example above in device_realize(), however, the next step of this
> transition is moving VirtIOConfigSizeParams into VirtioDeviceClass,
> so that it can be done automatically by the virtio initialization code.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  hw/virtio/virtio.c         | 17 +++++++++++++++++
>  include/hw/virtio/virtio.h |  9 +++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5d607aeaa0..8518382025 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3014,6 +3014,23 @@ size_t virtio_feature_get_config_size(const VirtIOFeature *feature_sizes,
>      return config_size;
>  }
>  
> +size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
> +                              uint64_t host_features)
> +{
> +    size_t config_size = params->min_size;
> +    const VirtIOFeature *feature_sizes = params->feature_sizes;
> +    size_t i;
> +
> +    for (i = 0; feature_sizes[i].flags != 0; i++) {
> +        if (host_features & feature_sizes[i].flags) {
> +            config_size = MAX(feature_sizes[i].end, config_size);
> +        }
> +    }
> +
> +    assert(config_size <= params->max_size);
> +    return config_size;
> +}
> +
>  int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>  {
>      int i, ret;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index db1c0ddf6b..1991c58d9b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -44,6 +44,15 @@ typedef struct VirtIOFeature {
>      size_t end;
>  } VirtIOFeature;
>  
> +typedef struct VirtIOConfigSizeParams {
> +    size_t min_size;
> +    size_t max_size;
> +    const VirtIOFeature *feature_sizes;
> +} VirtIOConfigSizeParams;
> +
> +size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
> +                              uint64_t host_features);
> +
>  size_t virtio_feature_get_config_size(const VirtIOFeature *features,
>                                        uint64_t host_features);
>  
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/8] virtio-blk: utilize VirtIOConfigSizeParams & virtio_get_config_size

[Raphael Norwitz]

On Fri, Aug 26, 2022 at 05:32:42PM +0300, Daniil Tatianin wrote:
> Use the common helper instead of duplicating the same logic.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

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

> ---
>  hw/block/virtio-blk.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e9ba752f6b..10c47c2934 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -49,13 +49,11 @@ static const VirtIOFeature feature_sizes[] = {
>      {}
>  };
>  
> -static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
> -{
> -    s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
> -        virtio_feature_get_config_size(feature_sizes, host_features));
> -
> -    assert(s->config_size <= sizeof(struct virtio_blk_config));
> -}
> +static const VirtIOConfigSizeParams cfg_size_params = {
> +    .min_size = VIRTIO_BLK_CFG_SIZE,
> +    .max_size = sizeof(struct virtio_blk_config),
> +    .feature_sizes = feature_sizes
> +};
>  
>  static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
>                                      VirtIOBlockReq *req)
> @@ -1204,8 +1202,8 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    virtio_blk_set_config_size(s, s->host_features);
> -
> +    s->config_size = virtio_get_config_size(&cfg_size_params,
> +                                            s->host_features);
>      virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
>  
>      s->blk = conf->conf.blk;
> -- 
> 2.25.1
> 

Re: [PATCH v2 3/8] virtio-net: utilize VirtIOConfigSizeParams & virtio_get_config_size

[Raphael Norwitz]

On Fri, Aug 26, 2022 at 05:32:43PM +0300, Daniil Tatianin wrote:
> Use the new common helper. As an added bonus this also makes use of
> config size sanity checking via the 'max_size' field.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  hw/net/virtio-net.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index dd0d056fde..dfc8dd8562 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -106,6 +106,11 @@ static const VirtIOFeature feature_sizes[] = {
>      {}
>  };
>  
> +static const VirtIOConfigSizeParams cfg_size_params = {

Can we have a zero length virtio-net config size? The both the v1.0 and
v1.1 section 5.1.4 say “The mac address field always exists (though is
only valid if VIRTIO_NET_F_MAC is set)” so we should probably set
min_size to offset_of status?

Otherwise LGTM.

> +    .max_size = sizeof(struct virtio_net_config),
> +    .feature_sizes = feature_sizes
> +};
> +
>  static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
>  {
>      VirtIONet *n = qemu_get_nic_opaque(nc);
> @@ -3246,8 +3251,7 @@ static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
>  {
>      virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
>  
> -    n->config_size = virtio_feature_get_config_size(feature_sizes,
> -                                                    host_features);
> +    n->config_size = virtio_get_config_size(&cfg_size_params, host_features);
>  }
>  
>  void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
> -- 
> 2.25.1
> 

Re: [PATCH v2 4/8] virtio: remove the virtio_feature_get_config_size helper

[Raphael Norwitz]

On Fri, Aug 26, 2022 at 05:32:44PM +0300, Daniil Tatianin wrote:
> This has no more users and is superseded by virtio_get_config_size.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

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

> ---
>  hw/virtio/virtio.c         | 15 ---------------
>  include/hw/virtio/virtio.h |  3 ---
>  2 files changed, 18 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 8518382025..c0bf8dd6fd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2999,21 +2999,6 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
>      return ret;
>  }
>  
> -size_t virtio_feature_get_config_size(const VirtIOFeature *feature_sizes,
> -                                      uint64_t host_features)
> -{
> -    size_t config_size = 0;
> -    int i;
> -
> -    for (i = 0; feature_sizes[i].flags != 0; i++) {
> -        if (host_features & feature_sizes[i].flags) {
> -            config_size = MAX(feature_sizes[i].end, config_size);
> -        }
> -    }
> -
> -    return config_size;
> -}
> -
>  size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
>                                uint64_t host_features)
>  {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 1991c58d9b..f3ff6710d5 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -53,9 +53,6 @@ typedef struct VirtIOConfigSizeParams {
>  size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
>                                uint64_t host_features);
>  
> -size_t virtio_feature_get_config_size(const VirtIOFeature *features,
> -                                      uint64_t host_features);
> -
>  typedef struct VirtQueue VirtQueue;
>  
>  #define VIRTQUEUE_MAX_SIZE 1024
> -- 
> 2.25.1
> 

Re: [PATCH v2 5/8] virtio-blk: move config size params to virtio-blk-common

[Raphael Norwitz]

The vhost-user-blk bits in meson.build and Maintainers should probably
be in patch 8?

Otherwise LGTM.

On Fri, Aug 26, 2022 at 05:32:45PM +0300, Daniil Tatianin wrote:
> This way we can reuse it for other virtio-blk devices, e.g
> vhost-user-blk, which currently does not control its config space size
> dynamically.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  MAINTAINERS                           |  4 +++
>  hw/block/meson.build                  |  4 +--
>  hw/block/virtio-blk-common.c          | 39 +++++++++++++++++++++++++++
>  hw/block/virtio-blk.c                 | 24 ++---------------
>  include/hw/virtio/virtio-blk-common.h | 20 ++++++++++++++
>  5 files changed, 67 insertions(+), 24 deletions(-)
>  create mode 100644 hw/block/virtio-blk-common.c
>  create mode 100644 include/hw/virtio/virtio-blk-common.h
> 

<snip>

i.e. move this.

> @@ -2271,11 +2273,13 @@ S: Maintained
>  F: contrib/vhost-user-blk/
>  F: contrib/vhost-user-scsi/
>  F: hw/block/vhost-user-blk.c
> +F: hw/block/virtio-blk-common.c
>  F: hw/scsi/vhost-user-scsi.c
>  F: hw/virtio/vhost-user-blk-pci.c
>  F: hw/virtio/vhost-user-scsi-pci.c
>  F: include/hw/virtio/vhost-user-blk.h
>  F: include/hw/virtio/vhost-user-scsi.h
> +F: include/hw/virtio/virtio-blk-common.h
>  
>  vhost-user-gpu
>  M: Marc-André Lureau <marcandre.lureau@redhat.com>
> diff --git a/hw/block/meson.build b/hw/block/meson.build
> index 2389326112..1908abd45c 100644
> --- a/hw/block/meson.build
> +++ b/hw/block/meson.build
> @@ -16,7 +16,7 @@ softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
>  softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
>  softmmu_ss.add(when: 'CONFIG_TC58128', if_true: files('tc58128.c'))
>  
> -specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
> -specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c'))
> +specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c', 'virtio-blk-common.c'))

And this

> +specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c', 'virtio-blk-common.c'))

Re: [PATCH v2 6/8] vhost-user-blk: make it possible to disable write-zeroes/discard

[Raphael Norwitz]

On Fri, Aug 26, 2022 at 05:32:46PM +0300, Daniil Tatianin wrote:
> It is useful to have the ability to disable these features for
> compatibility with older VMs that don't have these implemented.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

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

> ---
>  hw/block/vhost-user-blk.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..4c9727e08c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -259,8 +259,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
>      virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
>      virtio_add_feature(&features, VIRTIO_BLK_F_RO);
> -    virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
> -    virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
>  
>      if (s->config_wce) {
>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> @@ -592,6 +590,10 @@ static Property vhost_user_blk_properties[] = {
>                         VHOST_USER_BLK_AUTO_NUM_QUEUES),
>      DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
>      DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
> +    DEFINE_PROP_BIT64("discard", VHostUserBlk, parent_obj.host_features,
> +                      VIRTIO_BLK_F_DISCARD, true),
> +    DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, parent_obj.host_features,
> +                      VIRTIO_BLK_F_WRITE_ZEROES, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 2.25.1
> 

Re: [PATCH v2 7/8] vhost-user-blk: make 'config_wce' part of 'host_features'

[Raphael Norwitz]

On Fri, Aug 26, 2022 at 05:32:47PM +0300, Daniil Tatianin wrote:
> No reason to have this be a separate field. This also makes it more akin
> to what the virtio-blk device does.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

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

> ---
>  hw/block/vhost-user-blk.c          | 6 ++----
>  include/hw/virtio/vhost-user-blk.h | 1 -
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 4c9727e08c..0d916edefa 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -260,9 +260,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
>      virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
>      virtio_add_feature(&features, VIRTIO_BLK_F_RO);
>  
> -    if (s->config_wce) {
> -        virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> -    }
>      if (s->num_queues > 1) {
>          virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
>      }
> @@ -589,7 +586,8 @@ static Property vhost_user_blk_properties[] = {
>      DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
>                         VHOST_USER_BLK_AUTO_NUM_QUEUES),
>      DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
> -    DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
> +    DEFINE_PROP_BIT64("config-wce", VHostUserBlk, parent_obj.host_features,
> +                      VIRTIO_BLK_F_CONFIG_WCE, true),
>      DEFINE_PROP_BIT64("discard", VHostUserBlk, parent_obj.host_features,
>                        VIRTIO_BLK_F_DISCARD, true),
>      DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, parent_obj.host_features,
> diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
> index 7c91f15040..ea085ee1ed 100644
> --- a/include/hw/virtio/vhost-user-blk.h
> +++ b/include/hw/virtio/vhost-user-blk.h
> @@ -34,7 +34,6 @@ struct VHostUserBlk {
>      struct virtio_blk_config blkcfg;
>      uint16_t num_queues;
>      uint32_t queue_size;
> -    uint32_t config_wce;
>      struct vhost_dev dev;
>      struct vhost_inflight *inflight;
>      VhostUserState vhost_user;
> -- 
> 2.25.1
> 

Re: [PATCH v2 8/8] vhost-user-blk: dynamically resize config space based on features

[Raphael Norwitz]

On Fri, Aug 26, 2022 at 05:32:48PM +0300, Daniil Tatianin wrote:
> Make vhost-user-blk backwards compatible when migrating from older VMs
> running with modern features turned off, the same way it was done for
> virtio-blk in 20764be0421c ("virtio-blk: set config size depending on the features enabled")
> 
> It's currently impossible to migrate from an older VM with
> vhost-user-blk (with disable-legacy=off) because of errors like this:
> 
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: 41 device: 1 cmask: ff wmask: 80 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load virtio-blk:virtio
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:05.0:00.0:02.0/virtio-blk'
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> This is caused by the newer (destination) VM requiring a bigger BAR0
> alignment because it has to cover a bigger configuration space, which
> isn't actually needed since those additional config fields are not
> active (write-zeroes/discard).
>

With the relevant meson.build and MAINTAINERS bits rebased here:

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


> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  hw/block/vhost-user-blk.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 0d916edefa..8dd063eb96 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -23,6 +23,7 @@
>  #include "hw/qdev-core.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-properties-system.h"
> +#include "hw/virtio/virtio-blk-common.h"
>  #include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-user-blk.h"
>  #include "hw/virtio/virtio.h"
> @@ -63,7 +64,7 @@ static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      /* Our num_queues overrides the device backend */
>      virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);
>  
> -    memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
> +    memcpy(config, &s->blkcfg, vdev->config_len);
>  }
>  
>  static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> @@ -92,12 +93,12 @@ 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;
>  
>      ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
> -                               sizeof(struct virtio_blk_config),
> -                               &local_err);
> +                               vdev->config_len, &local_err);
>      if (ret < 0) {
>          error_report_err(local_err);
>          return ret;
> @@ -106,7 +107,7 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>      /* valid for resize only */
>      if (blkcfg.capacity != s->blkcfg.capacity) {
>          s->blkcfg.capacity = blkcfg.capacity;
> -        memcpy(dev->vdev->config, &s->blkcfg, sizeof(struct virtio_blk_config));
> +        memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
>          virtio_notify_config(dev->vdev);
>      }
>  
> @@ -442,7 +443,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
>      assert(s->connected);
>  
>      ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> -                               sizeof(struct virtio_blk_config), errp);
> +                               s->parent_obj.config_len, errp);
>      if (ret < 0) {
>          qemu_chr_fe_disconnect(&s->chardev);
>          vhost_dev_cleanup(&s->dev);
> @@ -457,6 +458,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>      ERRP_GUARD();
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +    size_t config_size;
>      int retries;
>      int i, ret;
>  
> @@ -487,8 +489,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    virtio_init(vdev, VIRTIO_ID_BLOCK,
> -                sizeof(struct virtio_blk_config));
> +    config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
> +                                         vdev->host_features);
> +    virtio_init(vdev, VIRTIO_ID_BLOCK, config_size);
>  
>      s->virtqs = g_new(VirtQueue *, s->num_queues);
>      for (i = 0; i < s->num_queues; i++) {
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size

[Daniil Tatianin]

On 9/2/22 8:52 PM, Raphael Norwitz wrote:
> I feel like it would be easier to review if the first 4 patches were
> squashed together, but that’s not a big deal.

Yeah, I think that's fair although I initially thought that maybe that 
was a bit too big of a change to put in one single commit. I can squash 
the first four if that would be better.

> For this one:
> 
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> 
> On Fri, Aug 26, 2022 at 05:32:41PM +0300, Daniil Tatianin wrote:
>> This is the first step towards moving all device config size calculation
>> logic into the virtio core code. In particular, this adds a struct that
>> contains all the necessary information for common virtio code to be able
>> to calculate the final config size for a device. This is expected to be
>> used with the new virtio_get_config_size helper, which calculates the
>> final length based on the provided host features.
>>
>> This builds on top of already existing code like VirtIOFeature and
>> virtio_feature_get_config_size(), but adds additional fields, as well as
>> sanity checking so that device-specifc code doesn't have to duplicate it.
>>
>> An example usage would be:
>>
>>      static const VirtIOFeature dev_features[] = {
>>          {.flags = 1ULL << FEATURE_1_BIT,
>>           .end = endof(struct virtio_dev_config, feature_1)},
>>          {.flags = 1ULL << FEATURE_2_BIT,
>>           .end = endof(struct virtio_dev_config, feature_2)},
>>          {}
>>      };
>>
>>      static const VirtIOConfigSizeParams dev_cfg_size_params = {
>>          .min_size = DEV_BASE_CONFIG_SIZE,
>>          .max_size = sizeof(struct virtio_dev_config),
>>          .feature_sizes = dev_features
>>      };
>>
>>      // code inside my_dev_device_realize()
>>      size_t config_size = virtio_get_config_size(&dev_cfg_size_params,
>>                                                  host_features);
>>      virtio_init(vdev, VIRTIO_ID_MYDEV, config_size);
>>
>> Currently every device is expected to write its own boilerplate from the
>> example above in device_realize(), however, the next step of this
>> transition is moving VirtIOConfigSizeParams into VirtioDeviceClass,
>> so that it can be done automatically by the virtio initialization code.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>>   hw/virtio/virtio.c         | 17 +++++++++++++++++
>>   include/hw/virtio/virtio.h |  9 +++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 5d607aeaa0..8518382025 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -3014,6 +3014,23 @@ size_t virtio_feature_get_config_size(const VirtIOFeature *feature_sizes,
>>       return config_size;
>>   }
>>   
>> +size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
>> +                              uint64_t host_features)
>> +{
>> +    size_t config_size = params->min_size;
>> +    const VirtIOFeature *feature_sizes = params->feature_sizes;
>> +    size_t i;
>> +
>> +    for (i = 0; feature_sizes[i].flags != 0; i++) {
>> +        if (host_features & feature_sizes[i].flags) {
>> +            config_size = MAX(feature_sizes[i].end, config_size);
>> +        }
>> +    }
>> +
>> +    assert(config_size <= params->max_size);
>> +    return config_size;
>> +}
>> +
>>   int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>>   {
>>       int i, ret;
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index db1c0ddf6b..1991c58d9b 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -44,6 +44,15 @@ typedef struct VirtIOFeature {
>>       size_t end;
>>   } VirtIOFeature;
>>   
>> +typedef struct VirtIOConfigSizeParams {
>> +    size_t min_size;
>> +    size_t max_size;
>> +    const VirtIOFeature *feature_sizes;
>> +} VirtIOConfigSizeParams;
>> +
>> +size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
>> +                              uint64_t host_features);
>> +
>>   size_t virtio_feature_get_config_size(const VirtIOFeature *features,
>>                                         uint64_t host_features);
>>   
>> -- 
>> 2.25.1

Re: [PATCH v2 5/8] virtio-blk: move config size params to virtio-blk-common

[Daniil Tatianin]

On 9/2/22 8:57 PM, Raphael Norwitz wrote:
> The vhost-user-blk bits in meson.build and Maintainers should probably
> be in patch 8?

You're totally right, thanks.

> Otherwise LGTM.
> 
> On Fri, Aug 26, 2022 at 05:32:45PM +0300, Daniil Tatianin wrote:
>> This way we can reuse it for other virtio-blk devices, e.g
>> vhost-user-blk, which currently does not control its config space size
>> dynamically.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>>   MAINTAINERS                           |  4 +++
>>   hw/block/meson.build                  |  4 +--
>>   hw/block/virtio-blk-common.c          | 39 +++++++++++++++++++++++++++
>>   hw/block/virtio-blk.c                 | 24 ++---------------
>>   include/hw/virtio/virtio-blk-common.h | 20 ++++++++++++++
>>   5 files changed, 67 insertions(+), 24 deletions(-)
>>   create mode 100644 hw/block/virtio-blk-common.c
>>   create mode 100644 include/hw/virtio/virtio-blk-common.h
>>
> 
> <snip>
> 
> i.e. move this.
> 
>> @@ -2271,11 +2273,13 @@ S: Maintained
>>   F: contrib/vhost-user-blk/
>>   F: contrib/vhost-user-scsi/
>>   F: hw/block/vhost-user-blk.c
>> +F: hw/block/virtio-blk-common.c
>>   F: hw/scsi/vhost-user-scsi.c
>>   F: hw/virtio/vhost-user-blk-pci.c
>>   F: hw/virtio/vhost-user-scsi-pci.c
>>   F: include/hw/virtio/vhost-user-blk.h
>>   F: include/hw/virtio/vhost-user-scsi.h
>> +F: include/hw/virtio/virtio-blk-common.h
>>   
>>   vhost-user-gpu
>>   M: Marc-André Lureau <marcandre.lureau@redhat.com>
>> diff --git a/hw/block/meson.build b/hw/block/meson.build
>> index 2389326112..1908abd45c 100644
>> --- a/hw/block/meson.build
>> +++ b/hw/block/meson.build
>> @@ -16,7 +16,7 @@ softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
>>   softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
>>   softmmu_ss.add(when: 'CONFIG_TC58128', if_true: files('tc58128.c'))
>>   
>> -specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
>> -specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c'))
>> +specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c', 'virtio-blk-common.c'))
> 
> And this
> 
>> +specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c', 'virtio-blk-common.c'))

Re: [PATCH v2 3/8] virtio-net: utilize VirtIOConfigSizeParams & virtio_get_config_size

[Daniil Tatianin]

On 9/2/22 8:54 PM, Raphael Norwitz wrote:
> On Fri, Aug 26, 2022 at 05:32:43PM +0300, Daniil Tatianin wrote:
>> Use the new common helper. As an added bonus this also makes use of
>> config size sanity checking via the 'max_size' field.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>>   hw/net/virtio-net.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index dd0d056fde..dfc8dd8562 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -106,6 +106,11 @@ static const VirtIOFeature feature_sizes[] = {
>>       {}
>>   };
>>   
>> +static const VirtIOConfigSizeParams cfg_size_params = {
> 
> Can we have a zero length virtio-net config size? The both the v1.0 and
> v1.1 section 5.1.4 say “The mac address field always exists (though is
> only valid if VIRTIO_NET_F_MAC is set)” so we should probably set
> min_size to offset_of status?

It currently hardcodes VIRTIO_NET_F_MAC as always on, but I guess it
doesn't hurt to be more explicit about it. Will add that in the next
version.
(resending because forgot to reply-all last time)
> Otherwise LGTM.
> 
>> +    .max_size = sizeof(struct virtio_net_config),
>> +    .feature_sizes = feature_sizes
>> +};
>> +
>>   static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
>>   {
>>       VirtIONet *n = qemu_get_nic_opaque(nc);
>> @@ -3246,8 +3251,7 @@ static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
>>   {
>>       virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
>>   
>> -    n->config_size = virtio_feature_get_config_size(feature_sizes,
>> -                                                    host_features);
>> +    n->config_size = virtio_get_config_size(&cfg_size_params, host_features);
>>   }
>>   
>>   void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
>> -- 
>> 2.25.1

Re: [PATCH v2 1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size

[Raphael Norwitz]

> I can squash the first four if that would be better.

I think so. Just may be easier for other reviewers :)