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
signature.asc
Description: PGP signature
