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>
---
 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 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