On 2025/05/16 10:44, Jason Wang wrote:
On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
VIRTIO_NET_F_MQ uses bit 22.
Instead of inferring the required number of queues from
vdev->guest_features, use the number loaded from the vm state.
Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
---
include/hw/virtio/virtio.h | 2 +-
hw/net/virtio-net.c | 11 ++++-------
hw/virtio/virtio.c | 14 +++++++-------
3 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 638691028050..af52580c1e63 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -211,7 +211,7 @@ struct VirtioDeviceClass {
int (*start_ioeventfd)(VirtIODevice *vdev);
void (*stop_ioeventfd)(VirtIODevice *vdev);
/* Called before loading queues. Useful to add queues before loading. */
- int (*pre_load_queues)(VirtIODevice *vdev);
+ int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
This turns out to be tricky as it has a lot of assumptions as
described in the changelog (e.g only lower 32bit of guest_features is
correct etc when calling this function).
The problem is that I missed the following comment in
hw/virtio/virtio.c:
/*
* Temporarily set guest_features low bits - needed by
* virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
* VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
*
* Note: devices should always test host features in future - don't
create
* new dependencies like this.
*/
vdev->guest_features = features;
This problem is specific to guest_features so avoiding it should give us
a reliable solution.
I meant not all the states were fully loaded for pre_load_queues(),
this seems another trick besides the above one. We should seek a way
to do it in post_load() or at least document the assumptions.
The name of the function already clarifies the state is not fully
loaded. An implementation of the function can make no assumption on the
state except that you can add queues here, which is already documented.
Looking at the commit that introduces this which is 9379ea9db3c that says:
"""
Otherwise the loaded queues will not have handlers and elements
in them will not be processed.
"""
I fail to remember what it means or what happens if we don't do 9379ea9db3c.
The packets and control commands in the queues except the first queue
will not be processed because they do not have handle_output set.
I don't understand here, the VM is not resumed in this case. Or what
issue did you see here?
Below is the order of relevant events that happen when loading and
resuming a VM for migration:
1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
queue, and one control queue.
2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
the "n" parameter requires that.
3) The state of queues are loaded.
4) The VM gets resumed.
If we skip 2), 3) will load states of queues that are not added yet,
which breaks these queues and packets and leave control packets on them
unprocessed.
Regards,
Akihiko Odaki
