diff mbox series

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

Message ID 20220824091837.301708-3-d-tatianin@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series vhost-user-blk: dynamically resize config space based on features | expand

Commit Message

Daniil Tatianin Aug. 24, 2022, 9:18 a.m. UTC 
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

Comments

Stefan Hajnoczi Aug. 24, 2022, 6:13 p.m. UTC | #1 
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
Daniil Tatianin Aug. 24, 2022, 9:11 p.m. UTC | #2 
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
Stefan Hajnoczi Aug. 25, 2022, 1:45 p.m. UTC | #3 
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
diff mbox series

Patch

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