[PATCH] virtio: add the queue number check

Yang Zhong posted 1 patch 4 years, 3 months ago
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191223082813.28930-1-yang.zhong@intel.com
Maintainers: Fam Zheng <fam@euphon.net>, Paolo Bonzini <pbonzini@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
hw/block/vhost-user-blk.c | 11 +++++++++++
hw/block/virtio-blk.c     | 11 ++++++++++-
hw/scsi/virtio-scsi.c     | 12 ++++++++++++
3 files changed, 33 insertions(+), 1 deletion(-)
[PATCH] virtio: add the queue number check
Posted by Yang Zhong 4 years, 3 months ago
In the guest kernel driver, like virtio_blk.c and virtio_scsi.c,
there are some definitions like below:

num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs)

If the queue number is bigger than vcpu number, the VM will be
stuck in the guest driver because the qemu and guest driver have
different queue number. So, this check can avoid this issues.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 hw/block/vhost-user-blk.c | 11 +++++++++++
 hw/block/virtio-blk.c     | 11 ++++++++++-
 hw/scsi/virtio-scsi.c     | 12 ++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 63da9bb619..250e72abe4 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -23,6 +23,8 @@
 #include "qom/object.h"
 #include "hw/qdev-core.h"
 #include "hw/qdev-properties.h"
+#include "qemu/option.h"
+#include "qemu/config-file.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user-blk.h"
 #include "hw/virtio/virtio.h"
@@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostUserBlk *s = VHOST_USER_BLK(vdev);
     Error *err = NULL;
+    unsigned cpus;
     int i, ret;
 
     if (!s->chardev.chr) {
@@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
+                               "cpus", 0);
+    if (s->num_queues > cpus ) {
+        error_setg(errp, "vhost-user-blk: the queue number should be equal "
+                "or less than vcpu number");
+        return;
+    }
+
     if (!s->queue_size) {
         error_setg(errp, "vhost-user-blk: queue size must be non-zero");
         return;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index d62e6377c2..b2f4d01148 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -18,6 +18,8 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "trace.h"
+#include "qemu/option.h"
+#include "qemu/config-file.h"
 #include "hw/block/block.h"
 #include "hw/qdev-properties.h"
 #include "sysemu/blockdev.h"
@@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     VirtIOBlock *s = VIRTIO_BLK(dev);
     VirtIOBlkConf *conf = &s->conf;
     Error *err = NULL;
-    unsigned i;
+    unsigned i,cpus;
 
     if (!conf->conf.blk) {
         error_setg(errp, "drive property not set");
@@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "num-queues property must be larger than 0");
         return;
     }
+    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
+                               "cpus", 0);
+    if (conf->num_queues > cpus ) {
+        error_setg(errp, "virtio-blk: the queue number should be equal "
+                "or less than vcpu number");
+        return;
+    }
     if (!is_power_of_2(conf->queue_size) ||
         conf->queue_size > VIRTQUEUE_MAX_SIZE) {
         error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e8b2b64d09..8e3e44f6b9 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -21,6 +21,8 @@
 #include "qemu/error-report.h"
 #include "qemu/iov.h"
 #include "qemu/module.h"
+#include "qemu/option.h"
+#include "qemu/config-file.h"
 #include "sysemu/block-backend.h"
 #include "hw/qdev-properties.h"
 #include "hw/scsi/scsi.h"
@@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
+    unsigned cpus;
     int i;
 
     virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
@@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
         virtio_cleanup(vdev);
         return;
     }
+
+    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
+                               "cpus", 0);
+    if (s->conf.num_queues > cpus ) {
+        error_setg(errp, "virtio-scsi: the queue number should be equal "
+                "or less than vcpu number");
+        return;
+    }
+
     s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
     s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
-- 
2.17.1


Re: [PATCH] virtio: add the queue number check
Posted by Paolo Bonzini 4 years, 3 months ago
On 23/12/19 09:28, Yang Zhong wrote:
> In the guest kernel driver, like virtio_blk.c and virtio_scsi.c,
> there are some definitions like below:
> 
> num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs)
> 
> If the queue number is bigger than vcpu number, the VM will be
> stuck in the guest driver because the qemu and guest driver have
> different queue number. So, this check can avoid this issues.

Can you explain how the bug happens? This would be an issue in the
kernel driver, QEMU shouldn't care about how the kernel driver chooses
to steer requests to virtqueues.

Paolo

> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  hw/block/vhost-user-blk.c | 11 +++++++++++
>  hw/block/virtio-blk.c     | 11 ++++++++++-
>  hw/scsi/virtio-scsi.c     | 12 ++++++++++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 63da9bb619..250e72abe4 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -23,6 +23,8 @@
>  #include "qom/object.h"
>  #include "hw/qdev-core.h"
>  #include "hw/qdev-properties.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
>  #include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-user-blk.h"
>  #include "hw/virtio/virtio.h"
> @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
>      Error *err = NULL;
> +    unsigned cpus;
>      int i, ret;
>  
>      if (!s->chardev.chr) {
> @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> +                               "cpus", 0);
> +    if (s->num_queues > cpus ) {
> +        error_setg(errp, "vhost-user-blk: the queue number should be equal "
> +                "or less than vcpu number");
> +        return;
> +    }
> +
>      if (!s->queue_size) {
>          error_setg(errp, "vhost-user-blk: queue size must be non-zero");
>          return;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index d62e6377c2..b2f4d01148 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -18,6 +18,8 @@
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
>  #include "trace.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
>  #include "hw/block/block.h"
>  #include "hw/qdev-properties.h"
>  #include "sysemu/blockdev.h"
> @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>      VirtIOBlock *s = VIRTIO_BLK(dev);
>      VirtIOBlkConf *conf = &s->conf;
>      Error *err = NULL;
> -    unsigned i;
> +    unsigned i,cpus;
>  
>      if (!conf->conf.blk) {
>          error_setg(errp, "drive property not set");
> @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          error_setg(errp, "num-queues property must be larger than 0");
>          return;
>      }
> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> +                               "cpus", 0);
> +    if (conf->num_queues > cpus ) {
> +        error_setg(errp, "virtio-blk: the queue number should be equal "
> +                "or less than vcpu number");
> +        return;
> +    }
>      if (!is_power_of_2(conf->queue_size) ||
>          conf->queue_size > VIRTQUEUE_MAX_SIZE) {
>          error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index e8b2b64d09..8e3e44f6b9 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -21,6 +21,8 @@
>  #include "qemu/error-report.h"
>  #include "qemu/iov.h"
>  #include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
>  #include "sysemu/block-backend.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/scsi/scsi.h"
> @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
> +    unsigned cpus;
>      int i;
>  
>      virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
> @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
>          virtio_cleanup(vdev);
>          return;
>      }
> +
> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> +                               "cpus", 0);
> +    if (s->conf.num_queues > cpus ) {
> +        error_setg(errp, "virtio-scsi: the queue number should be equal "
> +                "or less than vcpu number");
> +        return;
> +    }
> +
>      s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
>      s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
>      s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
> 


