The -1 assumes that last index counts all vhost device models as having
two queues, but they count only the ones that models the data queues.

Because of that, the right change in last_index is to actually add the
cvq, not to remove the missing one.

This is not a problem to vhost-net, but it is to vhost-vdpa, which
device model trust to reach the last index to finish starting the
device.

Tested with vp_vdpa with host's vhost=on and vhost=off, with ctrl_vq=on
and ctrl_vq=off.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 081946dc93..fe2b8a2b83 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -329,8 +329,8 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
     int r, e, i, last_index = data_queue_pairs * 2;
     NetClientState *peer;
 
-    if (!cvq) {
-        last_index -= 1;
+    if (cvq) {
+        last_index += 1;
     }
 
     if (!k->set_guest_notifiers) {
-- 
2.27.0

Eugenio Pérez <eperezma@redhat.com> wrote:
> The -1 assumes that last index counts all vhost device models as having
> two queues, but they count only the ones that models the data queues.
>
> Because of that, the right change in last_index is to actually add the
> cvq, not to remove the missing one.
>
> This is not a problem to vhost-net, but it is to vhost-vdpa, which
> device model trust to reach the last index to finish starting the
> device.
>
> Tested with vp_vdpa with host's vhost=on and vhost=off, with ctrl_vq=on
> and ctrl_vq=off.
>
> Fixes: 049eb15b5fc9 ("vhost: record the last virtqueue index for the
> virtio device")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

On Wed, Nov 3, 2021 at 6:01 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> The -1 assumes that last index counts all vhost device models as having
> two queues, but they count only the ones that models the data queues.
>
> Because of that, the right change in last_index is to actually add the
> cvq, not to remove the missing one.
>
> This is not a problem to vhost-net, but it is to vhost-vdpa, which
> device model trust to reach the last index to finish starting the
> device.
>
> Tested with vp_vdpa with host's vhost=on and vhost=off, with ctrl_vq=on
> and ctrl_vq=off.
>
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 081946dc93..fe2b8a2b83 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -329,8 +329,8 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>      int r, e, i, last_index = data_queue_pairs * 2;
>      NetClientState *peer;
>
> -    if (!cvq) {
> -        last_index -= 1;
> +    if (cvq) {
> +        last_index += 1;

Patch looks ok.

Not a native speaker but I guess "last" should be inside the closed
interval. If this is true, I still prefer to change the check in
vhost_vdpa_dev_start().

Thanks

>      }
>
>      if (!k->set_guest_notifiers) {
> --
> 2.27.0
>

On Thu, Nov 4, 2021 at 3:47 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Nov 3, 2021 at 6:01 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > The -1 assumes that last index counts all vhost device models as having
> > two queues, but they count only the ones that models the data queues.
> >
> > Because of that, the right change in last_index is to actually add the
> > cvq, not to remove the missing one.
> >
> > This is not a problem to vhost-net, but it is to vhost-vdpa, which
> > device model trust to reach the last index to finish starting the
> > device.
> >
> > Tested with vp_vdpa with host's vhost=on and vhost=off, with ctrl_vq=on
> > and ctrl_vq=off.
> >
> > 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 | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 081946dc93..fe2b8a2b83 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -329,8 +329,8 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >      int r, e, i, last_index = data_queue_pairs * 2;
> >      NetClientState *peer;
> >
> > -    if (!cvq) {
> > -        last_index -= 1;
> > +    if (cvq) {
> > +        last_index += 1;
>
> Patch looks ok.
>
> Not a native speaker but I guess "last" should be inside the closed
> interval. If this is true, I still prefer to change the check in
> vhost_vdpa_dev_start().
>

You are right, it's the way we are using it in iova address and there
are some other examples. I'll rename to end, similar to other
containers.

Thanks!

> Thanks
>
> >      }
> >
> >      if (!k->set_guest_notifiers) {
> > --
> > 2.27.0
> >
>