On Mon, Oct 14, 2024 at 11:16 PM Laurent Vivier <lviv...@redhat.com> wrote:
>
> On 14/10/2024 10:30, Laurent Vivier wrote:
> > Hi Akihiko,
> >
> > On 04/06/2024 09:37, Jason Wang wrote:
> >> From: Akihiko Odaki <akihiko.od...@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.od...@daynix.com>
> >> Signed-off-by: Jason Wang <jasow...@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
>
I agree.
Laurent, would you like to send a patch to revert this?
Thanks
>
