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

[Stefan Hajnoczi]

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