Re: [PATCH] virtio: add the queue number check
Posted by Yang Zhong 4 years, 3 months ago
On Mon, Dec 23, 2019 at 09:44:46AM +0100, Paolo Bonzini wrote:
> On 23/12/19 09:28, Yang Zhong wrote:
> > In the guest kernel driver, like virtio_blk.c and virtio_scsi.c,
> > there are some definitions like below:
> > 
> > num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs)
> > 
> > If the queue number is bigger than vcpu number, the VM will be
> > stuck in the guest driver because the qemu and guest driver have
> > different queue number. So, this check can avoid this issues.
> 
> Can you explain how the bug happens? This would be an issue in the
> kernel driver, QEMU shouldn't care about how the kernel driver chooses
> to steer requests to virtqueues.
> 
  Paolo, Merry Christmas!

  This issue is easy to reproduce, when i enabled multiple queue with SPDK,
  and if the num-queues > cpu number in the qemu command, the guest kernel
  will stuck there and after guest kernel do "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
  , the guest kernel will crash.

  My qemu command:
  ./qemu-system-x86_64 \
    -machine q35,accel=kvm \
    -cpu host -m 1024,maxmem=20G,slots=2 -smp 2 \
    -chardev socket,id=char0,path=/var/tmp/vhost.1 \
    -device vhost-user-blk-pci,chardev=char0,num-queues=4,bootindex=2,config-wce=true \
    ......

  My debug patch as below:
  diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
  index 7ffd719d89de..b0a2fb503c00 100644
  --- a/drivers/block/virtio_blk.c
  +++ b/drivers/block/virtio_blk.c
  @@ -511,11 +511,12 @@ static int init_vq(struct virtio_blk *vblk)
          err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
                                     struct virtio_blk_config, num_queues,
                                     &num_vqs);
  +printk("---0------blk_probe and init_vq, the num_queues=%d---\n", num_vqs);
        if (err)
                num_vqs = 1;
  -
  +printk("---1------blk_probe and init_vq, the num_queues=%d---\n", num_vqs);
        num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
  -
  +printk("---2------blk_probe and init_vq, the num_queues=%d---\n", num_vqs);
        vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);

  The debug log:
  [    0.225780] Linux agpgart interface v0.103
  [    0.226908] loop: module loaded
  [    0.227079] ---0------blk_probe and init_vq, the num_queues=4---
  [    0.227192] ---1------blk_probe and init_vq, the num_queues=4---
  [    0.227260] ---2------blk_probe and init_vq, the num_queues=2---
  [    0.227813] virtio_blk virtio1: [vda] 1048576 512-byte logical blocks (537 MB/512 MiB)

  In this time, the queue number in the front-end block driver is 2, but
  the queue number in qemu side is still 4. So the guest virtio_blk
  driver will failed to create vq with backend. There is no "set back"
  mechnism for block driver to inform backend this new queue number.
  So, i added this check in qemu side.

  Since the current virtio-blk and vhost-user-blk device always
  defaultly use 1 queue, it's hard to find this issue.

  I checked the guest kernel driver, virtio-scsi and virtio-blk all
  have same check in their driver probe:

  num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
 
  It's possible the guest driver has different queue number with qemu
  side.

  I also want to fix this issue from guest driver side, but currently there 
  is no better solution to fix this issue.

  By the way, i did not try scsi with this corner case, and only check
  driver and qemu code to find same issue. thanks! 

  Yang

> Paolo
> 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  hw/block/vhost-user-blk.c | 11 +++++++++++
> >  hw/block/virtio-blk.c     | 11 ++++++++++-
> >  hw/scsi/virtio-scsi.c     | 12 ++++++++++++
> >  3 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 63da9bb619..250e72abe4 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -23,6 +23,8 @@
> >  #include "qom/object.h"
> >  #include "hw/qdev-core.h"
> >  #include "hw/qdev-properties.h"
> > +#include "qemu/option.h"
> > +#include "qemu/config-file.h"
> >  #include "hw/virtio/vhost.h"
> >  #include "hw/virtio/vhost-user-blk.h"
> >  #include "hw/virtio/virtio.h"
> > @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> >      Error *err = NULL;
> > +    unsigned cpus;
> >      int i, ret;
> >  
> >      if (!s->chardev.chr) {
> > @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >  
> > +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> > +                               "cpus", 0);
> > +    if (s->num_queues > cpus ) {
> > +        error_setg(errp, "vhost-user-blk: the queue number should be equal "
> > +                "or less than vcpu number");
> > +        return;
> > +    }
> > +
> >      if (!s->queue_size) {
> >          error_setg(errp, "vhost-user-blk: queue size must be non-zero");
> >          return;
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index d62e6377c2..b2f4d01148 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -18,6 +18,8 @@
> >  #include "qemu/error-report.h"
> >  #include "qemu/main-loop.h"
> >  #include "trace.h"
> > +#include "qemu/option.h"
> > +#include "qemu/config-file.h"
> >  #include "hw/block/block.h"
> >  #include "hw/qdev-properties.h"
> >  #include "sysemu/blockdev.h"
> > @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >      VirtIOBlock *s = VIRTIO_BLK(dev);
> >      VirtIOBlkConf *conf = &s->conf;
> >      Error *err = NULL;
> > -    unsigned i;
> > +    unsigned i,cpus;
> >  
> >      if (!conf->conf.blk) {
> >          error_setg(errp, "drive property not set");
> > @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >          error_setg(errp, "num-queues property must be larger than 0");
> >          return;
> >      }
> > +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> > +                               "cpus", 0);
> > +    if (conf->num_queues > cpus ) {
> > +        error_setg(errp, "virtio-blk: the queue number should be equal "
> > +                "or less than vcpu number");
> > +        return;
> > +    }
> >      if (!is_power_of_2(conf->queue_size) ||
> >          conf->queue_size > VIRTQUEUE_MAX_SIZE) {
> >          error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index e8b2b64d09..8e3e44f6b9 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -21,6 +21,8 @@
> >  #include "qemu/error-report.h"
> >  #include "qemu/iov.h"
> >  #include "qemu/module.h"
> > +#include "qemu/option.h"
> > +#include "qemu/config-file.h"
> >  #include "sysemu/block-backend.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/scsi/scsi.h"
> > @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
> > +    unsigned cpus;
> >      int i;
> >  
> >      virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
> > @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
> >          virtio_cleanup(vdev);
> >          return;
> >      }
> > +
> > +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> > +                               "cpus", 0);
> > +    if (s->conf.num_queues > cpus ) {
> > +        error_setg(errp, "virtio-scsi: the queue number should be equal "
> > +                "or less than vcpu number");
> > +        return;
> > +    }
> > +
> >      s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
> >      s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
> >      s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
> > 

Re: [PATCH] virtio: add the queue number check
Posted by Paolo Bonzini 4 years, 3 months ago
On 23/12/19 10:18, Yang Zhong wrote:
>   In this time, the queue number in the front-end block driver is 2, but
>   the queue number in qemu side is still 4. So the guest virtio_blk
>   driver will failed to create vq with backend.

Where?

>   There is no "set back"
>   mechnism for block driver to inform backend this new queue number.
>   So, i added this check in qemu side.

Perhaps the guest kernel should still create the virtqueues, and just
not use them.  In any case, now that you have explained it, it is
certainly a guest bug.

Paolo

