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

[PATCH] 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 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;
     }
 
     if (!k->set_guest_notifiers) {
-- 
2.27.0

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

[Michael S. Tsirkin]

On Fri, Oct 29, 2021 at 04:16:08PM +0200, Eugenio Pérez 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;
>      }
>  
>      if (!k->set_guest_notifiers) {

could this code be made clearer?

> -- 
> 2.27.0

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

[Jason Wang]

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) {
...
}

Thanks

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

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

[Eugenio Perez Martin]

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?

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

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

[Eugenio Perez Martin]

On Sun, Oct 31, 2021 at 10:47 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Oct 29, 2021 at 04:16:08PM +0200, Eugenio Pérez 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;
> >      }
> >
> >      if (!k->set_guest_notifiers) {
>
> could this code be made clearer?
>

I can expand both for sure, but do you mean clearer because the
operation is obscure (to remove the last bit to make last_index even)
or because the need of that operation is not clear enough?

Thanks!

> > --
> > 2.27.0
>

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

[Michael S. Tsirkin]

On Mon, Nov 01, 2021 at 10:03:19AM +0100, Eugenio Perez Martin wrote:
> On Sun, Oct 31, 2021 at 10:47 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Oct 29, 2021 at 04:16:08PM +0200, Eugenio Pérez 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;
> > >      }
> > >
> > >      if (!k->set_guest_notifiers) {
> >
> > could this code be made clearer?
> >
> 
> I can expand both for sure, but do you mean clearer because the
> operation is obscure (to remove the last bit to make last_index even)
> or because the need of that operation is not clear enough?
> 
> Thanks!

I'm not sure ... both?

> > > --
> > > 2.27.0
> >

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

[Eugenio Perez Martin]

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!




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

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

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

[Jason Wang]

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.

Thanks

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

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

[Eugenio Perez Martin]

On Mon, Nov 1, 2021 at 9:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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.
>

I meant to totally delete the check, since it's assuming a device has
an odd number of virtqueues but cvq feature flag enabled. And this is
invalid in -net kind of devices.

I can send a patch with that for sure.

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

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

[Eugenio Perez Martin]

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.

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

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

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

[Eugenio Perez Martin]

On Tue, Nov 2, 2021 at 8:04 AM Jason Wang <jasowang@redhat.com> wrote:
>
> 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.

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