On Tue, Nov 2, 2021 at 8:04 AM Jason Wang <jasow...@redhat.com> wrote: > > On Tue, Nov 2, 2021 at 2:59 PM Eugenio Perez Martin <epere...@redhat.com> > wrote: > > > > On Tue, Nov 2, 2021 at 5:09 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > On Mon, Nov 1, 2021 at 4:59 PM Eugenio Perez Martin <epere...@redhat.com> > > > wrote: > > > > > > > > On Mon, Nov 1, 2021 at 4:34 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > > > > > On Fri, Oct 29, 2021 at 10:16 PM Eugenio Pérez <epere...@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 <epere...@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.
Clarification: I meant networking here :). > > The last_index will be 1, so the device will be started after > vhost_dev 1 is initialized? > In !cvq !mq case, last_index is the number of virtqueues. It is initialized as: int ... last_index = data_queue_pairs * 2; and data queue pairs comes from caller function virtio_net_vhost_status: int queue_pairs = n->multiqueue ? n->max_queue_pairs : 1; so last_index ends up being 2. I didn't check a MQ device with !cvq, but I think it will be the same. Thanks! > 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 > > > > > > > > > > > > > > > > > > > > >