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

[Paolo Bonzini]

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