diff mbox series

[v2,1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size

Message ID 20220826143248.580939-2-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. 26, 2022, 2:32 p.m. UTC 
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(+)

Comments

Raphael Norwitz Sept. 2, 2022, 5:52 p.m. UTC | #1 
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
>
Daniil Tatianin Sept. 2, 2022, 10:12 p.m. UTC | #2 
On 9/2/22 8:52 PM, Raphael Norwitz wrote:
> I feel like it would be easier to review if the first 4 patches were
> squashed together, but that’s not a big deal.

Yeah, I think that's fair although I initially thought that maybe that 
was a bit too big of a change to put in one single commit. I can squash 
the first four if that would be better.

> 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
Raphael Norwitz Sept. 4, 2022, 10:40 p.m. UTC | #3 
> I can squash the first four if that would be better.

I think so. Just may be easier for other reviewers :)
diff mbox series

Patch

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);