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
>