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

[Michael S. Tsirkin]

On Mon, Nov 01, 2021 at 04:42:01PM +0100, Eugenio Perez Martin wrote:
> 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!
> 

For sure, no need to handle an invalid configuration.
Do you have a patch in mind? It'd be easier to discuss
things with a specific patch rather than theoretically.

> 
> 
> > 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
> > > >
> > >