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

[Yang Zhong <yang.zhong@intel.com>]

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