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