On Thu, May 22, 2025 at 12:39 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2025/05/22 10:50, 'Jason Wang' via devel wrote:
> > On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.od...@daynix.com>
> > wrote:
> >>
> >> On 2025/05/21 9:51, Jason Wang wrote:
> >>> On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.od...@daynix.com>
> >>> wrote:
> >>>>
> >>>> 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.
> >>>
> >>> Where? I can only see this:
> >>>
> >>> /* Called before loading queues. Useful to add queues before
> >>> loading. */
> >>
> >> I meant it is documented that it can add queues. There is nothing else
> >> to document as an implementation of the function can make no assumption
> >> else.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> 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.
> >>>
> >>> Ok, let's document this and
> >>>
> >>> 1) explain why only virtio-net requires the pre_load_queues (due to
> >>> the fact that the index of ctrl vq could be changed according to
> >>> #queue_paris)
> >>
> >> We would need this logic even if the index of ctrl vq didn't change. We
> >> need it because virtio-net have varying number of queues, which needs to
> >> be added before loading.
> >
> > Well, if the ctrl vq index doesn't change we don't need a dynamic
> > virtio_add_queue() we can do them all in realize just like other
> > multiqueue devices.
>
> The number of virtqueues also affects the behavior visible to the guest
> so we shouldn't add them all in realize anyway. In particular, struct
> virtio_pci_common_cfg contains fields related virtqueus, and most (if
> not all) of them are affected by the number of virtqueues.
I don't understand here, is this requested by the spec for example?
Thanks
>
> Regards,
> Akihiko Odaki
>
