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

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

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

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

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