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

[Paolo Bonzini]

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