>   Since the current virtio-blk and vhost-user-blk device always
>   defaultly use 1 queue, it's hard to find this issue.
> 
>   I checked the guest kernel driver, virtio-scsi and virtio-blk all
>   have same check in their driver probe:
> 
>   num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
>  
>   It's possible the guest driver has different queue number with qemu
>   side.
> 
>   I also want to fix this issue from guest driver side, but currently there 
>   is no better solution to fix this issue.
> 
>   By the way, i did not try scsi with this corner case, and only check
>   driver and qemu code to find same issue. thanks! 
> 
>   Yang
> 
>> Paolo
>>
>>> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
>>> ---
>>>  hw/block/vhost-user-blk.c | 11 +++++++++++
>>>  hw/block/virtio-blk.c     | 11 ++++++++++-
>>>  hw/scsi/virtio-scsi.c     | 12 ++++++++++++
>>>  3 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index 63da9bb619..250e72abe4 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -23,6 +23,8 @@
>>>  #include "qom/object.h"
>>>  #include "hw/qdev-core.h"
>>>  #include "hw/qdev-properties.h"
>>> +#include "qemu/option.h"
>>> +#include "qemu/config-file.h"
>>>  #include "hw/virtio/vhost.h"
>>>  #include "hw/virtio/vhost-user-blk.h"
>>>  #include "hw/virtio/virtio.h"
>>> @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>>      Error *err = NULL;
>>> +    unsigned cpus;
>>>      int i, ret;
>>>  
>>>      if (!s->chardev.chr) {
>>> @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>>>          return;
>>>      }
>>>  
>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
>>> +                               "cpus", 0);
>>> +    if (s->num_queues > cpus ) {
>>> +        error_setg(errp, "vhost-user-blk: the queue number should be equal "
>>> +                "or less than vcpu number");
>>> +        return;
>>> +    }
>>> +
>>>      if (!s->queue_size) {
>>>          error_setg(errp, "vhost-user-blk: queue size must be non-zero");
>>>          return;
>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>>> index d62e6377c2..b2f4d01148 100644
>>> --- a/hw/block/virtio-blk.c
>>> +++ b/hw/block/virtio-blk.c
>>> @@ -18,6 +18,8 @@
>>>  #include "qemu/error-report.h"
>>>  #include "qemu/main-loop.h"
>>>  #include "trace.h"
>>> +#include "qemu/option.h"
>>> +#include "qemu/config-file.h"
>>>  #include "hw/block/block.h"
>>>  #include "hw/qdev-properties.h"
>>>  #include "sysemu/blockdev.h"
>>> @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>>      VirtIOBlock *s = VIRTIO_BLK(dev);
>>>      VirtIOBlkConf *conf = &s->conf;
>>>      Error *err = NULL;
>>> -    unsigned i;
>>> +    unsigned i,cpus;
>>>  
>>>      if (!conf->conf.blk) {
>>>          error_setg(errp, "drive property not set");
>>> @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>>          error_setg(errp, "num-queues property must be larger than 0");
>>>          return;
>>>      }
>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
>>> +                               "cpus", 0);
>>> +    if (conf->num_queues > cpus ) {
>>> +        error_setg(errp, "virtio-blk: the queue number should be equal "
>>> +                "or less than vcpu number");
>>> +        return;
>>> +    }
>>>      if (!is_power_of_2(conf->queue_size) ||
>>>          conf->queue_size > VIRTQUEUE_MAX_SIZE) {
>>>          error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>>> index e8b2b64d09..8e3e44f6b9 100644
>>> --- a/hw/scsi/virtio-scsi.c
>>> +++ b/hw/scsi/virtio-scsi.c
>>> @@ -21,6 +21,8 @@
>>>  #include "qemu/error-report.h"
>>>  #include "qemu/iov.h"
>>>  #include "qemu/module.h"
>>> +#include "qemu/option.h"
>>> +#include "qemu/config-file.h"
>>>  #include "sysemu/block-backend.h"
>>>  #include "hw/qdev-properties.h"
>>>  #include "hw/scsi/scsi.h"
>>> @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
>>>  {
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
>>> +    unsigned cpus;
>>>      int i;
>>>  
>>>      virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
>>> @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
>>>          virtio_cleanup(vdev);
>>>          return;
>>>      }
>>> +
>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
>>> +                               "cpus", 0);
>>> +    if (s->conf.num_queues > cpus ) {
>>> +        error_setg(errp, "virtio-scsi: the queue number should be equal "
>>> +                "or less than vcpu number");
>>> +        return;
>>> +    }
>>> +
>>>      s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
>>>      s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
>>>      s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
>>>
> 


Re: [PATCH] virtio: add the queue number check
Posted by Michael S. Tsirkin 4 years, 3 months ago
On Mon, Dec 23, 2019 at 12:02:18PM +0100, Paolo Bonzini wrote:
> On 23/12/19 10:18, Yang Zhong wrote:
> >   In this time, the queue number in the front-end block driver is 2, but
> >   the queue number in qemu side is still 4. So the guest virtio_blk
> >   driver will failed to create vq with backend.
> 
> Where?
> 
> >   There is no "set back"
> >   mechnism for block driver to inform backend this new queue number.
> >   So, i added this check in qemu side.
> 
> Perhaps the guest kernel should still create the virtqueues, and just
> not use them.  In any case, now that you have explained it, it is
> certainly a guest bug.
> 
> Paolo


Paolo do you understand where the bug is?
E.g. I see this in vhost user block:

    /* Kick right away to begin processing requests already in vring */
    for (i = 0; i < s->dev.nvqs; i++) {
        VirtQueue *kick_vq = virtio_get_queue(vdev, i);

        if (!virtio_queue_get_desc_addr(vdev, i)) {
            continue;
        }
        event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
    }

which is an (admittedly hacky) want to skip VQs which
were not configured by guest ....


