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.zh...@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;
>>>>>
>>>
>
