Re: [Qemu-devel] [PATCH v2] virtio-net: unbreak the minix guest

[Aurelien Jarno <aurelien@aurel32.net>]

On Thu, Apr 25, 2013 at 03:24:23PM +0800, Jason Wang wrote:
> Multiqueue patchset conditionally add control vq only when guest negotiate the
> feature. Though the spec is not clear on this but it breaks the minix guest
> since it will identify the ctrl vq even if it does not support it. Though this
> behavior seems a violation on the spec "If the VIRTIO_NET_F_CTRL_VQ feature bit
> is negotiated, identify the control virtqueue.", to keep the backward
> compatibility, always add the ctrl vq at end of the queues.
> 
> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> ---
> Changes from V1:
> - Drop the unrelated cleanups
> - Drop the meaningless ctrl parameter of virtio_net_set_multiqueue()
> ---
>  hw/net/virtio-net.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)


Thanks a lot for this patch. It fixes the issue observed on Minix. For
the record it also fixes virtio-net with NetBSD 6.0.1, before it was
triggering an assertion in QEMU.

Now that the problem is clear, I will try to work on fixing the problem
on the OS side when I find some time.

Tested-by: Aurelien Jarno <aurelien@aurel32.net>

> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 4d2cdd2..1662f46 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -309,7 +309,7 @@ static void virtio_net_set_queues(VirtIONet *n)
>      }
>  }
>  
> -static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl);
> +static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>  
>  static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
>  {
> @@ -364,8 +364,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      int i;
>  
> -    virtio_net_set_multiqueue(n, !!(features & (1 << VIRTIO_NET_F_MQ)),
> -                              !!(features & (1 << VIRTIO_NET_F_CTRL_VQ)));
> +    virtio_net_set_multiqueue(n, !!(features & (1 << VIRTIO_NET_F_MQ)));
>  
>      virtio_net_set_mrg_rx_bufs(n, !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)));
>  
> @@ -1038,7 +1037,7 @@ static void virtio_net_tx_bh(void *opaque)
>      }
>  }
>  
> -static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl)
> +static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>      int i, max = multiqueue ? n->max_queues : 1;
> @@ -1067,9 +1066,11 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl)
>          n->vqs[i].n = n;
>      }
>  
> -    if (ctrl) {
> -        n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
> -    }
> +    /* Note: Minux Guests (version 3.2.1) use ctrl vq but don't ack
> +     * VIRTIO_NET_F_CTRL_VQ. Create ctrl vq unconditionally to avoid
> +     * breaking them.
> +     */
> +    n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
>  
>      virtio_net_set_queues(n);
>  }
> -- 
> 1.7.1
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net