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

[PATCH v1 0/5] 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.

In particular it adds the following things:
- Ability to disable modern features like discard/write-zeroes.
- Dynamic configuration space resizing based on enabled features,
  by reusing the code, which was already present in virtio-blk.
- Makes the VHostUserBlk structure a bit less clunky by using the
  'host_features' field to represent enabled features, as opposed to
  using a separate field per feature. This was already done for
  virtio-blk a long time ago.

Daniil Tatianin (5):
  virtio-blk: decouple config size determination code from VirtIOBlock
  virtio-blk: move config space sizing code 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          | 42 +++++++++++++++++++++++++++
 hw/block/virtio-blk.c                 | 25 ++--------------
 include/hw/virtio/vhost-user-blk.h    |  4 ++-
 include/hw/virtio/virtio-blk-common.h | 21 ++++++++++++++
 7 files changed, 90 insertions(+), 39 deletions(-)
 create mode 100644 hw/block/virtio-blk-common.c
 create mode 100644 include/hw/virtio/virtio-blk-common.h

-- 
2.25.1

[PATCH v1 1/5] virtio-blk: decouple config size determination code from VirtIOBlock

[Daniil Tatianin]

Make it more stand-alone so that we can reuse it for other virtio-blk
devices that are not VirtIOBlock in the future commits.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e9ba752f6b..a4162dbbf2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -49,12 +49,13 @@ static const VirtIOFeature feature_sizes[] = {
     {}
 };
 
-static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
+static size_t virtio_blk_common_get_config_size(uint64_t host_features)
 {
-    s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
+    size_t 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));
+    assert(config_size <= sizeof(struct virtio_blk_config));
+    return config_size;
 }
 
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
@@ -1204,7 +1205,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_blk_set_config_size(s, s->host_features);
+    s->config_size = virtio_blk_common_get_config_size(s->host_features);
 
     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
 
-- 
2.25.1

[PATCH v1 2/5] virtio-blk: move config space sizing code 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          | 42 +++++++++++++++++++++++++++
 hw/block/virtio-blk.c                 | 24 +--------------
 include/hw/virtio/virtio-blk-common.h | 21 ++++++++++++++
 5 files changed, 70 insertions(+), 25 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..ac54568eb6