> >   Since the current virtio-blk and vhost-user-blk device always
> >   defaultly use 1 queue, it's hard to find this issue.
> > 
> >   I checked the guest kernel driver, virtio-scsi and virtio-blk all
> >   have same check in their driver probe:
> > 
> >   num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
> >  
> >   It's possible the guest driver has different queue number with qemu
> >   side.
> > 
> >   I also want to fix this issue from guest driver side, but currently there 
> >   is no better solution to fix this issue.
> > 
> >   By the way, i did not try scsi with this corner case, and only check
> >   driver and qemu code to find same issue. thanks! 
> > 
> >   Yang
> > 
> >> Paolo
> >>
> >>> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> >>> ---
> >>>  hw/block/vhost-user-blk.c | 11 +++++++++++
> >>>  hw/block/virtio-blk.c     | 11 ++++++++++-
> >>>  hw/scsi/virtio-scsi.c     | 12 ++++++++++++
> >>>  3 files changed, 33 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >>> index 63da9bb619..250e72abe4 100644
> >>> --- a/hw/block/vhost-user-blk.c
> >>> +++ b/hw/block/vhost-user-blk.c
> >>> @@ -23,6 +23,8 @@
> >>>  #include "qom/object.h"
> >>>  #include "hw/qdev-core.h"
> >>>  #include "hw/qdev-properties.h"
> >>> +#include "qemu/option.h"
> >>> +#include "qemu/config-file.h"
> >>>  #include "hw/virtio/vhost.h"
> >>>  #include "hw/virtio/vhost-user-blk.h"
> >>>  #include "hw/virtio/virtio.h"
> >>> @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> >>>      Error *err = NULL;
> >>> +    unsigned cpus;
> >>>      int i, ret;
> >>>  
> >>>      if (!s->chardev.chr) {
> >>> @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >>>          return;
> >>>      }
> >>>  
> >>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> >>> +                               "cpus", 0);
> >>> +    if (s->num_queues > cpus ) {
> >>> +        error_setg(errp, "vhost-user-blk: the queue number should be equal "
> >>> +                "or less than vcpu number");
> >>> +        return;
> >>> +    }
> >>> +
> >>>      if (!s->queue_size) {
> >>>          error_setg(errp, "vhost-user-blk: queue size must be non-zero");
> >>>          return;
> >>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >>> index d62e6377c2..b2f4d01148 100644
> >>> --- a/hw/block/virtio-blk.c
> >>> +++ b/hw/block/virtio-blk.c
> >>> @@ -18,6 +18,8 @@
> >>>  #include "qemu/error-report.h"
> >>>  #include "qemu/main-loop.h"
> >>>  #include "trace.h"
> >>> +#include "qemu/option.h"
> >>> +#include "qemu/config-file.h"
> >>>  #include "hw/block/block.h"
> >>>  #include "hw/qdev-properties.h"
> >>>  #include "sysemu/blockdev.h"
> >>> @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >>>      VirtIOBlock *s = VIRTIO_BLK(dev);
> >>>      VirtIOBlkConf *conf = &s->conf;
> >>>      Error *err = NULL;
> >>> -    unsigned i;
> >>> +    unsigned i,cpus;
> >>>  
> >>>      if (!conf->conf.blk) {
> >>>          error_setg(errp, "drive property not set");
> >>> @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >>>          error_setg(errp, "num-queues property must be larger than 0");
> >>>          return;
> >>>      }
> >>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> >>> +                               "cpus", 0);
> >>> +    if (conf->num_queues > cpus ) {
> >>> +        error_setg(errp, "virtio-blk: the queue number should be equal "
> >>> +                "or less than vcpu number");
> >>> +        return;
> >>> +    }
> >>>      if (!is_power_of_2(conf->queue_size) ||
> >>>          conf->queue_size > VIRTQUEUE_MAX_SIZE) {
> >>>          error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
> >>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> >>> index e8b2b64d09..8e3e44f6b9 100644
> >>> --- a/hw/scsi/virtio-scsi.c
> >>> +++ b/hw/scsi/virtio-scsi.c
> >>> @@ -21,6 +21,8 @@
> >>>  #include "qemu/error-report.h"
> >>>  #include "qemu/iov.h"
> >>>  #include "qemu/module.h"
> >>> +#include "qemu/option.h"
> >>> +#include "qemu/config-file.h"
> >>>  #include "sysemu/block-backend.h"
> >>>  #include "hw/qdev-properties.h"
> >>>  #include "hw/scsi/scsi.h"
> >>> @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
> >>>  {
> >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
> >>> +    unsigned cpus;
> >>>      int i;
> >>>  
> >>>      virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
> >>> @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
> >>>          virtio_cleanup(vdev);
> >>>          return;
> >>>      }
> >>> +
> >>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> >>> +                               "cpus", 0);
> >>> +    if (s->conf.num_queues > cpus ) {
> >>> +        error_setg(errp, "virtio-scsi: the queue number should be equal "
> >>> +                "or less than vcpu number");
> >>> +        return;
> >>> +    }
> >>> +
> >>>      s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
> >>>      s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
> >>>      s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
> >>>
> > 


Re: [PATCH] virtio: add the queue number check
Posted by Paolo Bonzini 4 years, 3 months ago
On 23/12/19 15:25, Michael S. Tsirkin wrote:
> On Mon, Dec 23, 2019 at 12:02:18PM +0100, Paolo Bonzini wrote:
>> On 23/12/19 10:18, Yang Zhong wrote:
>>>   In this time, the queue number in the front-end block driver is 2, but
>>>   the queue number in qemu side is still 4. So the guest virtio_blk
>>>   driver will failed to create vq with backend.
>>
>> Where?
>>
>>>   There is no "set back"
>>>   mechnism for block driver to inform backend this new queue number.
>>>   So, i added this check in qemu side.
>>
>> Perhaps the guest kernel should still create the virtqueues, and just
>> not use them.  In any case, now that you have explained it, it is
>> certainly a guest bug.
> 
> Paolo do you understand where the bug is?

No, I asked above where does the virtio_blk driver fail to create the
virtqueues.  But it shouldn't; it is legal for the guest not to
configure all virtqueues, and QEMU knows to ignore the extra ones.  For
example, firmware can ignore virtio-scsi request queues above the first,
or ignore the virtio-scsi control and event queues (see
src/hw/virtio-scsi.c in SeaBIOS, it only calls vp_find_vq with index 2).

In particular this is the reason why request queues for virtio-scsi are
numbered 2 and above, rather than starting from zero: this way, the
guest can just pretend that unnecessary queues do not exist, and still
keep the virtqueue numbers consecutive.

> E.g. I see this in vhost user block:
> 
>     /* Kick right away to begin processing requests already in vring */
>     for (i = 0; i < s->dev.nvqs; i++) {
>         VirtQueue *kick_vq = virtio_get_queue(vdev, i);
> 
>         if (!virtio_queue_get_desc_addr(vdev, i)) {
>             continue;
>         }
>         event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
>     }
> 
> which is an (admittedly hacky) want to skip VQs which
> were not configured by guest ....

Right, this is an example of QEMU ignoring extra virtqueues.

Paolo

