From 38b440964736a5fca2c95606ce3efaa35381fa9d Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 4 May 2021 19:06:56 +0300 Subject: [PATCH 01/13] qcow2: set bdi->is_dirty Set bdi->is_dirty, so that qemu-img info could show dirty flag. After this commit the following check will show '"dirty-flag": true': ./build/qemu-img create -f qcow2 -o lazy_refcounts=on x 1M ./build/qemu-io x qemu-io> write 0 1M After "write" command success, kill the qemu-io process: kill -9 ./build/qemu-img info --output=json x This will show '"dirty-flag": true' among other things. (before this commit it shows '"dirty-flag": false') Note, that qcow2's dirty-bit is not a "dirty bit for the image". It only protects qcow2 lazy refcounts feature. So, there are a lot of conditions when qcow2 session may be not closed correctly, but bit is 0. Still, when bit is set, the last session is definitely not finished correctly and it's better to report it. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210504160656.462836-1-vsementsov@virtuozzo.com> Tested-by: Kirill Tkhai Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qcow2.c b/block/qcow2.c index 9727ae8fe3..39b91ef940 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5089,6 +5089,7 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) BDRVQcow2State *s = bs->opaque; bdi->cluster_size = s->cluster_size; bdi->vm_state_offset = qcow2_vm_state_offset(s); + bdi->is_dirty = s->incompatible_features & QCOW2_INCOMPAT_DIRTY; return 0; } From e878bb1293d2cd0082550b320c3ccf245d0a69d4 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 3 May 2021 13:05:54 +0200 Subject: [PATCH 02/13] block: Fix Transaction leak in bdrv_root_attach_child() The error path needs to call tran_finalize(), too. Fixes: CID 1452773 Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 Signed-off-by: Kevin Wolf Message-Id: <20210503110555.24001-2-kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 75a82af641..6dbe9af2cd 100644 --- a/block.c +++ b/block.c @@ -2916,13 +2916,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, child_role, perm, shared_perm, opaque, &child, tran, errp); if (ret < 0) { - bdrv_unref(child_bs); - return NULL; + assert(child == NULL); + goto out; } ret = bdrv_refresh_perms(child_bs, errp); - tran_finalize(tran, ret); +out: + tran_finalize(tran, ret); bdrv_unref(child_bs); return child; } From e3fc91aaaacbdc2a88bbb4109c8a1cded628e36f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 3 May 2021 13:05:55 +0200 Subject: [PATCH 03/13] block: Fix Transaction leak in bdrv_reopen_multiple() Like other error paths, this one needs to call tran_finalize() and clean up the BlockReopenQueue, too. Fixes: CID 1452772 Fixes: 72373e40fbc7e4218061a8211384db362d3e7348 Signed-off-by: Kevin Wolf Message-Id: <20210503110555.24001-3-kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block.c b/block.c index 6dbe9af2cd..0dc97281dc 100644 --- a/block.c +++ b/block.c @@ -4050,7 +4050,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) ret = bdrv_flush(bs_entry->state.bs); if (ret < 0) { error_setg_errno(errp, -ret, "Error flushing drive"); - goto cleanup; + goto abort; } } From 1b0b2e6d06c26185ecc820e868e025c894f979f7 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 31 Mar 2021 15:27:27 +0100 Subject: [PATCH 04/13] block/export: improve vu_blk_sect_range_ok() The checks in vu_blk_sect_range_ok() assume VIRTIO_BLK_SECTOR_SIZE is equal to BDRV_SECTOR_SIZE. This is true, but let's add a QEMU_BUILD_BUG_ON() to make it explicit. We might as well check that the request buffer size is a multiple of VIRTIO_BLK_SECTOR_SIZE while we're at it. Suggested-by: Max Reitz Signed-off-by: Stefan Hajnoczi Message-Id: <20210331142727.391465-1-stefanha@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/export/vhost-user-blk-server.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index fa06996d37..1862563336 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -70,9 +70,16 @@ static void vu_blk_req_complete(VuBlkReq *req) static bool vu_blk_sect_range_ok(VuBlkExport *vexp, uint64_t sector, size_t size) { - uint64_t nb_sectors = size >> BDRV_SECTOR_BITS; + uint64_t nb_sectors; uint64_t total_sectors; + if (size % VIRTIO_BLK_SECTOR_SIZE) { + return false; + } + + nb_sectors = size >> VIRTIO_BLK_SECTOR_BITS; + + QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != VIRTIO_BLK_SECTOR_SIZE); if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) { return false; } From 806952026df41939680abe92b329715b9b4e01cc Mon Sep 17 00:00:00 2001 From: Coiby Xu Date: Mon, 22 Mar 2021 09:23:25 +0000 Subject: [PATCH 05/13] test: new qTest case to test the vhost-user-blk-server This test case has the same tests as tests/virtio-blk-test.c except for tests have block_resize. Since the vhost-user-blk export only serves one client one time, two exports are started by qemu-storage-daemon for the hotplug test. Suggested-by: Thomas Huth Signed-off-by: Coiby Xu Signed-off-by: Stefan Hajnoczi Message-Id: <20210309094106.196911-3-stefanha@redhat.com> Signed-off-by: Kevin Wolf Message-Id: <20210322092327.150720-2-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- MAINTAINERS | 2 + tests/qtest/libqos/meson.build | 1 + tests/qtest/libqos/vhost-user-blk.c | 130 +++++ tests/qtest/libqos/vhost-user-blk.h | 48 ++ tests/qtest/meson.build | 4 + tests/qtest/vhost-user-blk-test.c | 794 ++++++++++++++++++++++++++++ 6 files changed, 979 insertions(+) create mode 100644 tests/qtest/libqos/vhost-user-blk.c create mode 100644 tests/qtest/libqos/vhost-user-blk.h create mode 100644 tests/qtest/vhost-user-blk-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 78561a223f..15028172b9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3244,6 +3244,8 @@ F: block/export/vhost-user-blk-server.c F: block/export/vhost-user-blk-server.h F: include/qemu/vhost-user-server.h F: tests/qtest/libqos/vhost-user-blk.c +F: tests/qtest/libqos/vhost-user-blk.h +F: tests/qtest/vhost-user-blk-test.c F: util/vhost-user-server.c FUSE block device exports diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build index 1cddf5bdaa..1f5c8f1053 100644 --- a/tests/qtest/libqos/meson.build +++ b/tests/qtest/libqos/meson.build @@ -32,6 +32,7 @@ libqos_srcs = files('../libqtest.c', 'virtio-9p.c', 'virtio-balloon.c', 'virtio-blk.c', + 'vhost-user-blk.c', 'virtio-mmio.c', 'virtio-net.c', 'virtio-pci.c', diff --git a/tests/qtest/libqos/vhost-user-blk.c b/tests/qtest/libqos/vhost-user-blk.c new file mode 100644 index 0000000000..568c3426ed --- /dev/null +++ b/tests/qtest/libqos/vhost-user-blk.c @@ -0,0 +1,130 @@ +/* + * libqos driver framework + * + * Based on tests/qtest/libqos/virtio-blk.c + * + * Copyright (c) 2020 Coiby Xu + * + * Copyright (c) 2018 Emanuele Giuseppe Esposito + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License version 2.1 as published by the Free Software Foundation. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see + */ + +#include "qemu/osdep.h" +#include "libqtest.h" +#include "qemu/module.h" +#include "standard-headers/linux/virtio_blk.h" +#include "vhost-user-blk.h" + +#define PCI_SLOT 0x04 +#define PCI_FN 0x00 + +/* virtio-blk-device */ +static void *qvhost_user_blk_get_driver(QVhostUserBlk *v_blk, + const char *interface) +{ + if (!g_strcmp0(interface, "vhost-user-blk")) { + return v_blk; + } + if (!g_strcmp0(interface, "virtio")) { + return v_blk->vdev; + } + + fprintf(stderr, "%s not present in vhost-user-blk-device\n", interface); + g_assert_not_reached(); +} + +static void *qvhost_user_blk_device_get_driver(void *object, + const char *interface) +{ + QVhostUserBlkDevice *v_blk = object; + return qvhost_user_blk_get_driver(&v_blk->blk, interface); +} + +static void *vhost_user_blk_device_create(void *virtio_dev, + QGuestAllocator *t_alloc, + void *addr) +{ + QVhostUserBlkDevice *vhost_user_blk = g_new0(QVhostUserBlkDevice, 1); + QVhostUserBlk *interface = &vhost_user_blk->blk; + + interface->vdev = virtio_dev; + + vhost_user_blk->obj.get_driver = qvhost_user_blk_device_get_driver; + + return &vhost_user_blk->obj; +} + +/* virtio-blk-pci */ +static void *qvhost_user_blk_pci_get_driver(void *object, const char *interface) +{ + QVhostUserBlkPCI *v_blk = object; + if (!g_strcmp0(interface, "pci-device")) { + return v_blk->pci_vdev.pdev; + } + return qvhost_user_blk_get_driver(&v_blk->blk, interface); +} + +static void *vhost_user_blk_pci_create(void *pci_bus, QGuestAllocator *t_alloc, + void *addr) +{ + QVhostUserBlkPCI *vhost_user_blk = g_new0(QVhostUserBlkPCI, 1); + QVhostUserBlk *interface = &vhost_user_blk->blk; + QOSGraphObject *obj = &vhost_user_blk->pci_vdev.obj; + + virtio_pci_init(&vhost_user_blk->pci_vdev, pci_bus, addr); + interface->vdev = &vhost_user_blk->pci_vdev.vdev; + + g_assert_cmphex(interface->vdev->device_type, ==, VIRTIO_ID_BLOCK); + + obj->get_driver = qvhost_user_blk_pci_get_driver; + + return obj; +} + +static void vhost_user_blk_register_nodes(void) +{ + /* + * FIXME: every test using these two nodes needs to setup a + * -drive,id=drive0 otherwise QEMU is not going to start. + * Therefore, we do not include "produces" edge for virtio + * and pci-device yet. + */ + + char *arg = g_strdup_printf("id=drv0,chardev=char1,addr=%x.%x", + PCI_SLOT, PCI_FN); + + QPCIAddress addr = { + .devfn = QPCI_DEVFN(PCI_SLOT, PCI_FN), + }; + + QOSGraphEdgeOptions opts = { }; + + /* virtio-blk-device */ + /** opts.extra_device_opts = "drive=drive0"; */ + qos_node_create_driver("vhost-user-blk-device", + vhost_user_blk_device_create); + qos_node_consumes("vhost-user-blk-device", "virtio-bus", &opts); + qos_node_produces("vhost-user-blk-device", "vhost-user-blk"); + + /* virtio-blk-pci */ + opts.extra_device_opts = arg; + add_qpci_address(&opts, &addr); + qos_node_create_driver("vhost-user-blk-pci", vhost_user_blk_pci_create); + qos_node_consumes("vhost-user-blk-pci", "pci-bus", &opts); + qos_node_produces("vhost-user-blk-pci", "vhost-user-blk"); + + g_free(arg); +} + +libqos_init(vhost_user_blk_register_nodes); diff --git a/tests/qtest/libqos/vhost-user-blk.h b/tests/qtest/libqos/vhost-user-blk.h new file mode 100644 index 0000000000..2a03456a45 --- /dev/null +++ b/tests/qtest/libqos/vhost-user-blk.h @@ -0,0 +1,48 @@ +/* + * libqos driver framework + * + * Based on tests/qtest/libqos/virtio-blk.c + * + * Copyright (c) 2020 Coiby Xu + * + * Copyright (c) 2018 Emanuele Giuseppe Esposito + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License version 2 as published by the Free Software Foundation. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see + */ + +#ifndef TESTS_LIBQOS_VHOST_USER_BLK_H +#define TESTS_LIBQOS_VHOST_USER_BLK_H + +#include "qgraph.h" +#include "virtio.h" +#include "virtio-pci.h" + +typedef struct QVhostUserBlk QVhostUserBlk; +typedef struct QVhostUserBlkPCI QVhostUserBlkPCI; +typedef struct QVhostUserBlkDevice QVhostUserBlkDevice; + +struct QVhostUserBlk { + QVirtioDevice *vdev; +}; + +struct QVhostUserBlkPCI { + QVirtioPCIDevice pci_vdev; + QVhostUserBlk blk; +}; + +struct QVhostUserBlkDevice { + QOSGraphObject obj; + QVhostUserBlk blk; +}; + +#endif diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 49de74ff59..c3a223a83d 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -231,6 +231,9 @@ if have_virtfs qos_test_ss.add(files('virtio-9p-test.c')) endif qos_test_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-test.c')) +if have_tools and have_vhost_user_blk_server + qos_test_ss.add(files('vhost-user-blk-test.c')) +endif tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] @@ -269,6 +272,7 @@ foreach dir : target_dirs endif qtest_env.set('G_TEST_DBUS_DAEMON', meson.source_root() / 'tests/dbus-vmstate-daemon.sh') qtest_env.set('QTEST_QEMU_BINARY', './qemu-system-' + target_base) + qtest_env.set('QTEST_QEMU_STORAGE_DAEMON_BINARY', './storage-daemon/qemu-storage-daemon') foreach test : target_qtests # Executables are shared across targets, declare them only the first time we diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c new file mode 100644 index 0000000000..3e79549899 --- /dev/null +++ b/tests/qtest/vhost-user-blk-test.c @@ -0,0 +1,794 @@ +/* + * QTest testcase for Vhost-user Block Device + * + * Based on tests/qtest//virtio-blk-test.c + + * Copyright (c) 2014 SUSE LINUX Products GmbH + * Copyright (c) 2014 Marc Marí + * Copyright (c) 2020 Coiby Xu + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "libqtest-single.h" +#include "qemu/bswap.h" +#include "qemu/module.h" +#include "standard-headers/linux/virtio_blk.h" +#include "standard-headers/linux/virtio_pci.h" +#include "libqos/qgraph.h" +#include "libqos/vhost-user-blk.h" +#include "libqos/libqos-pc.h" + +#define TEST_IMAGE_SIZE (64 * 1024 * 1024) +#define QVIRTIO_BLK_TIMEOUT_US (30 * 1000 * 1000) +#define PCI_SLOT_HP 0x06 + +typedef struct { + pid_t pid; +} QemuStorageDaemonState; + +typedef struct QVirtioBlkReq { + uint32_t type; + uint32_t ioprio; + uint64_t sector; + char *data; + uint8_t status; +} QVirtioBlkReq; + +#ifdef HOST_WORDS_BIGENDIAN +static const bool host_is_big_endian = true; +#else +static const bool host_is_big_endian; /* false */ +#endif + +static inline void virtio_blk_fix_request(QVirtioDevice *d, QVirtioBlkReq *req) +{ + if (qvirtio_is_big_endian(d) != host_is_big_endian) { + req->type = bswap32(req->type); + req->ioprio = bswap32(req->ioprio); + req->sector = bswap64(req->sector); + } +} + +static inline void virtio_blk_fix_dwz_hdr(QVirtioDevice *d, + struct virtio_blk_discard_write_zeroes *dwz_hdr) +{ + if (qvirtio_is_big_endian(d) != host_is_big_endian) { + dwz_hdr->sector = bswap64(dwz_hdr->sector); + dwz_hdr->num_sectors = bswap32(dwz_hdr->num_sectors); + dwz_hdr->flags = bswap32(dwz_hdr->flags); + } +} + +static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d, + QVirtioBlkReq *req, uint64_t data_size) +{ + uint64_t addr; + uint8_t status = 0xFF; + QTestState *qts = global_qtest; + + switch (req->type) { + case VIRTIO_BLK_T_IN: + case VIRTIO_BLK_T_OUT: + g_assert_cmpuint(data_size % 512, ==, 0); + break; + case VIRTIO_BLK_T_DISCARD: + case VIRTIO_BLK_T_WRITE_ZEROES: + g_assert_cmpuint(data_size % + sizeof(struct virtio_blk_discard_write_zeroes), ==, 0); + break; + default: + g_assert_cmpuint(data_size, ==, 0); + } + + addr = guest_alloc(alloc, sizeof(*req) + data_size); + + virtio_blk_fix_request(d, req); + + qtest_memwrite(qts, addr, req, 16); + qtest_memwrite(qts, addr + 16, req->data, data_size); + qtest_memwrite(qts, addr + 16 + data_size, &status, sizeof(status)); + + return addr; +} + +/* Returns the request virtqueue so the caller can perform further tests */ +static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc) +{ + QVirtioBlkReq req; + uint64_t req_addr; + uint64_t capacity; + uint64_t features; + uint32_t free_head; + uint8_t status; + char *data; + QTestState *qts = global_qtest; + QVirtQueue *vq; + + features = qvirtio_get_features(dev); + features = features & ~(QVIRTIO_F_BAD_FEATURE | + (1u << VIRTIO_RING_F_INDIRECT_DESC) | + (1u << VIRTIO_RING_F_EVENT_IDX) | + (1u << VIRTIO_BLK_F_SCSI)); + qvirtio_set_features(dev, features); + + capacity = qvirtio_config_readq(dev, 0); + g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); + + vq = qvirtqueue_setup(dev, alloc, 0); + + qvirtio_set_driver_ok(dev); + + /* Write and read with 3 descriptor layout */ + /* Write request */ + req.type = VIRTIO_BLK_T_OUT; + req.ioprio = 1; + req.sector = 0; + req.data = g_malloc0(512); + strcpy(req.data, "TEST"); + + req_addr = virtio_blk_request(alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 512, false, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + guest_free(alloc, req_addr); + + /* Read request */ + req.type = VIRTIO_BLK_T_IN; + req.ioprio = 1; + req.sector = 0; + req.data = g_malloc0(512); + + req_addr = virtio_blk_request(alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 512, true, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + data = g_malloc0(512); + qtest_memread(qts, req_addr + 16, data, 512); + g_assert_cmpstr(data, ==, "TEST"); + g_free(data); + + guest_free(alloc, req_addr); + + if (features & (1u << VIRTIO_BLK_F_WRITE_ZEROES)) { + struct virtio_blk_discard_write_zeroes dwz_hdr; + void *expected; + + /* + * WRITE_ZEROES request on the same sector of previous test where + * we wrote "TEST". + */ + req.type = VIRTIO_BLK_T_WRITE_ZEROES; + req.data = (char *) &dwz_hdr; + dwz_hdr.sector = 0; + dwz_hdr.num_sectors = 1; + dwz_hdr.flags = 0; + + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr); + + req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr)); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true); + qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, + false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 16 + sizeof(dwz_hdr)); + g_assert_cmpint(status, ==, 0); + + guest_free(alloc, req_addr); + + /* Read request to check if the sector contains all zeroes */ + req.type = VIRTIO_BLK_T_IN; + req.ioprio = 1; + req.sector = 0; + req.data = g_malloc0(512); + + req_addr = virtio_blk_request(alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 512, true, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + data = g_malloc(512); + expected = g_malloc0(512); + qtest_memread(qts, req_addr + 16, data, 512); + g_assert_cmpmem(data, 512, expected, 512); + g_free(expected); + g_free(data); + + guest_free(alloc, req_addr); + } + + if (features & (1u << VIRTIO_BLK_F_DISCARD)) { + struct virtio_blk_discard_write_zeroes dwz_hdr; + + req.type = VIRTIO_BLK_T_DISCARD; + req.data = (char *) &dwz_hdr; + dwz_hdr.sector = 0; + dwz_hdr.num_sectors = 1; + dwz_hdr.flags = 0; + + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr); + + req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr)); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true); + qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), + 1, true, false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 16 + sizeof(dwz_hdr)); + g_assert_cmpint(status, ==, 0); + + guest_free(alloc, req_addr); + } + + if (features & (1u << VIRTIO_F_ANY_LAYOUT)) { + /* Write and read with 2 descriptor layout */ + /* Write request */ + req.type = VIRTIO_BLK_T_OUT; + req.ioprio = 1; + req.sector = 1; + req.data = g_malloc0(512); + strcpy(req.data, "TEST"); + + req_addr = virtio_blk_request(alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 528, false, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + guest_free(alloc, req_addr); + + /* Read request */ + req.type = VIRTIO_BLK_T_IN; + req.ioprio = 1; + req.sector = 1; + req.data = g_malloc0(512); + + req_addr = virtio_blk_request(alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 513, true, false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + data = g_malloc0(512); + qtest_memread(qts, req_addr + 16, data, 512); + g_assert_cmpstr(data, ==, "TEST"); + g_free(data); + + guest_free(alloc, req_addr); + } + + return vq; +} + +static void basic(void *obj, void *data, QGuestAllocator *t_alloc) +{ + QVhostUserBlk *blk_if = obj; + QVirtQueue *vq; + + vq = test_basic(blk_if->vdev, t_alloc); + qvirtqueue_cleanup(blk_if->vdev->bus, vq, t_alloc); + +} + +static void indirect(void *obj, void *u_data, QGuestAllocator *t_alloc) +{ + QVirtQueue *vq; + QVhostUserBlk *blk_if = obj; + QVirtioDevice *dev = blk_if->vdev; + QVirtioBlkReq req; + QVRingIndirectDesc *indirect; + uint64_t req_addr; + uint64_t capacity; + uint64_t features; + uint32_t free_head; + uint8_t status; + char *data; + QTestState *qts = global_qtest; + + features = qvirtio_get_features(dev); + g_assert_cmphex(features & (1u << VIRTIO_RING_F_INDIRECT_DESC), !=, 0); + features = features & ~(QVIRTIO_F_BAD_FEATURE | + (1u << VIRTIO_RING_F_EVENT_IDX) | + (1u << VIRTIO_BLK_F_SCSI)); + qvirtio_set_features(dev, features); + + capacity = qvirtio_config_readq(dev, 0); + g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); + + vq = qvirtqueue_setup(dev, t_alloc, 0); + qvirtio_set_driver_ok(dev); + + /* Write request */ + req.type = VIRTIO_BLK_T_OUT; + req.ioprio = 1; + req.sector = 0; + req.data = g_malloc0(512); + strcpy(req.data, "TEST"); + + req_addr = virtio_blk_request(t_alloc, dev, &req, 512); + + g_free(req.data); + + indirect = qvring_indirect_desc_setup(qts, dev, t_alloc, 2); + qvring_indirect_desc_add(dev, qts, indirect, req_addr, 528, false); + qvring_indirect_desc_add(dev, qts, indirect, req_addr + 528, 1, true); + free_head = qvirtqueue_add_indirect(qts, vq, indirect); + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + g_free(indirect); + guest_free(t_alloc, req_addr); + + /* Read request */ + req.type = VIRTIO_BLK_T_IN; + req.ioprio = 1; + req.sector = 0; + req.data = g_malloc0(512); + strcpy(req.data, "TEST"); + + req_addr = virtio_blk_request(t_alloc, dev, &req, 512); + + g_free(req.data); + + indirect = qvring_indirect_desc_setup(qts, dev, t_alloc, 2); + qvring_indirect_desc_add(dev, qts, indirect, req_addr, 16, false); + qvring_indirect_desc_add(dev, qts, indirect, req_addr + 16, 513, true); + free_head = qvirtqueue_add_indirect(qts, vq, indirect); + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + data = g_malloc0(512); + qtest_memread(qts, req_addr + 16, data, 512); + g_assert_cmpstr(data, ==, "TEST"); + g_free(data); + + g_free(indirect); + guest_free(t_alloc, req_addr); + qvirtqueue_cleanup(dev->bus, vq, t_alloc); +} + +static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc) +{ + QVirtQueue *vq; + QVhostUserBlkPCI *blk = obj; + QVirtioPCIDevice *pdev = &blk->pci_vdev; + QVirtioDevice *dev = &pdev->vdev; + QVirtioBlkReq req; + uint64_t req_addr; + uint64_t capacity; + uint64_t features; + uint32_t free_head; + uint32_t write_head; + uint32_t desc_idx; + uint8_t status; + char *data; + QOSGraphObject *blk_object = obj; + QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device"); + QTestState *qts = global_qtest; + + if (qpci_check_buggy_msi(pci_dev)) { + return; + } + + qpci_msix_enable(pdev->pdev); + qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0); + + features = qvirtio_get_features(dev); + features = features & ~(QVIRTIO_F_BAD_FEATURE | + (1u << VIRTIO_RING_F_INDIRECT_DESC) | + (1u << VIRTIO_F_NOTIFY_ON_EMPTY) | + (1u << VIRTIO_BLK_F_SCSI)); + qvirtio_set_features(dev, features); + + capacity = qvirtio_config_readq(dev, 0); + g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); + + vq = qvirtqueue_setup(dev, t_alloc, 0); + qvirtqueue_pci_msix_setup(pdev, (QVirtQueuePCI *)vq, t_alloc, 1); + + qvirtio_set_driver_ok(dev); + + /* + * libvhost-user signals the call fd in VHOST_USER_SET_VRING_CALL, make + * sure to wait for the isr here so we don't race and confuse it later on. + */ + qvirtio_wait_queue_isr(qts, dev, vq, QVIRTIO_BLK_TIMEOUT_US); + + /* Write request */ + req.type = VIRTIO_BLK_T_OUT; + req.ioprio = 1; + req.sector = 0; + req.data = g_malloc0(512); + strcpy(req.data, "TEST"); + + req_addr = virtio_blk_request(t_alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 512, false, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + + /* Write request */ + req.type = VIRTIO_BLK_T_OUT; + req.ioprio = 1; + req.sector = 1; + req.data = g_malloc0(512); + strcpy(req.data, "TEST"); + + req_addr = virtio_blk_request(t_alloc, dev, &req, 512); + + g_free(req.data); + + /* Notify after processing the third request */ + qvirtqueue_set_used_event(qts, vq, 2); + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 512, false, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + qvirtqueue_kick(qts, dev, vq, free_head); + write_head = free_head; + + /* No notification expected */ + status = qvirtio_wait_status_byte_no_isr(qts, dev, + vq, req_addr + 528, + QVIRTIO_BLK_TIMEOUT_US); + g_assert_cmpint(status, ==, 0); + + guest_free(t_alloc, req_addr); + + /* Read request */ + req.type = VIRTIO_BLK_T_IN; + req.ioprio = 1; + req.sector = 1; + req.data = g_malloc0(512); + + req_addr = virtio_blk_request(t_alloc, dev, &req, 512); + + g_free(req.data); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, 512, true, true); + qvirtqueue_add(qts, vq, req_addr + 528, 1, true, false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + /* We get just one notification for both requests */ + qvirtio_wait_used_elem(qts, dev, vq, write_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + g_assert(qvirtqueue_get_buf(qts, vq, &desc_idx, NULL)); + g_assert_cmpint(desc_idx, ==, free_head); + + status = readb(req_addr + 528); + g_assert_cmpint(status, ==, 0); + + data = g_malloc0(512); + qtest_memread(qts, req_addr + 16, data, 512); + g_assert_cmpstr(data, ==, "TEST"); + g_free(data); + + guest_free(t_alloc, req_addr); + + /* End test */ + qpci_msix_disable(pdev->pdev); + + qvirtqueue_cleanup(dev->bus, vq, t_alloc); +} + +static void pci_hotplug(void *obj, void *data, QGuestAllocator *t_alloc) +{ + QVirtioPCIDevice *dev1 = obj; + QVirtioPCIDevice *dev; + QTestState *qts = dev1->pdev->bus->qts; + + /* plug secondary disk */ + qtest_qmp_device_add(qts, "vhost-user-blk-pci", "drv1", + "{'addr': %s, 'chardev': 'char2'}", + stringify(PCI_SLOT_HP) ".0"); + + dev = virtio_pci_new(dev1->pdev->bus, + &(QPCIAddress) { .devfn = QPCI_DEVFN(PCI_SLOT_HP, 0) + }); + g_assert_nonnull(dev); + g_assert_cmpint(dev->vdev.device_type, ==, VIRTIO_ID_BLOCK); + qvirtio_pci_device_disable(dev); + qos_object_destroy((QOSGraphObject *)dev); + + /* unplug secondary disk */ + qpci_unplug_acpi_device_test(qts, "drv1", PCI_SLOT_HP); +} + +/* + * Check that setting the vring addr on a non-existent virtqueue does + * not crash. + */ +static void test_nonexistent_virtqueue(void *obj, void *data, + QGuestAllocator *t_alloc) +{ + QVhostUserBlkPCI *blk = obj; + QVirtioPCIDevice *pdev = &blk->pci_vdev; + QPCIBar bar0; + QPCIDevice *dev; + + dev = qpci_device_find(pdev->pdev->bus, QPCI_DEVFN(4, 0)); + g_assert(dev != NULL); + qpci_device_enable(dev); + + bar0 = qpci_iomap(dev, 0, NULL); + + qpci_io_writeb(dev, bar0, VIRTIO_PCI_QUEUE_SEL, 2); + qpci_io_writel(dev, bar0, VIRTIO_PCI_QUEUE_PFN, 1); + + g_free(dev); +} + +static const char *qtest_qemu_storage_daemon_binary(void) +{ + const char *qemu_storage_daemon_bin; + + qemu_storage_daemon_bin = getenv("QTEST_QEMU_STORAGE_DAEMON_BINARY"); + if (!qemu_storage_daemon_bin) { + fprintf(stderr, "Environment variable " + "QTEST_QEMU_STORAGE_DAEMON_BINARY required\n"); + exit(0); + } + + return qemu_storage_daemon_bin; +} + +/* g_test_queue_destroy() cleanup function for files */ +static void destroy_file(void *path) +{ + unlink(path); + g_free(path); + qos_invalidate_command_line(); +} + +static char *drive_create(void) +{ + int fd, ret; + /** vhost-user-blk won't recognize drive located in /tmp */ + char *t_path = g_strdup("qtest.XXXXXX"); + + /** Create a temporary raw image */ + fd = mkstemp(t_path); + g_assert_cmpint(fd, >=, 0); + ret = ftruncate(fd, TEST_IMAGE_SIZE); + g_assert_cmpint(ret, ==, 0); + close(fd); + + g_test_queue_destroy(destroy_file, t_path); + return t_path; +} + +static char *create_listen_socket(int *fd) +{ + int tmp_fd; + char *path; + + /* No race because our pid makes the path unique */ + path = g_strdup_printf("/tmp/qtest-%d-sock.XXXXXX", getpid()); + tmp_fd = mkstemp(path); + g_assert_cmpint(tmp_fd, >=, 0); + close(tmp_fd); + unlink(path); + + *fd = qtest_socket_server(path); + g_test_queue_destroy(destroy_file, path); + return path; +} + +/* + * g_test_queue_destroy() and qtest_add_abrt_handler() cleanup function for + * qemu-storage-daemon. + */ +static void quit_storage_daemon(void *data) +{ + QemuStorageDaemonState *qsd = data; + int wstatus; + pid_t pid; + + /* + * If we were invoked as a g_test_queue_destroy() cleanup function we need + * to remove the abrt handler to avoid being called again if the code below + * aborts. Also, we must not leave the abrt handler installed after + * cleanup. + */ + qtest_remove_abrt_handler(data); + + /* Before quitting storage-daemon, quit qemu to avoid dubious messages */ + qtest_kill_qemu(global_qtest); + + kill(qsd->pid, SIGTERM); + pid = waitpid(qsd->pid, &wstatus, 0); + g_assert_cmpint(pid, ==, qsd->pid); + if (!WIFEXITED(wstatus)) { + fprintf(stderr, "%s: expected qemu-storage-daemon to exit\n", + __func__); + abort(); + } + if (WEXITSTATUS(wstatus) != 0) { + fprintf(stderr, "%s: expected qemu-storage-daemon to exit " + "successfully, got %d\n", + __func__, WEXITSTATUS(wstatus)); + abort(); + } + + g_free(data); +} + +static void start_vhost_user_blk(GString *cmd_line, int vus_instances) +{ + const char *vhost_user_blk_bin = qtest_qemu_storage_daemon_binary(); + int i; + gchar *img_path; + GString *storage_daemon_command = g_string_new(NULL); + QemuStorageDaemonState *qsd; + + g_string_append_printf(storage_daemon_command, + "exec %s ", + vhost_user_blk_bin); + + g_string_append_printf(cmd_line, + " -object memory-backend-memfd,id=mem,size=256M,share=on " + " -M memory-backend=mem -m 256M "); + + for (i = 0; i < vus_instances; i++) { + int fd; + char *sock_path = create_listen_socket(&fd); + + /* create image file */ + img_path = drive_create(); + g_string_append_printf(storage_daemon_command, + "--blockdev driver=file,node-name=disk%d,filename=%s " + "--export type=vhost-user-blk,id=disk%d,addr.type=unix,addr.path=%s," + "node-name=disk%i,writable=on ", + i, img_path, i, sock_path, i); + + g_string_append_printf(cmd_line, "-chardev socket,id=char%d,path=%s ", + i + 1, sock_path); + } + + g_test_message("starting vhost-user backend: %s", + storage_daemon_command->str); + pid_t pid = fork(); + if (pid == 0) { + /* + * Close standard file descriptors so tap-driver.pl pipe detects when + * our parent terminates. + */ + close(0); + close(1); + open("/dev/null", O_RDONLY); + open("/dev/null", O_WRONLY); + + execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL); + exit(1); + } + g_string_free(storage_daemon_command, true); + + qsd = g_new(QemuStorageDaemonState, 1); + qsd->pid = pid; + + /* Make sure qemu-storage-daemon is stopped */ + qtest_add_abrt_handler(quit_storage_daemon, qsd); + g_test_queue_destroy(quit_storage_daemon, qsd); +} + +static void *vhost_user_blk_test_setup(GString *cmd_line, void *arg) +{ + start_vhost_user_blk(cmd_line, 1); + return arg; +} + +/* + * Setup for hotplug. + * + * Since vhost-user server only serves one vhost-user client one time, + * another exprot + * + */ +static void *vhost_user_blk_hotplug_test_setup(GString *cmd_line, void *arg) +{ + /* "-chardev socket,id=char2" is used for pci_hotplug*/ + start_vhost_user_blk(cmd_line, 2); + return arg; +} + +static void register_vhost_user_blk_test(void) +{ + QOSGraphTestOptions opts = { + .before = vhost_user_blk_test_setup, + }; + + /* + * tests for vhost-user-blk and vhost-user-blk-pci + * The tests are borrowed from tests/virtio-blk-test.c. But some tests + * regarding block_resize don't work for vhost-user-blk. + * vhost-user-blk device doesn't have -drive, so tests containing + * block_resize are also abandoned, + * - config + * - resize + */ + qos_add_test("basic", "vhost-user-blk", basic, &opts); + qos_add_test("indirect", "vhost-user-blk", indirect, &opts); + qos_add_test("idx", "vhost-user-blk-pci", idx, &opts); + qos_add_test("nxvirtq", "vhost-user-blk-pci", + test_nonexistent_virtqueue, &opts); + + opts.before = vhost_user_blk_hotplug_test_setup; + qos_add_test("hotplug", "vhost-user-blk-pci", pci_hotplug, &opts); +} + +libqos_init(register_vhost_user_blk_test); From 9c4e99e8794cd4ef1ef5e21fdc6efb6b5f779497 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 22 Mar 2021 09:23:26 +0000 Subject: [PATCH 06/13] tests/qtest: add multi-queue test case to vhost-user-blk-test Signed-off-by: Stefan Hajnoczi Message-Id: <20210309094106.196911-4-stefanha@redhat.com> Signed-off-by: Kevin Wolf Message-Id: <20210322092327.150720-3-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- tests/qtest/vhost-user-blk-test.c | 81 +++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 5 deletions(-) diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c index 3e79549899..d37e1c30bd 100644 --- a/tests/qtest/vhost-user-blk-test.c +++ b/tests/qtest/vhost-user-blk-test.c @@ -569,6 +569,67 @@ static void pci_hotplug(void *obj, void *data, QGuestAllocator *t_alloc) qpci_unplug_acpi_device_test(qts, "drv1", PCI_SLOT_HP); } +static void multiqueue(void *obj, void *data, QGuestAllocator *t_alloc) +{ + QVirtioPCIDevice *pdev1 = obj; + QVirtioDevice *dev1 = &pdev1->vdev; + QVirtioPCIDevice *pdev8; + QVirtioDevice *dev8; + QTestState *qts = pdev1->pdev->bus->qts; + uint64_t features; + uint16_t num_queues; + + /* + * The primary device has 1 queue and VIRTIO_BLK_F_MQ is not enabled. The + * VIRTIO specification allows VIRTIO_BLK_F_MQ to be enabled when there is + * only 1 virtqueue, but --device vhost-user-blk-pci doesn't do this (which + * is also spec-compliant). + */ + features = qvirtio_get_features(dev1); + g_assert_cmpint(features & (1u << VIRTIO_BLK_F_MQ), ==, 0); + features = features & ~(QVIRTIO_F_BAD_FEATURE | + (1u << VIRTIO_RING_F_INDIRECT_DESC) | + (1u << VIRTIO_F_NOTIFY_ON_EMPTY) | + (1u << VIRTIO_BLK_F_SCSI)); + qvirtio_set_features(dev1, features); + + /* Hotplug a secondary device with 8 queues */ + qtest_qmp_device_add(qts, "vhost-user-blk-pci", "drv1", + "{'addr': %s, 'chardev': 'char2', 'num-queues': 8}", + stringify(PCI_SLOT_HP) ".0"); + + pdev8 = virtio_pci_new(pdev1->pdev->bus, + &(QPCIAddress) { + .devfn = QPCI_DEVFN(PCI_SLOT_HP, 0) + }); + g_assert_nonnull(pdev8); + g_assert_cmpint(pdev8->vdev.device_type, ==, VIRTIO_ID_BLOCK); + + qos_object_start_hw(&pdev8->obj); + + dev8 = &pdev8->vdev; + features = qvirtio_get_features(dev8); + g_assert_cmpint(features & (1u << VIRTIO_BLK_F_MQ), + ==, + (1u << VIRTIO_BLK_F_MQ)); + features = features & ~(QVIRTIO_F_BAD_FEATURE | + (1u << VIRTIO_RING_F_INDIRECT_DESC) | + (1u << VIRTIO_F_NOTIFY_ON_EMPTY) | + (1u << VIRTIO_BLK_F_SCSI) | + (1u << VIRTIO_BLK_F_MQ)); + qvirtio_set_features(dev8, features); + + num_queues = qvirtio_config_readw(dev8, + offsetof(struct virtio_blk_config, num_queues)); + g_assert_cmpint(num_queues, ==, 8); + + qvirtio_pci_device_disable(pdev8); + qos_object_destroy(&pdev8->obj); + + /* unplug secondary disk */ + qpci_unplug_acpi_device_test(qts, "drv1", PCI_SLOT_HP); +} + /* * Check that setting the vring addr on a non-existent virtqueue does * not crash. @@ -688,7 +749,8 @@ static void quit_storage_daemon(void *data) g_free(data); } -static void start_vhost_user_blk(GString *cmd_line, int vus_instances) +static void start_vhost_user_blk(GString *cmd_line, int vus_instances, + int num_queues) { const char *vhost_user_blk_bin = qtest_qemu_storage_daemon_binary(); int i; @@ -713,8 +775,8 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances) g_string_append_printf(storage_daemon_command, "--blockdev driver=file,node-name=disk%d,filename=%s " "--export type=vhost-user-blk,id=disk%d,addr.type=unix,addr.path=%s," - "node-name=disk%i,writable=on ", - i, img_path, i, sock_path, i); + "node-name=disk%i,writable=on,num-queues=%d ", + i, img_path, i, sock_path, i, num_queues); g_string_append_printf(cmd_line, "-chardev socket,id=char%d,path=%s ", i + 1, sock_path); @@ -748,7 +810,7 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances) static void *vhost_user_blk_test_setup(GString *cmd_line, void *arg) { - start_vhost_user_blk(cmd_line, 1); + start_vhost_user_blk(cmd_line, 1, 1); return arg; } @@ -762,7 +824,13 @@ static void *vhost_user_blk_test_setup(GString *cmd_line, void *arg) static void *vhost_user_blk_hotplug_test_setup(GString *cmd_line, void *arg) { /* "-chardev socket,id=char2" is used for pci_hotplug*/ - start_vhost_user_blk(cmd_line, 2); + start_vhost_user_blk(cmd_line, 2, 1); + return arg; +} + +static void *vhost_user_blk_multiqueue_test_setup(GString *cmd_line, void *arg) +{ + start_vhost_user_blk(cmd_line, 2, 8); return arg; } @@ -789,6 +857,9 @@ static void register_vhost_user_blk_test(void) opts.before = vhost_user_blk_hotplug_test_setup; qos_add_test("hotplug", "vhost-user-blk-pci", pci_hotplug, &opts); + + opts.before = vhost_user_blk_multiqueue_test_setup; + qos_add_test("multiqueue", "vhost-user-blk-pci", multiqueue, &opts); } libqos_init(register_vhost_user_blk_test); From 7999e3136d5f0ba474bc1b37e3799099e778f9ed Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 22 Mar 2021 09:23:27 +0000 Subject: [PATCH 07/13] vhost-user-blk-test: test discard/write zeroes invalid inputs Exercise input validation code paths in block/export/vhost-user-blk-server.c. Signed-off-by: Stefan Hajnoczi Message-Id: <20210309094106.196911-5-stefanha@redhat.com> Signed-off-by: Kevin Wolf Message-Id: <20210322092327.150720-4-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- tests/qtest/vhost-user-blk-test.c | 124 ++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c index d37e1c30bd..8796c74ca4 100644 --- a/tests/qtest/vhost-user-blk-test.c +++ b/tests/qtest/vhost-user-blk-test.c @@ -94,6 +94,124 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d, return addr; } +static void test_invalid_discard_write_zeroes(QVirtioDevice *dev, + QGuestAllocator *alloc, + QTestState *qts, + QVirtQueue *vq, + uint32_t type) +{ + QVirtioBlkReq req; + struct virtio_blk_discard_write_zeroes dwz_hdr; + struct virtio_blk_discard_write_zeroes dwz_hdr2[2]; + uint64_t req_addr; + uint32_t free_head; + uint8_t status; + + /* More than one dwz is not supported */ + req.type = type; + req.data = (char *) dwz_hdr2; + dwz_hdr2[0].sector = 0; + dwz_hdr2[0].num_sectors = 1; + dwz_hdr2[0].flags = 0; + dwz_hdr2[1].sector = 1; + dwz_hdr2[1].num_sectors = 1; + dwz_hdr2[1].flags = 0; + + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr2[0]); + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr2[1]); + + req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr2)); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr2), false, true); + qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr2), 1, true, + false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 16 + sizeof(dwz_hdr2)); + g_assert_cmpint(status, ==, VIRTIO_BLK_S_UNSUPP); + + guest_free(alloc, req_addr); + + /* num_sectors must be less than config->max_write_zeroes_sectors */ + req.type = type; + req.data = (char *) &dwz_hdr; + dwz_hdr.sector = 0; + dwz_hdr.num_sectors = 0xffffffff; + dwz_hdr.flags = 0; + + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr); + + req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr)); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true); + qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, + false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 16 + sizeof(dwz_hdr)); + g_assert_cmpint(status, ==, VIRTIO_BLK_S_IOERR); + + guest_free(alloc, req_addr); + + /* sector must be less than the device capacity */ + req.type = type; + req.data = (char *) &dwz_hdr; + dwz_hdr.sector = TEST_IMAGE_SIZE / 512 + 1; + dwz_hdr.num_sectors = 1; + dwz_hdr.flags = 0; + + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr); + + req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr)); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true); + qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, + false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 16 + sizeof(dwz_hdr)); + g_assert_cmpint(status, ==, VIRTIO_BLK_S_IOERR); + + guest_free(alloc, req_addr); + + /* reserved flag bits must be zero */ + req.type = type; + req.data = (char *) &dwz_hdr; + dwz_hdr.sector = 0; + dwz_hdr.num_sectors = 1; + dwz_hdr.flags = ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP; + + virtio_blk_fix_dwz_hdr(dev, &dwz_hdr); + + req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr)); + + free_head = qvirtqueue_add(qts, vq, req_addr, 16, false, true); + qvirtqueue_add(qts, vq, req_addr + 16, sizeof(dwz_hdr), false, true); + qvirtqueue_add(qts, vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, + false); + + qvirtqueue_kick(qts, dev, vq, free_head); + + qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL, + QVIRTIO_BLK_TIMEOUT_US); + status = readb(req_addr + 16 + sizeof(dwz_hdr)); + g_assert_cmpint(status, ==, VIRTIO_BLK_S_UNSUPP); + + guest_free(alloc, req_addr); +} + /* Returns the request virtqueue so the caller can perform further tests */ static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc) { @@ -235,6 +353,9 @@ static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc) g_free(data); guest_free(alloc, req_addr); + + test_invalid_discard_write_zeroes(dev, alloc, qts, vq, + VIRTIO_BLK_T_WRITE_ZEROES); } if (features & (1u << VIRTIO_BLK_F_DISCARD)) { @@ -263,6 +384,9 @@ static QVirtQueue *test_basic(QVirtioDevice *dev, QGuestAllocator *alloc) g_assert_cmpint(status, ==, 0); guest_free(alloc, req_addr); + + test_invalid_discard_write_zeroes(dev, alloc, qts, vq, + VIRTIO_BLK_T_DISCARD); } if (features & (1u << VIRTIO_F_ANY_LAYOUT)) { From f26729715ef21325f972f693607580a829ad1cbb Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 29 Apr 2021 19:13:11 +0200 Subject: [PATCH 08/13] vhost-user-blk: Make sure to set Error on realize failure We have to set errp before jumping to virtio_err, otherwise the caller (virtio_device_realize()) will take this as success and crash when it later tries to access things that we've already freed in the error path. Fixes: 77542d431491788d1e8e79d93ce10172ef207775 Signed-off-by: Kevin Wolf Message-Id: <20210429171316.162022-2-kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Acked-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index f5e9682703..7c85248a7b 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -447,7 +447,6 @@ 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; int i, ret; if (!s->chardev.chr) { @@ -495,8 +494,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) NULL, true); reconnect: - if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) { - error_report_err(err); + if (qemu_chr_fe_wait_connected(&s->chardev, errp) < 0) { goto virtio_err; } From dabefdd6abcbc7d858e9413e4734aab2e0b5c8d9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 29 Apr 2021 19:13:12 +0200 Subject: [PATCH 09/13] vhost-user-blk: Don't reconnect during initialisation This is a partial revert of commits 77542d43149 and bc79c87bcde. Usually, an error during initialisation means that the configuration was wrong. Reconnecting won't make the error go away, but just turn the error condition into an endless loop. Avoid this and return errors again. Additionally, calling vhost_user_blk_disconnect() from the chardev event handler could result in use-after-free because none of the initialisation code expects that the device could just go away in the middle. So removing the call fixes crashes in several places. For example, using a num-queues setting that is incompatible with the backend would result in a crash like this (dereferencing dev->opaque, which is already NULL): #0 0x0000555555d0a4bd in vhost_user_read_cb (source=0x5555568f4690, condition=(G_IO_IN | G_IO_HUP), opaque=0x7fffffffcbf0) at ../hw/virtio/vhost-user.c:313 #1 0x0000555555d950d3 in qio_channel_fd_source_dispatch (source=0x555557c3f750, callback=0x555555d0a478 , user_data=0x7fffffffcbf0) at ../io/channel-watch.c:84 #2 0x00007ffff7b32a9f in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #3 0x00007ffff7b84a98 in g_main_context_iterate.constprop () at /lib64/libglib-2.0.so.0 #4 0x00007ffff7b32163 in g_main_loop_run () at /lib64/libglib-2.0.so.0 #5 0x0000555555d0a724 in vhost_user_read (dev=0x555557bc62f8, msg=0x7fffffffcc50) at ../hw/virtio/vhost-user.c:402 #6 0x0000555555d0ee6b in vhost_user_get_config (dev=0x555557bc62f8, config=0x555557bc62ac "", config_len=60) at ../hw/virtio/vhost-user.c:2133 #7 0x0000555555d56d46 in vhost_dev_get_config (hdev=0x555557bc62f8, config=0x555557bc62ac "", config_len=60) at ../hw/virtio/vhost.c:1566 #8 0x0000555555cdd150 in vhost_user_blk_device_realize (dev=0x555557bc60b0, errp=0x7fffffffcf90) at ../hw/block/vhost-user-blk.c:510 #9 0x0000555555d08f6d in virtio_device_realize (dev=0x555557bc60b0, errp=0x7fffffffcff0) at ../hw/virtio/virtio.c:3660 Note that this removes the ability to reconnect during initialisation (but not during operation) when there is no permanent error, but the backend restarts, as the implementation was buggy. This feature can be added back in a follow-up series after changing error paths to distinguish cases where retrying could help from cases with permanent errors. Signed-off-by: Kevin Wolf Message-Id: <20210429171316.162022-3-kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 59 +++++++++++---------------------------- 1 file changed, 17 insertions(+), 42 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 7c85248a7b..c0b9958da1 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -50,6 +50,8 @@ static const int user_feature_bits[] = { VHOST_INVALID_FEATURE_BIT }; +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event); + static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config) { VHostUserBlk *s = VHOST_USER_BLK(vdev); @@ -362,19 +364,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev) vhost_dev_cleanup(&s->dev); } -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, - bool realized); - -static void vhost_user_blk_event_realize(void *opaque, QEMUChrEvent event) -{ - vhost_user_blk_event(opaque, event, false); -} - -static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event) -{ - vhost_user_blk_event(opaque, event, true); -} - static void vhost_user_blk_chr_closed_bh(void *opaque) { DeviceState *dev = opaque; @@ -382,12 +371,11 @@ static void vhost_user_blk_chr_closed_bh(void *opaque) VHostUserBlk *s = VHOST_USER_BLK(vdev); vhost_user_blk_disconnect(dev); - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, - vhost_user_blk_event_oper, NULL, opaque, NULL, true); + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event, + NULL, opaque, NULL, true); } -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, - bool realized) +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) { DeviceState *dev = opaque; VirtIODevice *vdev = VIRTIO_DEVICE(dev); @@ -401,17 +389,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, } break; case CHR_EVENT_CLOSED: - /* - * Closing the connection should happen differently on device - * initialization and operation stages. - * On initalization, we want to re-start vhost_dev initialization - * from the very beginning right away when the connection is closed, - * so we clean up vhost_dev on each connection closing. - * On operation, we want to postpone vhost_dev cleanup to let the - * other code perform its own cleanup sequence using vhost_dev data - * (e.g. vhost_dev_set_log). - */ - if (realized && !runstate_check(RUN_STATE_SHUTDOWN)) { + if (!runstate_check(RUN_STATE_SHUTDOWN)) { /* * A close event may happen during a read/write, but vhost * code assumes the vhost_dev remains setup, so delay the @@ -431,8 +409,6 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event, * knowing its type (in this case vhost-user). */ s->dev.started = false; - } else { - vhost_user_blk_disconnect(dev); } break; case CHR_EVENT_BREAK: @@ -489,33 +465,32 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues); s->connected = false; - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, - vhost_user_blk_event_realize, NULL, (void *)dev, - NULL, true); - -reconnect: if (qemu_chr_fe_wait_connected(&s->chardev, errp) < 0) { goto virtio_err; } - /* check whether vhost_user_blk_connect() failed or not */ - if (!s->connected) { - goto reconnect; + if (vhost_user_blk_connect(dev) < 0) { + error_setg(errp, "vhost-user-blk: could not connect"); + qemu_chr_fe_disconnect(&s->chardev); + goto virtio_err; } + assert(s->connected); ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, sizeof(struct virtio_blk_config)); if (ret < 0) { - error_report("vhost-user-blk: get block config failed"); - goto reconnect; + error_setg(errp, "vhost-user-blk: get block config failed"); + goto vhost_err; } - /* we're fully initialized, now we can operate, so change the handler */ + /* we're fully initialized, now we can operate, so add the handler */ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, - vhost_user_blk_event_oper, NULL, (void *)dev, + vhost_user_blk_event, NULL, (void *)dev, NULL, true); return; +vhost_err: + vhost_dev_cleanup(&s->dev); virtio_err: g_free(s->vhost_vqs); s->vhost_vqs = NULL; From 5b9243d2654adc58ce472d0536a7a177b4fe0f90 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 29 Apr 2021 19:13:13 +0200 Subject: [PATCH 10/13] vhost-user-blk: Improve error reporting in realize Now that vhost_user_blk_connect() is not called from an event handler any more, but directly from vhost_user_blk_device_realize(), we can actually make use of Error again instead of calling error_report() in the inner function and setting a more generic and therefore less useful error message in realize() itself. With Error, the callers are responsible for adding context if necessary (such as the "-device" option the error refers to). Additional prefixes are redundant and better omitted. Signed-off-by: Kevin Wolf Acked-by: Raphael Norwitz Message-Id: <20210429171316.162022-4-kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index c0b9958da1..f3a45af97c 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -311,7 +311,7 @@ static void vhost_user_blk_reset(VirtIODevice *vdev) vhost_dev_free_inflight(s->inflight); } -static int vhost_user_blk_connect(DeviceState *dev) +static int vhost_user_blk_connect(DeviceState *dev, Error **errp) { VirtIODevice *vdev = VIRTIO_DEVICE(dev); VHostUserBlk *s = VHOST_USER_BLK(vdev); @@ -331,8 +331,7 @@ static int vhost_user_blk_connect(DeviceState *dev) ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0); if (ret < 0) { - error_report("vhost-user-blk: vhost initialization failed: %s", - strerror(-ret)); + error_setg_errno(errp, -ret, "vhost initialization failed"); return ret; } @@ -340,8 +339,7 @@ static int vhost_user_blk_connect(DeviceState *dev) if (virtio_device_started(vdev, vdev->status)) { ret = vhost_user_blk_start(vdev); if (ret < 0) { - error_report("vhost-user-blk: vhost start failed: %s", - strerror(-ret)); + error_setg_errno(errp, -ret, "vhost start failed"); return ret; } } @@ -380,10 +378,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) DeviceState *dev = opaque; VirtIODevice *vdev = VIRTIO_DEVICE(dev); VHostUserBlk *s = VHOST_USER_BLK(vdev); + Error *local_err = NULL; switch (event) { case CHR_EVENT_OPENED: - if (vhost_user_blk_connect(dev) < 0) { + if (vhost_user_blk_connect(dev, &local_err) < 0) { + error_report_err(local_err); qemu_chr_fe_disconnect(&s->chardev); return; } @@ -426,7 +426,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) int i, ret; if (!s->chardev.chr) { - error_setg(errp, "vhost-user-blk: chardev is mandatory"); + error_setg(errp, "chardev is mandatory"); return; } @@ -434,16 +434,16 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->num_queues = 1; } if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) { - error_setg(errp, "vhost-user-blk: invalid number of IO queues"); + error_setg(errp, "invalid number of IO queues"); return; } if (!s->queue_size) { - error_setg(errp, "vhost-user-blk: queue size must be non-zero"); + error_setg(errp, "queue size must be non-zero"); return; } if (s->queue_size > VIRTQUEUE_MAX_SIZE) { - error_setg(errp, "vhost-user-blk: queue size must not exceed %d", + error_setg(errp, "queue size must not exceed %d", VIRTQUEUE_MAX_SIZE); return; } @@ -469,8 +469,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) goto virtio_err; } - if (vhost_user_blk_connect(dev) < 0) { - error_setg(errp, "vhost-user-blk: could not connect"); + if (vhost_user_blk_connect(dev, errp) < 0) { qemu_chr_fe_disconnect(&s->chardev); goto virtio_err; } From 7556a320c98812ca6648b707393f4513387faf73 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 29 Apr 2021 19:13:14 +0200 Subject: [PATCH 11/13] vhost-user-blk: Get more feature flags from vhost device VIRTIO_F_RING_PACKED and VIRTIO_F_IOMMU_PLATFORM need to be supported by the vhost device, otherwise advertising it to the guest doesn't result in a working configuration. They are currently not supported by the vhost-user-blk export in QEMU. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1935020 Signed-off-by: Kevin Wolf Acked-by: Raphael Norwitz Message-Id: <20210429171316.162022-5-kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index f3a45af97c..c7e502f4c7 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -47,6 +47,8 @@ static const int user_feature_bits[] = { VIRTIO_RING_F_INDIRECT_DESC, VIRTIO_RING_F_EVENT_IDX, VIRTIO_F_NOTIFY_ON_EMPTY, + VIRTIO_F_RING_PACKED, + VIRTIO_F_IOMMU_PLATFORM, VHOST_INVALID_FEATURE_BIT }; From 04ceb61a4075fadbf374ef89662c41999da83489 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 29 Apr 2021 19:13:15 +0200 Subject: [PATCH 12/13] virtio: Fail if iommu_platform is requested, but unsupported Commit 2943b53f6 (' virtio: force VIRTIO_F_IOMMU_PLATFORM') made sure that vhost can't just reject VIRTIO_F_IOMMU_PLATFORM when it was requested. However, just adding it back to the negotiated flags isn't right either because it promises support to the guest that the device actually doesn't support. One example of a vhost-user device that doesn't have support for the flag is the vhost-user-blk export of QEMU. Instead of successfully creating a device that doesn't work, just fail to plug the device when it doesn't support the feature, but it was requested. This results in much clearer error messages. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1935019 Signed-off-by: Kevin Wolf Reviewed-by: Raphael Norwitz Message-Id: <20210429171316.162022-6-kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/virtio/virtio-bus.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index d6332d45c3..859978d248 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -69,6 +69,11 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) return; } + if (has_iommu && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { + error_setg(errp, "iommu_platform=true is not supported by the device"); + return; + } + if (klass->device_plugged != NULL) { klass->device_plugged(qbus->parent, &local_err); } From c90bd505a3e8210c23d69fecab9ee6f56ec4a161 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 29 Apr 2021 19:13:16 +0200 Subject: [PATCH 13/13] vhost-user-blk: Check that num-queues is supported by backend Creating a device with a number of queues that isn't supported by the backend is pointless, the device won't work properly and the error messages are rather confusing. Just fail to create the device if num-queues is higher than what the backend supports. Since the relationship between num-queues and the number of virtqueues depends on the specific device, this is an additional value that needs to be initialised by the device. For convenience, allow leaving it 0 if the check should be skipped. This makes sense for vhost-user-net where separate vhost devices are used for the queues and custom initialisation code is needed to perform the check. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1935031 Signed-off-by: Kevin Wolf Reviewed-by: Raphael Norwitz Message-Id: <20210429171316.162022-7-kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 1 + hw/virtio/vhost-user.c | 5 +++++ include/hw/virtio/vhost.h | 2 ++ 3 files changed, 8 insertions(+) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index c7e502f4c7..c6210fad0c 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -324,6 +324,7 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp) } s->connected = true; + s->dev.num_queues = s->num_queues; s->dev.nvqs = s->num_queues; s->dev.vqs = s->vhost_vqs; s->dev.vq_index = 0; diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index ded0c10453..ee57abe045 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1909,6 +1909,11 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) return err; } } + if (dev->num_queues && dev->max_queues < dev->num_queues) { + error_report("The maximum number of queues supported by the " + "backend is %" PRIu64, dev->max_queues); + return -EINVAL; + } if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) && !(virtio_has_feature(dev->protocol_features, diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 4a8bc75415..21a9a52088 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -74,6 +74,8 @@ struct vhost_dev { int nvqs; /* the first virtqueue which would be used by this vhost dev */ int vq_index; + /* if non-zero, minimum required value for max_queues */ + int num_queues; uint64_t features; uint64_t acked_features; uint64_t backend_features;