--- /dev/null
+++ b/hw/block/virtio-blk-common.c
@@ -0,0 +1,42 @@
+/*
+ * 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 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)},
+    {}
+};
+
+size_t virtio_blk_common_get_config_size(uint64_t host_features)
+{
+    size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
+        virtio_feature_get_config_size(feature_sizes, host_features));
+
+    assert(config_size <= sizeof(struct virtio_blk_config));
+    return config_size;
+}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a4162dbbf2..4ca6d0f211 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -32,31 +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 size_t virtio_blk_common_get_config_size(uint64_t host_features)
-{
-    size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
-        virtio_feature_get_config_size(feature_sizes, host_features));
-
-    assert(config_size <= sizeof(struct virtio_blk_config));
-    return config_size;
-}
 
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
diff --git a/include/hw/virtio/virtio-blk-common.h b/include/hw/virtio/virtio-blk-common.h
new file mode 100644
index 0000000000..08e9f1ab48
--- /dev/null
+++ b/include/hw/virtio/virtio-blk-common.h
@@ -0,0 +1,21 @@
+/*
+ * 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 <stddef.h>
+#include <stdint.h>
+
+size_t virtio_blk_common_get_config_size(uint64_t host_features);
+
+#endif
-- 
2.25.1

Re: [PATCH v1 2/5] virtio-blk: move config space sizing code to virtio-blk-common

[Stefan Hajnoczi]

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

On Wed, Aug 24, 2022 at 12:18:34PM +0300, Daniil Tatianin wrote:
> +size_t virtio_blk_common_get_config_size(uint64_t host_features)
> +{
> +    size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
> +        virtio_feature_get_config_size(feature_sizes, host_features));
> +
> +    assert(config_size <= sizeof(struct virtio_blk_config));
> +    return config_size;
> +}

This logic is common to all VIRTIO devices and I think it can be moved
to virtio_feature_get_config_size(). Then
virtio_blk_common_get_config_size() is no longer necessary and the
generic virtio_feature_get_config_size() can be called directly.

The only virtio-blk common part would be the
virtio_feature_get_config_size() parameter struct that describes the
minimum and maximum config space size, as well as how the feature bits
affect the size:

  size = virtio_feature_get_config_size(virtio_blk_config_size_params, host_features)

where virtio_blk_config_size_params is:

  const VirtIOConfigSizeParams virtio_blk_config_size_params = {
      .min_size = offsetof(struct virtio_blk_config, max_discard_sectors),
      .max_size = sizeof(struct virtio_blk_config),
      .features = {
          {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
           .end = endof(struct virtio_blk_config, discard_sector_alignment)},
          ...,
      },
  };

Then virtio-blk-common.h just needs to define:

  extern const VirtIOConfigSizeParams virtio_blk_config_size_params;

Taking it one step further, maybe VirtioDeviceClass should include a
const VirtIOConfigSizeParams *config_size_params field so
vdev->config_size can be computed by common VIRTIO code and the devices
only need to describe the parameters.

Stefan

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

Re: [PATCH v1 2/5] virtio-blk: move config space sizing code to virtio-blk-common

[Daniil Tatianin]

On 8/24/22 9:13 PM, Stefan Hajnoczi wrote:
> On Wed, Aug 24, 2022 at 12:18:34PM +0300, Daniil Tatianin wrote:
>> +size_t virtio_blk_common_get_config_size(uint64_t host_features)
>> +{
>> +    size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
>> +        virtio_feature_get_config_size(feature_sizes, host_features));
>> +
>> +    assert(config_size <= sizeof(struct virtio_blk_config));
>> +    return config_size;
>> +}
> 
> This logic is common to all VIRTIO devices and I think it can be moved
> to virtio_feature_get_config_size(). Then
> virtio_blk_common_get_config_size() is no longer necessary and the
> generic virtio_feature_get_config_size() can be called directly.
> 
> The only virtio-blk common part would be the
> virtio_feature_get_config_size() parameter struct that describes the
> minimum and maximum config space size, as well as how the feature bits
> affect the size:
> 
>    size = virtio_feature_get_config_size(virtio_blk_config_size_params, host_features)
> 
> where virtio_blk_config_size_params is:
> 
>    const VirtIOConfigSizeParams virtio_blk_config_size_params = {
>        .min_size = offsetof(struct virtio_blk_config, max_discard_sectors),
>        .max_size = sizeof(struct virtio_blk_config),
>        .features = {
>            {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
>             .end = endof(struct virtio_blk_config, discard_sector_alignment)},
>            ...,
>        },
>    };
> 
> Then virtio-blk-common.h just needs to define:
> 
>    extern const VirtIOConfigSizeParams virtio_blk_config_size_params;
> 
> Taking it one step further, maybe VirtioDeviceClass should include a
> const VirtIOConfigSizeParams *config_size_params field so
> vdev->config_size can be computed by common VIRTIO code and the devices
> only need to describe the parameters.

I think that's a great idea! Do you think it should be done 
automatically in 'virtio_init' if this field is not NULL? One problem I 
see with that is that you would have to make all virtio devices use 
'parent_obj.host_features' for feature properties, which is currently 
far from true, but then again this is very much opt-in. Another thing 
you could do is add a separate helper for that, which maybe defeats the 
purpose a little bit?

> 
> Stefan

Re: [PATCH v1 2/5] virtio-blk: move config space sizing code to virtio-blk-common

[Stefan Hajnoczi]

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

On Thu, Aug 25, 2022 at 12:11:10AM +0300, Daniil Tatianin wrote:
> 
> 
> On 8/24/22 9:13 PM, Stefan Hajnoczi wrote:
> > On Wed, Aug 24, 2022 at 12:18:34PM +0300, Daniil Tatianin wrote:
> > > +size_t virtio_blk_common_get_config_size(uint64_t host_features)
> > > +{
> > > +    size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
> > > +        virtio_feature_get_config_size(feature_sizes, host_features));
> > > +
> > > +    assert(config_size <= sizeof(struct virtio_blk_config));
> > > +    return config_size;
> > > +}
> > 
> > This logic is common to all VIRTIO devices and I think it can be moved
> > to virtio_feature_get_config_size(). Then
> > virtio_blk_common_get_config_size() is no longer necessary and the
> > generic virtio_feature_get_config_size() can be called directly.
> > 
> > The only virtio-blk common part would be the
> > virtio_feature_get_config_size() parameter struct that describes the
> > minimum and maximum config space size, as well as how the feature bits
> > affect the size:
> > 
> >    size = virtio_feature_get_config_size(virtio_blk_config_size_params, host_features)
> > 
> > where virtio_blk_config_size_params is:
> > 
> >    const VirtIOConfigSizeParams virtio_blk_config_size_params = {
> >        .min_size = offsetof(struct virtio_blk_config, max_discard_sectors),
> >        .max_size = sizeof(struct virtio_blk_config),
> >        .features = {
> >            {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
> >             .end = endof(struct virtio_blk_config, discard_sector_alignment)},
> >            ...,
> >        },
> >    };
> > 
> > Then virtio-blk-common.h just needs to define:
> > 
> >    extern const VirtIOConfigSizeParams virtio_blk_config_size_params;
> > 
> > Taking it one step further, maybe VirtioDeviceClass should include a
> > const VirtIOConfigSizeParams *config_size_params field so
> > vdev->config_size can be computed by common VIRTIO code and the devices
> > only need to describe the parameters.
> 
> I think that's a great idea! Do you think it should be done automatically in
> 'virtio_init' if this field is not NULL? One problem I see with that is that
> you would have to make all virtio devices use 'parent_obj.host_features' for
> feature properties, which is currently far from true, but then again this is
> very much opt-in. Another thing you could do is add a separate helper for
> that, which maybe defeats the purpose a little bit?

Yes, a helper is probably not necessary.

Refactoring virtio_feature_get_config_size() is enough for this patch
series. That way devices can still use their own host_features variables
as needed.

The virtio_init()/VirtioDeviceClass refactoring is probably a step too
far and I just wanted to share the idea :).

Stefan

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

[PATCH v1 3/5] 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          | 8 ++++++--
 include/hw/virtio/vhost-user-blk.h | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..e89164c358 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
 {
     VHostUserBlk *s = VHOST_USER_BLK(vdev);
 
+    features |= s->host_features;
+
     /* Turn on pre-defined features */
     virtio_add_feature(&features, VIRTIO_BLK_F_SIZE_MAX);
     virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX);
