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.
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.
Regards,
Akihiko Odaki
