| Message ID | 1366870793-769-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 25, 2013 at 02:19:53PM +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. > > Also remove the meaningless ctrl_vq initialization and vq deletion. > > Reported-by: Aurelien Jarno <aurelien@aurel32.net> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/net/virtio-net.c | 9 ++++----- > 1 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 4d2cdd2..1068f4e 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1045,7 +1045,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl) > > n->multiqueue = multiqueue; > > - for (i = 2; i <= n->max_queues * 2 + 1; i++) { > + for (i = 2; i <= n->max_queues * 2; i++) { > virtio_del_queue(vdev, i); > } > Unrelated cleanup? Separate patch pls. > @@ -1067,9 +1067,9 @@ 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); > - } > + /* Some guest (e.g minix) may identifiy ctrl vq even if it does not > + * support. */ /* * Note: Minux Guests (version XYZ) 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); > } > @@ -1317,7 +1317,6 @@ static int virtio_net_device_init(VirtIODevice *vdev) > virtio_net_handle_tx_bh); > n->vqs[0].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[0]); > } > - n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); Why drop this here? Unrelated cleanup? Separate patch pls. > qemu_macaddr_default_if_unset(&n->nic_conf.macaddr); > memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac)); > n->status = VIRTIO_NET_S_LINK_UP; > -- > 1.7.1
On 04/25/2013 02:57 PM, Michael S. Tsirkin wrote: > On Thu, Apr 25, 2013 at 02:19:53PM +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. >> >> Also remove the meaningless ctrl_vq initialization and vq deletion. >> >> Reported-by: Aurelien Jarno <aurelien@aurel32.net> >> Cc: Aurelien Jarno <aurelien@aurel32.net> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/net/virtio-net.c | 9 ++++----- >> 1 files changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 4d2cdd2..1068f4e 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -1045,7 +1045,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl) >> >> n->multiqueue = multiqueue; >> >> - for (i = 2; i <= n->max_queues * 2 + 1; i++) { >> + for (i = 2; i <= n->max_queues * 2; i++) { >> virtio_del_queue(vdev, i); >> } >> > Unrelated cleanup? Separate patch pls. Ok. > >> @@ -1067,9 +1067,9 @@ 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); >> - } >> + /* Some guest (e.g minix) may identifiy ctrl vq even if it does not >> + * support. */ > /* > * Note: Minux Guests (version XYZ) use ctrl vq but don't ack > * VIRTIO_NET_F_CTRL_VQ. Create ctrl vq unconditionally to avoid > * breaking them. > */ Sure. >> + n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); >> >> virtio_net_set_queues(n); >> } >> @@ -1317,7 +1317,6 @@ static int virtio_net_device_init(VirtIODevice *vdev) >> virtio_net_handle_tx_bh); >> n->vqs[0].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[0]); >> } >> - n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); > Why drop this here? Unrelated cleanup? Separate patch pls. Ok, will send v2. Thanks >> qemu_macaddr_default_if_unset(&n->nic_conf.macaddr); >> memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac)); >> n->status = VIRTIO_NET_S_LINK_UP; >> -- >> 1.7.1
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 4d2cdd2..1068f4e 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1045,7 +1045,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl) n->multiqueue = multiqueue; - for (i = 2; i <= n->max_queues * 2 + 1; i++) { + for (i = 2; i <= n->max_queues * 2; i++) { virtio_del_queue(vdev, i); } @@ -1067,9 +1067,9 @@ 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); - } + /* Some guest (e.g minix) may identifiy ctrl vq even if it does not + * support. */ + n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); virtio_net_set_queues(n); } @@ -1317,7 +1317,6 @@ static int virtio_net_device_init(VirtIODevice *vdev) virtio_net_handle_tx_bh); n->vqs[0].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[0]); } - n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); qemu_macaddr_default_if_unset(&n->nic_conf.macaddr); memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac)); n->status = VIRTIO_NET_S_LINK_UP;
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. Also remove the meaningless ctrl_vq initialization and vq deletion. Reported-by: Aurelien Jarno <aurelien@aurel32.net> Cc: Aurelien Jarno <aurelien@aurel32.net> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/net/virtio-net.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)