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

[Jason Wang]

On Tue, Nov 2, 2021 at 2:59 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Tue, Nov 2, 2021 at 5:09 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Nov 1, 2021 at 4:59 PM 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?
> >
> > For vq pair, you assume that it's a networking device, so the device
> > you described here violates the spec.
> >
> > >
> > > 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?
> >
> > It actually depends on how multiqueue is modeled for each specific
> > type of device. They need to initialize the vq_index and nvqs
> > correctly:
> >
> > E.g if we had a device with 3 queues, we could model it with the following:
> >
> > vhost_dev 1, vq_index = 0, nvqs = 2
> > vhost_dev 2, vq_index = 2, nvqs = 1
> >
> > In this case the last_index should be initialized to 2, then we know
> > all the vhost_dev is initialized and we can start the hardware.
> >
>
> Right, but in that case, cvq == true, and we never enter the
> conditional if (!cvq).
>
> If cvq is false at that moment, your vhost_dev 2 *must* not exist and
> the last index will be even, so we must not subtract 1 to last_index.
> The subtraction is the cause the device never starts.

The last_index will be 1, so the device will be started after
vhost_dev 1 is initialized?

Thanks

>
> Given all of the above, I think we can skip the conditional entirely.
>
> Thanks!
>
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > > > Thanks
> > > >
> > > > >
> > > > >      if (!k->set_guest_notifiers) {
> > > > > --
> > > > > 2.27.0
> > > > >
> > > >
> > >
> >
>