@@ -259,8 +261,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 +592,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, host_features,
+                      VIRTIO_BLK_F_DISCARD, true),
+    DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, host_features,
+                      VIRTIO_BLK_F_WRITE_ZEROES, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 7c91f15040..20573dd586 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -51,6 +51,8 @@ struct VHostUserBlk {
     bool connected;
     /* vhost_user_blk_start/vhost_user_blk_stop */
     bool started_vu;
+
+    uint64_t host_features;
 };
 
 #endif
-- 
2.25.1

Re: [PATCH v1 3/5] vhost-user-blk: make it possible to disable write-zeroes/discard

[Stefan Hajnoczi]

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

On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote:
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..e89164c358 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
>  {
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
>  
> +    features |= s->host_features;

I think you can eliminate this if you use vdev->host_features in the
qdev properties instead of adding a separate s->host_features field.
That will simplify the code.

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

Re: [PATCH v1 3/5] vhost-user-blk: make it possible to disable write-zeroes/discard

[Daniil Tatianin]

On 8/24/22 9:00 PM, Stefan Hajnoczi wrote:
> On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote:
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 9117222456..e89164c358 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
>>   {
>>       VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>   
>> +    features |= s->host_features;
> 
> I think you can eliminate this if you use vdev->host_features in the
> qdev properties instead of adding a separate s->host_features field.
> That will simplify the code.
Indeed, thanks for spotting that. I wonder why every virtio device 
implementation I've looked at has chosen to add their own host_features 
field (net/blk)?

Re: [PATCH v1 3/5] vhost-user-blk: make it possible to disable write-zeroes/discard

[Stefan Hajnoczi]

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

On Wed, Aug 24, 2022 at 11:24:55PM +0300, Daniil Tatianin wrote:
> On 8/24/22 9:00 PM, Stefan Hajnoczi wrote:
> > On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote:
> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > index 9117222456..e89164c358 100644
> > > --- a/hw/block/vhost-user-blk.c
> > > +++ b/hw/block/vhost-user-blk.c
> > > @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
> > >   {
> > >       VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > > +    features |= s->host_features;
> > 
> > I think you can eliminate this if you use vdev->host_features in the
> > qdev properties instead of adding a separate s->host_features field.
> > That will simplify the code.
> Indeed, thanks for spotting that. I wonder why every virtio device
> implementation I've looked at has chosen to add their own host_features
> field (net/blk)?

It's for historical reasons. Over time more common behavior moves into
the core, but not all devices have been refactored to take advantage of
that.

Thanks,
Stefan

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

[PATCH v1 4/5] 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 e89164c358..64f3457373 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -262,9 +262,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);
     }
@@ -591,7 +588,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, host_features,
+                      VIRTIO_BLK_F_CONFIG_WCE, true),
     DEFINE_PROP_BIT64("discard", VHostUserBlk, host_features,
                       VIRTIO_BLK_F_DISCARD, true),
     DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, host_features,
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 20573dd586..6252095c45 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 v1 4/5] vhost-user-blk: make 'config_wce' part of 'host_features'

