Re: [Qemu-devel] [PATCH v2 for-2.4] virtio-net: remove virtio queues if the guest doesn't support multiqueue

[Wen Congyang <wency@cn.fujitsu.com>]

On 07/15/2015 04:42 PM, Jason Wang wrote:
> 
> 
> On 07/15/2015 04:20 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 <wency@cn.fujitsu.com>
>> ---
>>  hw/net/virtio-net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 78 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index e3c2db3..48c7705 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1306,9 +1306,86 @@ 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_del_queue(VirtIONet *n, int index)
>> +{
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>> +    VirtIONetQueue *q = &n->vqs[index];
>> +    NetClientState *nc = qemu_get_subqueue(n->nic, index);
>> +
>> +    qemu_purge_queued_packets(nc);
>> +
>> +    virtio_del_queue(vdev, index * 2);
>> +    if (q->tx_timer) {
>> +        timer_del(q->tx_timer);
>> +        timer_free(q->tx_timer);
>> +    } else {
>> +        qemu_bh_delete(q->tx_bh);
>> +    }
>> +    virtio_del_queue(vdev, index * 2 + 1);
>> +}
> 
> Ok, then in unrealize() you may just want to delete bhs/timers up to
> curr_queues. Otherwise it may cause a use after free?

Yes. curr_queues is set in virtio_net_handle_mq(). It may be less than
max_queues. So I think we cannot use curr_queues directly. If mutliqueue
is enabled, we should delete bhs/timers up to max_queues, otherwise, up to 1.

Thanks
Wen Congyang

> 
>> +
>> +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;
>> +    }
>> +
>> +    /*
>> +     * We always need to remove and add ctrl vq if
>> +     * old_num_queues != new_num_queues. Remove ctrl_vq first,
>> +     * and then we only enter one of the following too loops.
>> +     */
>> +    virtio_del_queue(vdev, old_num_queues - 1);
>> +
>> +    for (i = new_num_queues - 1; i < old_num_queues - 1; i += 2) {
>> +        /* new_num_queues < old_num_queues */
>> +        virtio_net_del_queue(n, i / 2);
>> +    }
>> +
>> +    for (i = old_num_queues - 1; i < new_num_queues - 1; i += 2) {
>> +        /* new_num_queues > old_num_queues */
>> +        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 +1660,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);
> 
> .
>