Re: [PULL 08/20] virtio-net: Add only one queue pair when realizing

[Laurent Vivier <lvivier@redhat.com>]

On 14/10/2024 10:30, Laurent Vivier wrote:
> Hi Akihiko,
> 
> On 04/06/2024 09:37, Jason Wang wrote:
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> Multiqueue usage is not negotiated yet when realizing. If more than
>> one queue is added and the guest never requests to enable multiqueue,
>> the extra queues will not be deleted when unrealizing and leak.
>>
>> Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't support 
>> multiqueue")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/net/virtio-net.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 3cee2ef3ac..a8db8bfd9c 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -3743,9 +3743,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>>       n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n),
>>                                       n->net_conf.tx_queue_size);
>> -    for (i = 0; i < n->max_queue_pairs; i++) {
>> -        virtio_net_add_queue(n, i);
>> -    }
>> +    virtio_net_add_queue(n, 0);
>>       n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
>>       qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
> 
> This change breaks virtio net migration when multiqueue is enabled.
> 
> I think this is because virtqueues are half initialized after migration : they are 
> initialized on guest side (kernel is using them) but not on QEMU side (realized has only 
> initialized one). After migration, they are not initialized by the call to 
> virtio_net_set_multiqueue() from virtio_net_set_features() because virtio_get_num_queues() 
> reports already n->max_queue_pairs as this value is coming from the source guest memory.
> 
> I don't think we have a way to half-initialize a virtqueue (to initialize them only on 
> QEMU side as they are already initialized on kernel side).
> 
> I think this change should be reverted to fix the migration issue.
> 

Moreover, if I look in the code of virtio_load() and virtio_add_queue() we can guess it's 
not correct to migrate a virtqueue that is not initialized on the destination side because 
fields like 'vdev->vq[i].handle_output' or 'vdev->vq[i].used_elems' cannot be initialized 
by virtio_load() and neither by virtio_add_queue() after virtio_load() as fields like 
'vring.num' are already initialized by virtio_load().

For instance, in virtio_load() we set:

     for (i = 0; i < num; i++) {
         vdev->vq[i].vring.num = qemu_get_be32(f);

and in virtio_add_queue() we search for the firt available queue to add with:

     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
         if (vdev->vq[i].vring.num == 0)
             break;
     }

So virtio_add_queue() cannot be used to set:

     vdev->vq[i].handle_output = handle_output;
     vdev->vq[i].used_elems = g_new0(VirtQueueElement, queue_size);

Moreover it would overwrite fields already set by virtio_load():

     vdev->vq[i].vring.num = queue_size;
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;

It also explains why virtio_net_change_num_queue_pairs() (indirectly called by 
virtio_net_set_features()) doesn't update the queue pair numbers: vring.num is already set 
so it thinks there is no more queues to add.

Thanks,
LAurent