On 07/15/2015 02:59 PM, Jason Wang wrote: > > > On 07/15/2015 02:23 PM, Wen Congyang wrote: >> commit da51a335 adds all queues in .realize(). But if the >> guest doesn't support multiqueue, we forget to remove them. And >> we cannot handle the ctrl vq corretly. The guest will hang. >> >> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com> > > Thanks for the patch, some questions, see below. > >> --- >> hw/net/virtio-net.c | 69 >> +++++++++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 54 insertions(+), 15 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index e3c2db3..658806a 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -1306,9 +1306,62 @@ static void virtio_net_tx_bh(void *opaque) >> } >> } >> >> +static void virtio_net_add_queue(VirtIONet *n, int index) >> +{ >> + VirtIODevice *vdev = VIRTIO_DEVICE(n); >> + >> + n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx); >> + if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) { >> + n->vqs[index].tx_vq = >> + virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer); >> + n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, >> + virtio_net_tx_timer, >> + &n->vqs[index]); >> + } else { >> + n->vqs[index].tx_vq = >> + virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh); >> + n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]); >> + } >> + >> + n->vqs[index].tx_waiting = 0; >> + n->vqs[index].n = n; >> +} >> + >> +static void virtio_net_change_num_queues(VirtIONet *n, int new_max_queues) >> +{ >> + VirtIODevice *vdev = VIRTIO_DEVICE(n); >> + int old_num_queues = virtio_get_num_queues(vdev); >> + int new_num_queues = new_max_queues * 2 + 1; >> + int i; >> + >> + assert(old_num_queues >= 3); >> + assert(old_num_queues % 2 == 1); >> + >> + if (old_num_queues == new_num_queues) { >> + return; >> + } >> + >> + /* Remove ctrl_vq first */ >> + virtio_del_queue(vdev, old_num_queues - 1); > > Why ctrl vq must be first to be deleted?
It just make the codes clean. We always need to remove and add ctrl vq if old_num_queues != new_num_queues. We only enter one of the two loops. > >> + >> + for (i = new_num_queues - 1; i < old_num_queues - 1; i++) { >> + virtio_del_queue(vdev, i); > > Do we need to delete bh and timer also here? I think we should delete it too. Before commit da51a335, we only call virtio_del_queue(), so I forgot to do it. Thanks Wen Congyang > >> + } >> + >> + for (i = old_num_queues - 1; i < new_num_queues - 1; i += 2) { >> + virtio_net_add_queue(n, i / 2); >> + } >> + >> + /* add ctrl_vq last */ >> + n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); >> +} >> + >> static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue) >> { >> + int max = multiqueue ? n->max_queues : 1; >> + >> n->multiqueue = multiqueue; >> + virtio_net_change_num_queues(n, max); >> >> virtio_net_set_queues(n); >> } >> @@ -1583,21 +1636,7 @@ static void virtio_net_device_realize(DeviceState >> *dev, Error **errp) >> } >> >> for (i = 0; i < n->max_queues; i++) { >> - n->vqs[i].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx); >> - if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) { >> - n->vqs[i].tx_vq = >> - virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer); >> - n->vqs[i].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, >> - virtio_net_tx_timer, >> - &n->vqs[i]); >> - } else { >> - n->vqs[i].tx_vq = >> - virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh); >> - n->vqs[i].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[i]); >> - } >> - >> - n->vqs[i].tx_waiting = 0; >> - n->vqs[i].n = n; >> + virtio_net_add_queue(n, i); >> } >> >> n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); > > . >