[Stefan Hajnoczi]

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

On Wed, Aug 24, 2022 at 12:18:36PM +0300, Daniil Tatianin wrote:
> @@ -591,7 +588,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, host_features,
> +                      VIRTIO_BLK_F_CONFIG_WCE, true),

Please use parent_obj.host_features instead of a VHostUserBlk
host_features field.

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

[PATCH v1 5/5] 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          | 15 ++++++++-------
 include/hw/virtio/vhost-user-blk.h |  1 +
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 64f3457373..d18a7a2cd4 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, s->config_size);
 }
 
 static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -96,8 +97,7 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
     Error *local_err = NULL;
 
     ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
-                               sizeof(struct virtio_blk_config),
-                               &local_err);
+                               s->config_size, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
@@ -106,7 +106,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, s->config_size);
         virtio_notify_config(dev->vdev);
     }
 
@@ -444,7 +444,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->config_size, errp);
     if (ret < 0) {
         qemu_chr_fe_disconnect(&s->chardev);
         vhost_dev_cleanup(&s->dev);
@@ -489,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));
+    s->config_size = virtio_blk_common_get_config_size(s->host_features);
+
+    virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
 
     s->virtqs = g_new(VirtQueue *, s->num_queues);
     for (i = 0; i < s->num_queues; i++) {
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 6252095c45..b7810360b9 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -52,6 +52,7 @@ struct VHostUserBlk {
     bool started_vu;
 
     uint64_t host_features;
+    size_t config_size;
 };
 
 #endif
-- 
2.25.1

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

[Stefan Hajnoczi]

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

On Wed, Aug 24, 2022 at 12:18:37PM +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).
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  hw/block/vhost-user-blk.c          | 15 ++++++++-------
>  include/hw/virtio/vhost-user-blk.h |  1 +
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 64f3457373..d18a7a2cd4 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, s->config_size);

Please drop the s->config_size field introduced in this patch and use
vdev->config_len instead. When the same value is stored in multiple
places it's hard to be sure each copy remains identical and bugs can
creep in.

For example, if vdev->config_len is used consistently then it's clear
that buffer overflows and information leaks are prevented by common
code:

  uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
  {
      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
      uint8_t val;

      if (addr + sizeof(val) > vdev->config_len) {
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          return (uint32_t)-1;
      }

      k->get_config(vdev, vdev->config);

It's safe because vdev->config is g_malloc0(vdev->config_len).

Buf if I see s->config_size, I don't really know whether it's safe and I
need to audit the code to be sure.

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