Re: [PATCH] vhost: Fix last queue index of devices with no cvq

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Nov 1, 2021 at 9:58 AM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Mon, Nov 1, 2021 at 4:34 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Oct 29, 2021 at 10:16 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > The -1 assumes that all devices with no cvq have an spare vq allocated
> > > for them, but with no offer of VIRTIO_NET_F_CTRL_VQ. This may not be the
> > > case, and the device may have a pair number of queues.
> > >
> > > To fix this, just resort to the lower even number of queues.
> > >
> > > Fixes: 049eb15b5fc9 ("vhost: record the last virtqueue index for the virtio device")
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  hw/net/vhost_net.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 0d888f29a6..edf56a597f 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -330,7 +330,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> > >      NetClientState *peer;
> > >
> > >      if (!cvq) {
> > > -        last_index -= 1;
> > > +        last_index &= ~1ULL;
> > >      }
> >
> > The math here looks correct but we need to fix vhost_vdpa_dev_start() instead?
> >
> > if (dev->vq_index + dev->nvqs - 1 != dev->last_index) {
> > ...
> > }
> >
>
> If we just do that, devices that offer an odd number of queues but do
> not offer ctrl vq would never enable the last vq pair, isn't it?
>

To expand the issue,

With that condition it is not possible to make vp_vdpa work on devices
with no cvq. If I set the L0 guest's device with no cvq (with -device
virtio-net-pci,...,ctrl_vq=off,mq=off). The nested VM will enter that
conditional in vhost_net_start, and will mark last_index=1, making it
impossible to start a vhost_vdpa device.

However, re-reading the standard:

controlq only exists if VIRTIO_NET_F_CTRL_VQ set.

So the code is actually handling an invalid device: The device set
VIRTIO_NET_F_CTRL_VQ but offered an odd number of VQs.

Do we have an example of such a device? It's not the case on qemu
virtio-net, with or without vhost-net in L0 device. The operation &=
~1ULL is an intended noop in case the queues are already even. I'm
fine to keep making last_index even, so we have that safety net, with
further clarifications as MST said, just in case the device is not
behaving well. But maybe it's even better just to delete that
conditional entirely?

Thanks!




> Also, I would say that the right place for the solution of this
> problem should not be virtio/vhost-vdpa: This is highly dependent on
> having cvq, and this implies a knowledge about the use of each
> virtqueue. Another kind of device could have an odd number of
> virtqueues naturally, and that (-1) would not work for them, isn't it?
>
> Thanks!
>
> > Thanks
> >
> > >
> > >      if (!k->set_guest_notifiers) {
> > > --
> > > 2.27.0
> > >
> >