| Message ID | 20210223144653.811468-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: vhost-user-blk server tests and input validation | expand |
On Tue, Feb 23, 2021 at 02:46:42PM +0000, Stefan Hajnoczi wrote: > Treat the num_queues field as virtio-endian. On big-endian hosts the > vhost-user-blk num_queues field was in the wrong endianness. > > Move the blkcfg.num_queues store operation from realize to > vhost_user_blk_update_config() so feature negotiation has finished and > we know the endianness of the device. VIRTIO 1.0 devices are > little-endian, but in case someone wants to use legacy VIRTIO we support > all endianness cases. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> okay but as we recently discovered config space can in theory be read before FEATURES_OK. Nasty, I know. Things kind of work right now but we really need some other path to notify backends when legacy guest is active. E.g. VDPA also has this problem. > --- > hw/block/vhost-user-blk.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index da4fbf9084..b870a50e6b 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -54,6 +54,9 @@ static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config) > { > VHostUserBlk *s = VHOST_USER_BLK(vdev); > > + /* Our num_queues overrides the device backend */ > + virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues); > + > memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config)); > } > > @@ -491,10 +494,6 @@ reconnect: > goto reconnect; > } > > - if (s->blkcfg.num_queues != s->num_queues) { > - s->blkcfg.num_queues = s->num_queues; > - } > - > return; > > virtio_err: > -- > 2.29.2 >
On Tue, Feb 23, 2021 at 11:13:47AM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 23, 2021 at 02:46:42PM +0000, Stefan Hajnoczi wrote: > > Treat the num_queues field as virtio-endian. On big-endian hosts the > > vhost-user-blk num_queues field was in the wrong endianness. > > > > Move the blkcfg.num_queues store operation from realize to > > vhost_user_blk_update_config() so feature negotiation has finished and > > we know the endianness of the device. VIRTIO 1.0 devices are > > little-endian, but in case someone wants to use legacy VIRTIO we support > > all endianness cases. > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > okay but as we recently discovered config space can in theory > be read before FEATURES_OK. Nasty, I know. Things kind of work > right now but we really need some other path to notify backends > when legacy guest is active. E.g. VDPA also has this problem. Thanks for mentioning it. Do you know specifics about this type of guest driver? If it's only legacy drivers then device implementations can ensure compatibility by using host-endian until FEATURES_OK :). I guess existing code needs to be audited to check if this can be done. Stefan
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index da4fbf9084..b870a50e6b 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -54,6 +54,9 @@ static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config) { VHostUserBlk *s = VHOST_USER_BLK(vdev); + /* Our num_queues overrides the device backend */ + virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues); + memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config)); } @@ -491,10 +494,6 @@ reconnect: goto reconnect; } - if (s->blkcfg.num_queues != s->num_queues) { - s->blkcfg.num_queues = s->num_queues; - } - return; virtio_err: