Re: [PATCH] virtio: add the queue number check

[Yang Zhong]

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 <address@hidden>
> > ---
> >  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;
> >