| Message ID | 20220826143248.580939-2-d-tatianin@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-blk: dynamically resize config space based on features | expand |
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 >
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
> I can squash the first four if that would be better.
I think so. Just may be easier for other reviewers :)
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);
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(+)