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