[PATCH v3 0/2] vhost: Fix last queue index of devices with no cvq

[PATCH v3 0/2] vhost: Fix last queue index of devices with no cvq

[Eugenio Pérez]

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 is an invalid
device by the standard, so just stick to the right number of device
models.

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.

v3:
* Recover cvq devices.
* Rename last_index to last_vq_index

v2:
* Delete all the conditional code instead of ROUND_DOWN in a
  deinitely too-bit-tricky way.

Eugenio Pérez (2):
  vhost: Rename last_index to last_vq_index
  vhost: Fix last vq queue index of devices with no cvq

 include/hw/virtio/vhost.h | 2 +-
 hw/net/vhost_net.c        | 8 ++++----
 hw/virtio/vhost-vdpa.c    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.27.0

[PATCH v3 1/2] vhost: Rename last_index to last_vq_index

[Eugenio Pérez]

The doc of this field points out that last_index is the last vq index.
Since last_index can cause confusion if seen out of context, renaming
to last_vq_index, aligning with vq_index.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost.h | 2 +-
 hw/net/vhost_net.c        | 4 ++--
 hw/virtio/vhost-vdpa.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 3fa0b554ef..8a79833b54 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -75,7 +75,7 @@ struct vhost_dev {
     /* the first virtqueue which would be used by this vhost dev */
     int vq_index;
     /* the last vq index for the virtio device (not vhost) */
-    int last_index;
+    int last_vq_index;
     /* if non-zero, minimum required value for max_queues */
     int num_queues;
     uint64_t features;
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 0d888f29a6..081946dc93 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -232,10 +232,10 @@ fail:
 }
 
 static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index,
-                                   int last_index)
+                                   int last_vq_index)
 {
     net->dev.vq_index = vq_index;
-    net->dev.last_index = last_index;
+    net->dev.last_vq_index = last_vq_index;
 }
 
 static int vhost_net_start_one(struct vhost_net *net,
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 12661fd5b1..a1831b27ca 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -632,7 +632,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
         vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
     }
 
-    if (dev->vq_index + dev->nvqs != dev->last_index) {
+    if (dev->vq_index + dev->nvqs != dev->last_vq_index) {
         return 0;
     }
 
-- 
2.27.0

Re: [PATCH v3 1/2] vhost: Rename last_index to last_vq_index

[Juan Quintela]

Eugenio Pérez <eperezma@redhat.com> wrote:
> The doc of this field points out that last_index is the last vq index.
> Since last_index can cause confusion if seen out of context, renaming
> to last_vq_index, aligning with vq_index.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

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

Re: [PATCH v3 1/2] vhost: Rename last_index to last_vq_index

[Jason Wang]

On Wed, Nov 3, 2021 at 6:01 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> The doc of this field points out that last_index is the last vq index.
> Since last_index can cause confusion if seen out of context, renaming
> to last_vq_index, aligning with vq_index.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
>  include/hw/virtio/vhost.h | 2 +-
>  hw/net/vhost_net.c        | 4 ++--
>  hw/virtio/vhost-vdpa.c    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 3fa0b554ef..8a79833b54 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -75,7 +75,7 @@ struct vhost_dev {
>      /* the first virtqueue which would be used by this vhost dev */
>      int vq_index;
>      /* the last vq index for the virtio device (not vhost) */
> -    int last_index;
> +    int last_vq_index;
>      /* if non-zero, minimum required value for max_queues */
>      int num_queues;
>      uint64_t features;
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 0d888f29a6..081946dc93 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -232,10 +232,10 @@ fail:
>  }
>
>  static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index,
> -                                   int last_index)
> +                                   int last_vq_index)
>  {
>      net->dev.vq_index = vq_index;
> -    net->dev.last_index = last_index;
> +    net->dev.last_vq_index = last_vq_index;
>  }
>
>  static int vhost_net_start_one(struct vhost_net *net,
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 12661fd5b1..a1831b27ca 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -632,7 +632,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>          vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>      }
>
> -    if (dev->vq_index + dev->nvqs != dev->last_index) {
> +    if (dev->vq_index + dev->nvqs != dev->last_vq_index) {
>          return 0;
>      }
>
> --
> 2.27.0
>

[PATCH v3 2/2] vhost: Fix last vq queue index of devices with no cvq

[Eugenio Pérez]

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

Re: [PATCH v3 2/2] vhost: Fix last vq queue index of devices with no cvq

[Juan Quintela]

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>

Re: [PATCH v3 2/2] vhost: Fix last vq queue index of devices with no cvq

[Jason Wang]

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
>

Re: [PATCH v3 2/2] vhost: Fix last vq queue index of devices with no cvq

[Eugenio Perez Martin]

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