| Message ID | 20220824091837.301708-4-d-tatianin@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-blk: dynamically resize config space based on features | expand |
On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote: > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index 9117222456..e89164c358 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, > { > VHostUserBlk *s = VHOST_USER_BLK(vdev); > > + features |= s->host_features; I think you can eliminate this if you use vdev->host_features in the qdev properties instead of adding a separate s->host_features field. That will simplify the code.
On 8/24/22 9:00 PM, Stefan Hajnoczi wrote: > On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote: >> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c >> index 9117222456..e89164c358 100644 >> --- a/hw/block/vhost-user-blk.c >> +++ b/hw/block/vhost-user-blk.c >> @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, >> { >> VHostUserBlk *s = VHOST_USER_BLK(vdev); >> >> + features |= s->host_features; > > I think you can eliminate this if you use vdev->host_features in the > qdev properties instead of adding a separate s->host_features field. > That will simplify the code. Indeed, thanks for spotting that. I wonder why every virtio device implementation I've looked at has chosen to add their own host_features field (net/blk)?
On Wed, Aug 24, 2022 at 11:24:55PM +0300, Daniil Tatianin wrote: > On 8/24/22 9:00 PM, Stefan Hajnoczi wrote: > > On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote: > > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > > > index 9117222456..e89164c358 100644 > > > --- a/hw/block/vhost-user-blk.c > > > +++ b/hw/block/vhost-user-blk.c > > > @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, > > > { > > > VHostUserBlk *s = VHOST_USER_BLK(vdev); > > > + features |= s->host_features; > > > > I think you can eliminate this if you use vdev->host_features in the > > qdev properties instead of adding a separate s->host_features field. > > That will simplify the code. > Indeed, thanks for spotting that. I wonder why every virtio device > implementation I've looked at has chosen to add their own host_features > field (net/blk)? It's for historical reasons. Over time more common behavior moves into the core, but not all devices have been refactored to take advantage of that. Thanks, Stefan
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 9117222456..e89164c358 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, { VHostUserBlk *s = VHOST_USER_BLK(vdev); + features |= s->host_features; + /* Turn on pre-defined features */ virtio_add_feature(&features, VIRTIO_BLK_F_SIZE_MAX); virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX); @@ -259,8 +261,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH); virtio_add_feature(&features, VIRTIO_BLK_F_RO); - virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD); - virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES); if (s->config_wce) { virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); @@ -592,6 +592,10 @@ static Property vhost_user_blk_properties[] = { VHOST_USER_BLK_AUTO_NUM_QUEUES), DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128), DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true), + DEFINE_PROP_BIT64("discard", VHostUserBlk, host_features, + VIRTIO_BLK_F_DISCARD, true), + DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, host_features, + VIRTIO_BLK_F_WRITE_ZEROES, true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h index 7c91f15040..20573dd586 100644 --- a/include/hw/virtio/vhost-user-blk.h +++ b/include/hw/virtio/vhost-user-blk.h @@ -51,6 +51,8 @@ struct VHostUserBlk { bool connected; /* vhost_user_blk_start/vhost_user_blk_stop */ bool started_vu; + + uint64_t host_features; }; #endif
It is useful to have the ability to disable these features for compatibility with older VMs that don't have these implemented. Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru> --- hw/block/vhost-user-blk.c | 8 ++++++-- include/hw/virtio/vhost-user-blk.h | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-)