> 
> 
>>>   Since the current virtio-blk and vhost-user-blk device always
>>>   defaultly use 1 queue, it's hard to find this issue.
>>>
>>>   I checked the guest kernel driver, virtio-scsi and virtio-blk all
>>>   have same check in their driver probe:
>>>
>>>   num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
>>>  
>>>   It's possible the guest driver has different queue number with qemu
>>>   side.
>>>
>>>   I also want to fix this issue from guest driver side, but currently there 
>>>   is no better solution to fix this issue.
>>>
>>>   By the way, i did not try scsi with this corner case, and only check
>>>   driver and qemu code to find same issue. thanks! 
>>>
>>>   Yang
>>>
>>>> Paolo
>>>>
>>>>> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
>>>>> ---
>>>>>  hw/block/vhost-user-blk.c | 11 +++++++++++
>>>>>  hw/block/virtio-blk.c     | 11 ++++++++++-
>>>>>  hw/scsi/virtio-scsi.c     | 12 ++++++++++++
>>>>>  3 files changed, 33 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>>>> index 63da9bb619..250e72abe4 100644
>>>>> --- a/hw/block/vhost-user-blk.c
>>>>> +++ b/hw/block/vhost-user-blk.c
>>>>> @@ -23,6 +23,8 @@
>>>>>  #include "qom/object.h"
>>>>>  #include "hw/qdev-core.h"
>>>>>  #include "hw/qdev-properties.h"
>>>>> +#include "qemu/option.h"
>>>>> +#include "qemu/config-file.h"
>>>>>  #include "hw/virtio/vhost.h"
>>>>>  #include "hw/virtio/vhost-user-blk.h"
>>>>>  #include "hw/virtio/virtio.h"
>>>>> @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>>>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>>>>      Error *err = NULL;
>>>>> +    unsigned cpus;
>>>>>      int i, ret;
>>>>>  
>>>>>      if (!s->chardev.chr) {
>>>>> @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>>>>>          return;
>>>>>      }
>>>>>  
>>>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
>>>>> +                               "cpus", 0);
>>>>> +    if (s->num_queues > cpus ) {
>>>>> +        error_setg(errp, "vhost-user-blk: the queue number should be equal "
>>>>> +                "or less than vcpu number");
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>>      if (!s->queue_size) {
>>>>>          error_setg(errp, "vhost-user-blk: queue size must be non-zero");
>>>>>          return;
>>>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>>>>> index d62e6377c2..b2f4d01148 100644
>>>>> --- a/hw/block/virtio-blk.c
>>>>> +++ b/hw/block/virtio-blk.c
>>>>> @@ -18,6 +18,8 @@
>>>>>  #include "qemu/error-report.h"
>>>>>  #include "qemu/main-loop.h"
>>>>>  #include "trace.h"
>>>>> +#include "qemu/option.h"
>>>>> +#include "qemu/config-file.h"
>>>>>  #include "hw/block/block.h"
>>>>>  #include "hw/qdev-properties.h"
>>>>>  #include "sysemu/blockdev.h"
>>>>> @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>>>>      VirtIOBlock *s = VIRTIO_BLK(dev);
>>>>>      VirtIOBlkConf *conf = &s->conf;
>>>>>      Error *err = NULL;
>>>>> -    unsigned i;
>>>>> +    unsigned i,cpus;
>>>>>  
>>>>>      if (!conf->conf.blk) {
>>>>>          error_setg(errp, "drive property not set");
>>>>> @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>>>>          error_setg(errp, "num-queues property must be larger than 0");
>>>>>          return;
>>>>>      }
>>>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
>>>>> +                               "cpus", 0);
>>>>> +    if (conf->num_queues > cpus ) {
>>>>> +        error_setg(errp, "virtio-blk: the queue number should be equal "
>>>>> +                "or less than vcpu number");
>>>>> +        return;
>>>>> +    }
>>>>>      if (!is_power_of_2(conf->queue_size) ||
>>>>>          conf->queue_size > VIRTQUEUE_MAX_SIZE) {
>>>>>          error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
>>>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>>>>> index e8b2b64d09..8e3e44f6b9 100644
>>>>> --- a/hw/scsi/virtio-scsi.c
>>>>> +++ b/hw/scsi/virtio-scsi.c
>>>>> @@ -21,6 +21,8 @@
>>>>>  #include "qemu/error-report.h"
>>>>>  #include "qemu/iov.h"
>>>>>  #include "qemu/module.h"
>>>>> +#include "qemu/option.h"
>>>>> +#include "qemu/config-file.h"
>>>>>  #include "sysemu/block-backend.h"
>>>>>  #include "hw/qdev-properties.h"
>>>>>  #include "hw/scsi/scsi.h"
>>>>> @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
>>>>>  {
>>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>>>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
>>>>> +    unsigned cpus;
>>>>>      int i;
>>>>>  
>>>>>      virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
>>>>> @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
>>>>>          virtio_cleanup(vdev);
>>>>>          return;
>>>>>      }
>>>>> +
>>>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
>>>>> +                               "cpus", 0);
>>>>> +    if (s->conf.num_queues > cpus ) {
>>>>> +        error_setg(errp, "virtio-scsi: the queue number should be equal "
>>>>> +                "or less than vcpu number");
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>>      s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
>>>>>      s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
>>>>>      s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
>>>>>
>>>
> 


Re: [PATCH] virtio: add the queue number check
Posted by Yang Zhong 4 years, 2 months ago
On Mon, Dec 23, 2019 at 06:33:26PM +0100, Paolo Bonzini wrote:
> On 23/12/19 15:25, Michael S. Tsirkin wrote:
> > On Mon, Dec 23, 2019 at 12:02:18PM +0100, Paolo Bonzini wrote:
> >> On 23/12/19 10:18, Yang Zhong wrote:
> >>>   In this time, the queue number in the front-end block driver is 2, but
> >>>   the queue number in qemu side is still 4. So the guest virtio_blk
> >>>   driver will failed to create vq with backend.
> >>
> >> Where?
> >>
> >>>   There is no "set back"
> >>>   mechnism for block driver to inform backend this new queue number.
> >>>   So, i added this check in qemu side.
> >>
> >> Perhaps the guest kernel should still create the virtqueues, and just
> >> not use them.  In any case, now that you have explained it, it is
> >> certainly a guest bug.
> > 
> > Paolo do you understand where the bug is?
> 
> No, I asked above where does the virtio_blk driver fail to create the
> virtqueues.  But it shouldn't; it is legal for the guest not to
> configure all virtqueues, and QEMU knows to ignore the extra ones.  For
> example, firmware can ignore virtio-scsi request queues above the first,
> or ignore the virtio-scsi control and event queues (see
> src/hw/virtio-scsi.c in SeaBIOS, it only calls vp_find_vq with index 2).
> 
> In particular this is the reason why request queues for virtio-scsi are
> numbered 2 and above, rather than starting from zero: this way, the
> guest can just pretend that unnecessary queues do not exist, and still
> keep the virtqueue numbers consecutive.
> 
> > E.g. I see this in vhost user block:
> > 
> >     /* Kick right away to begin processing requests already in vring */
> >     for (i = 0; i < s->dev.nvqs; i++) {
> >         VirtQueue *kick_vq = virtio_get_queue(vdev, i);
> > 
> >         if (!virtio_queue_get_desc_addr(vdev, i)) {
> >             continue;
> >         }
> >         event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
> >     }
> > 
> > which is an (admittedly hacky) want to skip VQs which
> > were not configured by guest ....
> 
> Right, this is an example of QEMU ignoring extra virtqueues.
> 
  Thanks Paolo and Michael's comments.

  Yes, the Qemu should ignore those extra vqs when guest virtio-blk
  driver do min(cpu,num_vqs) in the probe function.

  I also tried virtio-blk device like below:
  https://patchwork.kernel.org/cover/10873193/

  The virtio-blk can work with this changes, but vhost-user-blk device
  failed with this kernel patch.

  in vhost_virtqueue_start() function, below operation to check if the
  desc addr set by guest kernel. This will ignore the extra vqs.
    a = virtio_queue_get_desc_addr(vdev, idx);
    if (a == 0) {
        /* Queue might not be ready for start */
        return 0;
    }

  If guest kernel add min(cpu,num_vqs), do we need add same check in
  realize function of vhost-user-blk device?

+    QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
+    cpus = qemu_opt_get_number(opts, "maxcpus", 0);
+    if (!cpus)
+        cpus = qemu_opt_get_number(opts, "cpus", 0);
+
+    s->num_queues = MIN(s->num_queues, cpus);

     if (!vhost_user_init(&s->vhost_user, &s->chardev, errp)) {
         return;

   If this patch is suitable for this issue, i will send this.
   or i will continue to check better solution in qemu and guest kernel.
   Thanks a lot!

   Yang

> Paolo
> 
> > 
> > 
> >>>   Since the current virtio-blk and vhost-user-blk device always
> >>>   defaultly use 1 queue, it's hard to find this issue.
> >>>
> >>>   I checked the guest kernel driver, virtio-scsi and virtio-blk all
> >>>   have same check in their driver probe:
> >>>
> >>>   num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
> >>>  
> >>>   It's possible the guest driver has different queue number with qemu
> >>>   side.
> >>>
> >>>   I also want to fix this issue from guest driver side, but currently there 
> >>>   is no better solution to fix this issue.
> >>>
> >>>   By the way, i did not try scsi with this corner case, and only check
> >>>   driver and qemu code to find same issue. thanks! 
> >>>
> >>>   Yang
> >>>
> >>>> Paolo
> >>>>
> >>>>> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> >>>>> ---
> >>>>>  hw/block/vhost-user-blk.c | 11 +++++++++++
> >>>>>  hw/block/virtio-blk.c     | 11 ++++++++++-
> >>>>>  hw/scsi/virtio-scsi.c     | 12 ++++++++++++
> >>>>>  3 files changed, 33 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >>>>> index 63da9bb619..250e72abe4 100644
> >>>>> --- a/hw/block/vhost-user-blk.c
> >>>>> +++ b/hw/block/vhost-user-blk.c
> >>>>> @@ -23,6 +23,8 @@
> >>>>>  #include "qom/object.h"
> >>>>>  #include "hw/qdev-core.h"
> >>>>>  #include "hw/qdev-properties.h"
> >>>>> +#include "qemu/option.h"
> >>>>> +#include "qemu/config-file.h"
> >>>>>  #include "hw/virtio/vhost.h"
> >>>>>  #include "hw/virtio/vhost-user-blk.h"
> >>>>>  #include "hw/virtio/virtio.h"
> >>>>> @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>>>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> >>>>>      Error *err = NULL;
> >>>>> +    unsigned cpus;
> >>>>>      int i, ret;
> >>>>>  
> >>>>>      if (!s->chardev.chr) {
> >>>>> @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >>>>>          return;
> >>>>>      }
> >>>>>  
> >>>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> >>>>> +                               "cpus", 0);
> >>>>> +    if (s->num_queues > cpus ) {
> >>>>> +        error_setg(errp, "vhost-user-blk: the queue number should be equal "
> >>>>> +                "or less than vcpu number");
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>>>      if (!s->queue_size) {
> >>>>>          error_setg(errp, "vhost-user-blk: queue size must be non-zero");
> >>>>>          return;
> >>>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >>>>> index d62e6377c2..b2f4d01148 100644
> >>>>> --- a/hw/block/virtio-blk.c
> >>>>> +++ b/hw/block/virtio-blk.c
> >>>>> @@ -18,6 +18,8 @@
> >>>>>  #include "qemu/error-report.h"
> >>>>>  #include "qemu/main-loop.h"
> >>>>>  #include "trace.h"
> >>>>> +#include "qemu/option.h"
> >>>>> +#include "qemu/config-file.h"
> >>>>>  #include "hw/block/block.h"
> >>>>>  #include "hw/qdev-properties.h"
> >>>>>  #include "sysemu/blockdev.h"
> >>>>> @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >>>>>      VirtIOBlock *s = VIRTIO_BLK(dev);
> >>>>>      VirtIOBlkConf *conf = &s->conf;
> >>>>>      Error *err = NULL;
> >>>>> -    unsigned i;
> >>>>> +    unsigned i,cpus;
> >>>>>  
> >>>>>      if (!conf->conf.blk) {
> >>>>>          error_setg(errp, "drive property not set");
> >>>>> @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> >>>>>          error_setg(errp, "num-queues property must be larger than 0");
> >>>>>          return;
> >>>>>      }
> >>>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> >>>>> +                               "cpus", 0);
> >>>>> +    if (conf->num_queues > cpus ) {
> >>>>> +        error_setg(errp, "virtio-blk: the queue number should be equal "
> >>>>> +                "or less than vcpu number");
> >>>>> +        return;
> >>>>> +    }
> >>>>>      if (!is_power_of_2(conf->queue_size) ||
> >>>>>          conf->queue_size > VIRTQUEUE_MAX_SIZE) {
> >>>>>          error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
> >>>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> >>>>> index e8b2b64d09..8e3e44f6b9 100644
> >>>>> --- a/hw/scsi/virtio-scsi.c
> >>>>> +++ b/hw/scsi/virtio-scsi.c
> >>>>> @@ -21,6 +21,8 @@
> >>>>>  #include "qemu/error-report.h"
> >>>>>  #include "qemu/iov.h"
> >>>>>  #include "qemu/module.h"
> >>>>> +#include "qemu/option.h"
> >>>>> +#include "qemu/config-file.h"
> >>>>>  #include "sysemu/block-backend.h"
> >>>>>  #include "hw/qdev-properties.h"
> >>>>>  #include "hw/scsi/scsi.h"
> >>>>> @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
> >>>>>  {
> >>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>>>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
> >>>>> +    unsigned cpus;
> >>>>>      int i;
> >>>>>  
> >>>>>      virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
> >>>>> @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
> >>>>>          virtio_cleanup(vdev);
> >>>>>          return;
> >>>>>      }
> >>>>> +
> >>>>> +    cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"), NULL),
> >>>>> +                               "cpus", 0);
> >>>>> +    if (s->conf.num_queues > cpus ) {
> >>>>> +        error_setg(errp, "virtio-scsi: the queue number should be equal "
> >>>>> +                "or less than vcpu number");
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>>>      s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
> >>>>>      s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
> >>>>>      s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
> >>>>>
> >>>
> > 

Re: [PATCH] virtio: add the queue number check
Posted by Paolo Bonzini 4 years, 2 months ago
Il ven 3 gen 2020, 16:08 Yang Zhong <yang.zhong@intel.com> ha scritto:

>   I also tried virtio-blk device like below:
>   https://patchwork.kernel.org/cover/10873193/
>
>   The virtio-blk can work with this changes, but vhost-user-blk device
>   failed with this kernel patch.
>
>   in vhost_virtqueue_start() function, below operation to check if the
>   desc addr set by guest kernel. This will ignore the extra vqs.
>     a = virtio_queue_get_desc_addr(vdev, idx);
>     if (a == 0) {
>         /* Queue might not be ready for start */
>         return 0;
>     }
>
>   If guest kernel add min(cpu,num_vqs), do we need add same check in
>   realize function of vhost-user-blk device?
>

No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
no check in cpu count, vhost-user-blk also doesn't.

You need to check first if the bug is in QEMU or the vhost-user-blk server.

Paolo
Re: [PATCH] virtio: add the queue number check
Posted by Yang Zhong 4 years, 2 months ago
On Fri, Jan 03, 2020 at 10:18:58PM +0100, Paolo Bonzini wrote:
> Il ven 3 gen 2020, 16:08 Yang Zhong <yang.zhong@intel.com> ha scritto:
> 
> >   I also tried virtio-blk device like below:
> >   https://patchwork.kernel.org/cover/10873193/
> >
> >   The virtio-blk can work with this changes, but vhost-user-blk device
> >   failed with this kernel patch.
> >
> >   in vhost_virtqueue_start() function, below operation to check if the
> >   desc addr set by guest kernel. This will ignore the extra vqs.
> >     a = virtio_queue_get_desc_addr(vdev, idx);
> >     if (a == 0) {
> >         /* Queue might not be ready for start */
> >         return 0;
> >     }
> >
> >   If guest kernel add min(cpu,num_vqs), do we need add same check in
> >   realize function of vhost-user-blk device?
> >
> 
> No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
> no check in cpu count, vhost-user-blk also doesn't.
> 
> You need to check first if the bug is in QEMU or the vhost-user-blk server.
> 
  Paolo and all,

  It seems i had root cause this issue, let me list what's i found. Any
  issue please correct me, thanks!

  I found there are 2 issues in DPDK and seabios by debugging this
  issue.

  (1). Seabios issue
  In init_virtio_blk() function, which set VIRTIO_CONFIG_S_DRIVER_OK
  status to qemu vhost-user-blk device.

  // the related code
  ......
  status |= VIRTIO_CONFIG_S_DRIVER_OK;
  vp_set_status(&vdrive->vp, status);
  ......

  I think there is no need for seabios to set VIRTIO_CONFIG_S_DRIVER_OK
  status to qemu vhost-user-blk device. In fact, this time, the guest
  virtio-blk module is not started. Once seabios set this DRIVER_OK
  status to qemu vhost user device, the vhost-user-blk will call
  vhost_user_blk_start().
  
  static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
  {
    VHostUserBlk *s = VHOST_USER_BLK(vdev);
    bool should_start = virtio_device_started(vdev, status);
    int ret;

    if (!vdev->vm_running) {
        should_start = false;
    }

    if (!s->connected) {
        return;
    }

    if (s->dev.started == should_start) {
        return;
    }

    if (should_start) {
        ret = vhost_user_blk_start(vdev);
        if (ret < 0) {
            error_report("vhost-user-blk: vhost start failed: %s",
                         strerror(-ret));
            qemu_chr_fe_disconnect(&s->chardev);
        }
    } else {
        vhost_user_blk_stop(vdev);
    }
    }

    In fact, this time vhost_user_blk_start almost do nothing because
    the real guest virtio-blk driver still not started yet. This time,
    there is only one vq can be used(this vq should be inited in seabios).

    When the guest virtio-blk driver really start and complet the
    probe(), the guest virtio-blk driver will set
    VIRTIO_CONFIG_S_DRIVER_OK to vhost-user-blk device again. This
    time, this driver will allocate RIGHT queue num according to
    MIN(vcpu, num_vqs).

    So, i think set VIRTIO_CONFIG_S_DRIVER_OK status should be removed from
    seabios, and guest driver should do this.

    I removed this status seting in the seabios, and test verified this.
    guest virtio-blk driver can be normally initialized by virtio-blk or
    vhost-user-blk device from qemu.

    (2). DPDK issue
     DPDK does not know the real queue number used by guest virtio-blk
     driver and it only know the queue number from vhost-user-blk
     commond line. Once the guest virtio-blk driver change the queue
     number according to MIN(vcpu, num_vqs), DPDK still use previous
     queue number and it think virtio is never ready by
     virtio_is_ready() function.

     There are two methods to fix this issue,

     one is add VHOST_USER_SET_QUEUE_NUM to vhost user protocol.
     vhost-user-blk device can call this to inform SPDK the real
     queue number. vhost-user-device can use below method to get
     the real queue number(to check if vring.desc is NON-NULL)
     for (i = 0; i < s->dev.nvqs; i++) {
        VirtQueue *kick_vq = virtio_get_queue(vdev, i);

        if (!virtio_queue_get_desc_addr(vdev, i)) {
            continue;
        }
     }
    
     Current vhost-user protocol only support GET_QUEUE_NUM(get max
     queue num), we can add SET_QUEUE_NUM.

     or DPDK can get the real queue number by checking if the vring.desc
     is NON-NULL.

     By the way, vhost SCSI device has the same issue with
     vhost-user-blk device. 

     Yang

> Paolo

Re: [PATCH] virtio: add the queue number check
Posted by Paolo Bonzini 4 years, 1 month ago
I have just found this email... sorry for the delay.

On 10/01/20 07:10, Yang Zhong wrote:
>> No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
>> no check in cpu count, vhost-user-blk also doesn't.
>>
>> You need to check first if the bug is in QEMU or the vhost-user-blk server.
>
>   (1). Seabios issue
>   In init_virtio_blk() function, which set VIRTIO_CONFIG_S_DRIVER_OK
>   status to qemu vhost-user-blk device.
> 
>   // the related code
>   ......
>   status |= VIRTIO_CONFIG_S_DRIVER_OK;
>   vp_set_status(&vdrive->vp, status);
>   ......
> 
>   I think there is no need for seabios to set VIRTIO_CONFIG_S_DRIVER_OK
>   status to qemu vhost-user-blk device.

It does so because it cannot know how it will be used.  It could be used
by the guest boot loader to load a kernel, for example.  SeaBIOS sets
DRIVER_OK because it has loaded a driver for the disk; that's exactly
what DRIVER_OK signals.


>     In fact, this time vhost_user_blk_start almost do nothing because
>     the real guest virtio-blk driver still not started yet. This time,
>     there is only one vq can be used(this vq should be inited in seabios).
> 
>     When the guest virtio-blk driver really start and complet the
>     probe(), the guest virtio-blk driver will set
>     VIRTIO_CONFIG_S_DRIVER_OK to vhost-user-blk device again. This
>     time, this driver will allocate RIGHT queue num according to
>     MIN(vcpu, num_vqs).

Doesn't it first reset the status to 0?

>     (2). DPDK issue
>      DPDK does not know the real queue number used by guest virtio-blk
>      driver and it only know the queue number from vhost-user-blk
>      commond line. Once the guest virtio-blk driver change the queue
>      number according to MIN(vcpu, num_vqs), DPDK still use previous
>      queue number and it think virtio is never ready by
>      virtio_is_ready() function.

What is virtio_is_ready()?  The virtio device should not wait for all
the queues to be set.  A device is ready when it sets DRIVER_OK, and
that's it.

>      or DPDK can get the real queue number by checking if the vring.desc
>      is NON-NULL.

Note that there is no requirement that the driver initializes a
consecutive number of virtqueues.  It is acceptable for it to initialize
virtqueues 0, 1 and 57.  It seems like the bug is in DPDK, possibly more
than one...

Paolo

>      By the way, vhost SCSI device has the same issue with
>      vhost-user-blk device. 
> 
>      Yang
> 
>> Paolo
> 


Re: [PATCH] virtio: add the queue number check
Posted by Michael S. Tsirkin 4 years, 1 month ago
On Fri, Jan 31, 2020 at 05:22:47PM +0100, Paolo Bonzini wrote:
> I have just found this email... sorry for the delay.
> 
> On 10/01/20 07:10, Yang Zhong wrote:
> >> No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
> >> no check in cpu count, vhost-user-blk also doesn't.
> >>
> >> You need to check first if the bug is in QEMU or the vhost-user-blk server.
> >
> >   (1). Seabios issue
> >   In init_virtio_blk() function, which set VIRTIO_CONFIG_S_DRIVER_OK
> >   status to qemu vhost-user-blk device.
> > 
> >   // the related code
> >   ......
> >   status |= VIRTIO_CONFIG_S_DRIVER_OK;
> >   vp_set_status(&vdrive->vp, status);
> >   ......
> > 
> >   I think there is no need for seabios to set VIRTIO_CONFIG_S_DRIVER_OK
> >   status to qemu vhost-user-blk device.
> 
> It does so because it cannot know how it will be used.  It could be used
> by the guest boot loader to load a kernel, for example.  SeaBIOS sets
> DRIVER_OK because it has loaded a driver for the disk; that's exactly
> what DRIVER_OK signals.

Right. More specifically DRIVER_OK means driver finished setup and is
going to add buffers and process used ones, so device should start
looking at queues.

> 
> >     In fact, this time vhost_user_blk_start almost do nothing because
> >     the real guest virtio-blk driver still not started yet. This time,
> >     there is only one vq can be used(this vq should be inited in seabios).
> > 
> >     When the guest virtio-blk driver really start and complet the
> >     probe(), the guest virtio-blk driver will set
> >     VIRTIO_CONFIG_S_DRIVER_OK to vhost-user-blk device again. This
> >     time, this driver will allocate RIGHT queue num according to
> >     MIN(vcpu, num_vqs).
> 
> Doesn't it first reset the status to 0?
> 
> >     (2). DPDK issue
> >      DPDK does not know the real queue number used by guest virtio-blk
> >      driver and it only know the queue number from vhost-user-blk
> >      commond line. Once the guest virtio-blk driver change the queue
> >      number according to MIN(vcpu, num_vqs), DPDK still use previous
> >      queue number and it think virtio is never ready by
> >      virtio_is_ready() function.
> 
> What is virtio_is_ready()?  The virtio device should not wait for all
> the queues to be set.  A device is ready when it sets DRIVER_OK, and
> that's it.

Or - if we want to support legacy guests, and due to a bunch of legacy
guest bugs - if a legacy guest kicked a queue before setting DRIVER_OK.

> >      or DPDK can get the real queue number by checking if the vring.desc
> >      is NON-NULL.
> 
> Note that there is no requirement that the driver initializes a
> consecutive number of virtqueues.  It is acceptable for it to initialize
> virtqueues 0, 1 and 57.  It seems like the bug is in DPDK, possibly more
> than one...
> 
> Paolo
> 
> >      By the way, vhost SCSI device has the same issue with
> >      vhost-user-blk device. 
> > 
> >      Yang
> > 
> >> Paolo
> > 


Re: [PATCH] virtio: add the queue number check
Posted by Yang Zhong 4 years, 1 month ago
On Fri, Jan 31, 2020 at 05:22:47PM +0100, Paolo Bonzini wrote:
> I have just found this email... sorry for the delay.
> 
> On 10/01/20 07:10, Yang Zhong wrote:
> >> No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
> >> no check in cpu count, vhost-user-blk also doesn't.
> >>
> >> You need to check first if the bug is in QEMU or the vhost-user-blk server.
> >
> >   (1). Seabios issue
> >   In init_virtio_blk() function, which set VIRTIO_CONFIG_S_DRIVER_OK
> >   status to qemu vhost-user-blk device.
> > 
> >   // the related code
> >   ......
> >   status |= VIRTIO_CONFIG_S_DRIVER_OK;
> >   vp_set_status(&vdrive->vp, status);
> >   ......
> > 
> >   I think there is no need for seabios to set VIRTIO_CONFIG_S_DRIVER_OK
> >   status to qemu vhost-user-blk device.
> 
> It does so because it cannot know how it will be used.  It could be used
> by the guest boot loader to load a kernel, for example.  SeaBIOS sets
> DRIVER_OK because it has loaded a driver for the disk; that's exactly
> what DRIVER_OK signals.
> 
  Yes, the virtio_blk driver in seabios is mainly for virtio format
  images. I did not consider this. Thanks Paolo!
  
  Yang
> 
> >     In fact, this time vhost_user_blk_start almost do nothing because
> >     the real guest virtio-blk driver still not started yet. This time,
> >     there is only one vq can be used(this vq should be inited in seabios).
> > 
> >     When the guest virtio-blk driver really start and complet the
> >     probe(), the guest virtio-blk driver will set
> >     VIRTIO_CONFIG_S_DRIVER_OK to vhost-user-blk device again. This
> >     time, this driver will allocate RIGHT queue num according to
> >     MIN(vcpu, num_vqs).
> 
> Doesn't it first reset the status to 0?
> 
  I checked the virtio_blk drivers in seabios and guest kernel, all do
  the reset before driver probe(). thanks!

  Yang

> >     (2). DPDK issue
> >      DPDK does not know the real queue number used by guest virtio-blk
> >      driver and it only know the queue number from vhost-user-blk
> >      commond line. Once the guest virtio-blk driver change the queue
> >      number according to MIN(vcpu, num_vqs), DPDK still use previous
> >      queue number and it think virtio is never ready by
> >      virtio_is_ready() function.
> 
> What is virtio_is_ready()?  The virtio device should not wait for all
> the queues to be set.  A device is ready when it sets DRIVER_OK, and
> that's it.
> 
  In spdk/dpdk/lib/librte_vhost/vhost_user.c
  static int
  virtio_is_ready(struct virtio_net *dev)
  {
      struct vhost_virtqueue *vq;
      uint32_t i;

      if (dev->nr_vring == 0)
          return 0;

      for (i = 0; i < dev->nr_vring; i++) {
          vq = dev->virtqueue[i];

          if (!vq_is_ready(dev, vq))
              return 0;
      }

      RTE_LOG(INFO, VHOST_CONFIG,
          "virtio is now ready for processing.\n");
      return 1;
  }

  static bool
  vq_is_ready(struct virtio_net *dev, struct vhost_virtqueue *vq)
  {
      bool rings_ok;

      if (!vq)
          return false;

      if (vq_is_packed(dev))
          rings_ok = !!vq->desc_packed;
      else
          rings_ok = vq->desc && vq->avail && vq->used;

      return rings_ok &&
             vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
             vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
  }

  The dev->nr_vring is equal to the queue number set in qemu cmdline, 
  which maybe different with guest virtio_blk driver by MIN(vcpu, num_vqs).
  If like this scenario, the virtio is NEVER ready from
  virtio_is_ready() because DPDK still check the left
  queues(i.e num_vqs-vcpu), which are not initialized by the guest
  driver.
  
  Yang
  
> >      or DPDK can get the real queue number by checking if the vring.desc
> >      is NON-NULL.
> 
> Note that there is no requirement that the driver initializes a
> consecutive number of virtqueues.  It is acceptable for it to initialize
> virtqueues 0, 1 and 57.  It seems like the bug is in DPDK, possibly more
> than one...
> 
  Yes, it seems the bug is in DPDK side. But one strange issue here, the
  virtio_net driver do the same thing to get real num_vqs by MIN(..),
  the DPDK with virtio_net can avoid this kind of issue(Sorry, i did not
  fully dig out the virtio_net and DPDK code and only hope to get the
  best solution to fix this issue). Anyway, i will firstly check this from 
  DPDK side. Thanks for your great help!

  Yang

> Paolo
> 
> >      By the way, vhost SCSI device has the same issue with
> >      vhost-user-blk device. 
> > 
> >      Yang
> > 
> >> Paolo
> > 

Re: [PATCH] virtio: add the queue number check
Posted by Yang Zhong 4 years, 2 months ago
On Fri, Jan 03, 2020 at 10:18:58PM +0100, Paolo Bonzini wrote:
> Il ven 3 gen 2020, 16:08 Yang Zhong <yang.zhong@intel.com> ha scritto:
> 
> >   I also tried virtio-blk device like below:
> >   https://patchwork.kernel.org/cover/10873193/
> >
> >   The virtio-blk can work with this changes, but vhost-user-blk device
> >   failed with this kernel patch.
> >
> >   in vhost_virtqueue_start() function, below operation to check if the
> >   desc addr set by guest kernel. This will ignore the extra vqs.
> >     a = virtio_queue_get_desc_addr(vdev, idx);
> >     if (a == 0) {
> >         /* Queue might not be ready for start */
> >         return 0;
> >     }
> >
> >   If guest kernel add min(cpu,num_vqs), do we need add same check in
> >   realize function of vhost-user-blk device?
> >
> 
> No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
> no check in cpu count, vhost-user-blk also doesn't.
> 
> You need to check first if the bug is in QEMU or the vhost-user-blk server.
>
  Thanks Paolo for your comments, and i will do it later. thanks!

  Yang

> Paolo