Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
On 8/23/22 13:11, Denis V. Lunev wrote:
On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote:
On 8/23/22 12:20, Denis V. Lunev wrote:
On 23.08.2022 09:23, Alexander Ivanov wrote:
On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
On 8/22/22 12:05, Alexander Ivanov wrote:
data_end field in BDRVParallelsState is set to the biggest offset present
in BAT. If this offset is outside of the image, any further write will create
the cluster at this offset and/or the image will be truncated to this
offset on close. This is definitely not correct.
Raise an error in parallels_open() if data_end points outside the image and
it is not a check (let the check to repaire the image).
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..c245ca35cd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
int ret, size, i;
+ int64_t file_size;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
}
+ file_size = bdrv_getlength(bs->file->bs);
+ if (file_size < 0) {
+ ret = file_size;
+ goto fail;
+ }
+
+ file_size >>= BDRV_SECTOR_BITS;
+ if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
+ error_setg(errp, "parallels: Offset in BAT is out of image");
+ ret = -EINVAL;
+ goto fail;
+ }
If image is unaligned to sector size, and image size is less than s->data_end,
but the difference itself is less than sector, the error message would be
misleading.
Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of
"file_size >>= BDRV_SECTOR_BITS"?
It's hardly possible to get such image on valid scenarios with Qemu (keeping in
mind bdrv_truncate() call in parallels_close()). But it still may be possible
to have such images produced by another software or by some failure path.
I think you are right, it would be better to align image size up to sector size.
I would say that we need to align not on sector size but on cluster size.
That would worth additional check.
And not simply align, as data_offset is not necessarily aligned to cluster size.
Finally, what should we check?
I suggest
diff --git a/block/parallels.c b/block/parallels.c
index 6d4ed77f16..b882ea1200 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -725,6 +725,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
int ret, size, i;
+ int64_t file_size;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
return -EINVAL;
}
+ file_size = bdrv_getlength(bs->file->bs);
+ if (file_size < 0) {
+ return file_size;
+ }
+
ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
if (ret < 0) {
goto fail;
@@ -798,6 +804,13 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
int64_t off = bat2sect(s, i);
+ if (off >= file_size) {
Like this, especially >= check which we have had missed.
Though this would break the repair. We need additional
if (flags & BDRV_O_CHECK) {
continue;
}
No incorrect data_end assignment, which would be
very welcome.
Den
'continue' here will change the logic around data_end. We'll drop "wrong"
clusters from calculation of data_end, and should check, how it affects further logic.
What about:
for (i = 0; i < s->bat_size; i++) {
int64_t off = bat2sect(s, i);
if (off >= file_size && !(flags & BDRV_O_CHECK)) {
error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
"is larger than file size (%" PRIi64 ")",
off, i, file_size);
ret = -EINVAL;
goto fail;
}
if (off >= s->data_end) {
s->data_end = off + s->tracks;
}
}
- this we simply add new error-out on no-O_CHECK path.
+ error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
+ "is larger than file size (%" PRIi64 ")",
+ off, i, file_size);
+ ret = -EINVAL;
+ goto fail;
+ }
if (off >= s->data_end) {
s->data_end = off + s->tracks;
}
- better error message, and we check exactly what's written in the spec
(docs/interop/parallels.c):
Cluster offsets specified by BAT entries must meet the following requirements:
[...]
- the value
[PATCH v3 3/3] util/aio-win32: Correct the event array size in aio_poll()
From: Bin Meng
WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
object handles. Correct the event array size in aio_poll() and
add a assert() to ensure it does not cause out of bound access.
Signed-off-by: Bin Meng
Reviewed-by: Stefan Weil
Reviewed-by: Marc-André Lureau
---
(no changes since v2)
Changes in v2:
- change 'count' to unsigned
util/aio-win32.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..80cfe012ad 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,9 +326,9 @@ void aio_dispatch(AioContext *ctx)
bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
-HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
+HANDLE events[MAXIMUM_WAIT_OBJECTS];
bool progress, have_select_revents, first;
-int count;
+unsigned count;
int timeout;
/*
@@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
QLIST_FOREACH_RCU(node, &ctx->aio_handlers, node) {
if (!node->deleted && node->io_notify
&& aio_node_check(ctx, node->is_external)) {
+assert(count < MAXIMUM_WAIT_OBJECTS);
events[count++] = event_notifier_get_handle(node->e);
}
}
--
2.34.1
[PATCH 02/51] tests/qtest: Use g_mkdtemp()
From: Bin Meng
Windows does not provide a mkdtemp() API, but glib does.
Replace mkdtemp() call with the glib version.
Signed-off-by: Bin Meng
---
tests/qtest/fuzz/generic_fuzz_configs.h | 2 +-
tests/qtest/cdrom-test.c| 2 +-
tests/qtest/cxl-test.c | 6 +++---
tests/qtest/ivshmem-test.c | 4 ++--
tests/qtest/libqos/virtio-9p.c | 4 ++--
tests/qtest/libqtest.c | 2 +-
tests/qtest/migration-test.c| 4 ++--
tests/qtest/qmp-test.c | 4 ++--
tests/qtest/vhost-user-test.c | 4 ++--
tests/unit/test-qga.c | 2 +-
10 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 004c701915..0775e6702b 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -21,7 +21,7 @@ typedef struct generic_fuzz_config {
static inline gchar *generic_fuzzer_virtio_9p_args(void){
char tmpdir[] = "/tmp/qemu-fuzz.XX";
-g_assert_nonnull(mkdtemp(tmpdir));
+g_assert_nonnull(g_mkdtemp(tmpdir));
return g_strdup_printf("-machine q35 -nodefaults "
"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index a7766a9e65..26a2400181 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -52,7 +52,7 @@ static int prepare_image(const char *arch, char *isoimage)
perror("Error creating temporary iso image file");
return -1;
}
-if (!mkdtemp(srcdir)) {
+if (!g_mkdtemp(srcdir)) {
perror("Error creating temporary directory");
goto cleanup;
}
diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index 2133e973f4..4e6d285061 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -95,7 +95,7 @@ static void cxl_t3d(void)
char template[] = "/tmp/cxl-test-XX";
const char *tmpfs;
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D, tmpfs, tmpfs);
@@ -109,7 +109,7 @@ static void cxl_1pxb_2rp_2t3d(void)
char template[] = "/tmp/cxl-test-XX";
const char *tmpfs;
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
g_string_printf(cmdline, QEMU_PXB_CMD QEMU_2RP QEMU_2T3D,
tmpfs, tmpfs, tmpfs, tmpfs);
@@ -124,7 +124,7 @@ static void cxl_2pxb_4rp_4t3d(void)
char template[] = "/tmp/cxl-test-XX";
const char *tmpfs;
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
g_string_printf(cmdline, QEMU_2PXB_CMD QEMU_4RP QEMU_4T3D,
tmpfs, tmpfs, tmpfs, tmpfs, tmpfs, tmpfs,
diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
index e23a97fa8e..9611d05eb5 100644
--- a/tests/qtest/ivshmem-test.c
+++ b/tests/qtest/ivshmem-test.c
@@ -481,8 +481,8 @@ int main(int argc, char **argv)
tmpshmem = mmap(0, TMPSHMSIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
g_assert(tmpshmem != MAP_FAILED);
/* server */
-if (mkdtemp(dir) == NULL) {
-g_error("mkdtemp: %s", g_strerror(errno));
+if (g_mkdtemp(dir) == NULL) {
+g_error("g_mkdtemp: %s", g_strerror(errno));
}
tmpdir = dir;
tmpserver = g_strconcat(tmpdir, "/server", NULL);
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 70aea8bf62..ae9b0a20e2 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -48,9 +48,9 @@ void virtio_9p_create_local_test_dir(void)
*/
char *template = concat_path(pwd, "qtest-9p-local-XX");
-local_test_path = mkdtemp(template);
+local_test_path = g_mkdtemp(template);
if (!local_test_path) {
-g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+g_test_message("g_mkdtemp('%s') failed: %s", template,
strerror(errno));
}
g_assert(local_test_path != NULL);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index ad6860d774..7c9fc07de4 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -393,7 +393,7 @@ QTestState *qtest_init_with_serial(const char *extra_args,
int *sock_fd)
char *sock_path, sock_dir[] = "/tmp/qtest-serial-XX";
QTestState *qts;
-g_assert_true(mkdtemp(sock_dir) != NULL);
+g_assert_true(g_mkdtemp(sock_dir) != NULL);
sock_path = g_strdup_printf("%s/sock", sock_dir);
sock_fd_init = init_socket(sock_path);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 520a5f917c..52988b86eb 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2450,9 +2450,9 @@ int main(int argc, char **argv)
return g_test_run();
}
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
if (!tmpfs) {
-g_test_message("mkdtemp on path
[PATCH 07/51] tests: Avoid using hardcoded /tmp in test cases
From: Bin Meng
Use g_get_tmp_dir() to get the directory to use for temporary files.
Signed-off-by: Bin Meng
---
tests/qtest/fuzz/generic_fuzz_configs.h | 6 --
tests/qtest/ahci-test.c | 15 +++
tests/qtest/aspeed_smc-test.c | 4 +++-
tests/qtest/boot-serial-test.c | 8 ++--
tests/qtest/cxl-test.c | 9 ++---
tests/qtest/fdc-test.c | 4 +++-
tests/qtest/fuzz/virtio_blk_fuzz.c | 2 +-
tests/qtest/hd-geo-test.c | 8
tests/qtest/ide-test.c | 8 ++--
tests/qtest/libqtest.c | 10 +++---
tests/qtest/migration-test.c| 4 +++-
tests/qtest/pflash-cfi02-test.c | 7 +--
tests/qtest/qmp-test.c | 4 +++-
tests/qtest/vhost-user-blk-test.c | 3 ++-
tests/qtest/vhost-user-test.c | 3 ++-
tests/qtest/virtio-blk-test.c | 2 +-
tests/qtest/virtio-scsi-test.c | 3 ++-
tests/unit/test-image-locking.c | 6 --
tests/unit/test-qga.c | 2 +-
tests/vhost-user-bridge.c | 3 ++-
20 files changed, 76 insertions(+), 35 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 0775e6702b..d0f9961187 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -20,13 +20,15 @@ typedef struct generic_fuzz_config {
} generic_fuzz_config;
static inline gchar *generic_fuzzer_virtio_9p_args(void){
-char tmpdir[] = "/tmp/qemu-fuzz.XX";
+char *tmpdir = g_strdup_printf("%s/qemu-fuzz.XX", g_get_tmp_dir());
g_assert_nonnull(g_mkdtemp(tmpdir));
-return g_strdup_printf("-machine q35 -nodefaults "
+gchar *args = g_strdup_printf("-machine q35 -nodefaults "
"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
"-fsdev local,id=hshare,path=%s,security_model=mapped-xattr,"
"writeout=immediate,fmode=0600,dmode=0700", tmpdir);
+g_free(tmpdir);
+return args;
}
const generic_fuzz_config predefined_configs[] = {
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f1e510b0ac..f26cd6f86f 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -44,9 +44,9 @@
#define TEST_IMAGE_SIZE_MB_SMALL 64
/*** Globals ***/
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
-static char mig_socket[] = "/tmp/qtest-migration.XX";
+static char *tmp_path;
+static char *debug_path;
+static char *mig_socket;
static bool ahci_pedantic;
static const char *imgfmt;
static unsigned test_image_size_mb;
@@ -1437,7 +1437,7 @@ static void test_ncq_simple(void)
static int prepare_iso(size_t size, unsigned char **buf, char **name)
{
-char cdrom_path[] = "/tmp/qtest.iso.XX";
+char *cdrom_path = g_strdup_printf("%s/qtest.iso.XX", g_get_tmp_dir());
unsigned char *patt;
ssize_t ret;
int fd = mkstemp(cdrom_path);
@@ -1454,6 +1454,7 @@ static int prepare_iso(size_t size, unsigned char **buf,
char **name)
*name = g_strdup(cdrom_path);
*buf = patt;
+g_free(cdrom_path);
return fd;
}
@@ -1872,6 +1873,7 @@ int main(int argc, char **argv)
}
/* Create a temporary image */
+tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
@@ -1889,11 +1891,13 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", g_get_tmp_dir());
fd = mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Reserve a hollow file to use as a socket for migration tests */
+mig_socket = g_strdup_printf("%s/qtest-migration.XX", g_get_tmp_dir());
fd = mkstemp(mig_socket);
g_assert(fd >= 0);
close(fd);
@@ -1947,8 +1951,11 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
unlink(mig_socket);
+g_free(mig_socket);
return ret;
}
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index 05ce941566..cab769459c 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -608,7 +608,7 @@ static void test_write_block_protect_bottom_bit(void)
flash_reset();
}
-static char tmp_path[] = "/tmp/qtest.m25p80.XX";
+static char *tmp_path;
int main(int argc, char **argv)
{
@@ -617,6 +617,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
+tmp_path = g_strdup_printf("%s/qtest.m25p80.XX", g_get_tmp_dir());
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
ret = ftruncate(fd, FLASH_SIZE);
@@ -646,5 +647,6 @@ int main(int argc, char **argv)
qtest_quit(global_qtest);
u
[PATCH 00/51] tests/qtest: Enable running qtest on Windows
In prepartion to adding virtio-9p support on Windows, this series
enables running qtest on Windows, so that we can run the virtio-9p
tests on Windows to make sure it does not break accidently.
Patch 1-22 updates various components (mostly test cases) so that
they can build on Windows with the same functionality.
Patch 23 supports the qtest accelerator for Windows.
Patch 31 updates the libqtest core for Windows.
Patch 32-47 are the fixes of qtest errors exposed when running on Windows.
Patch 49 fixes the instability of running qtests on Windows.
Patch 50 updates the CI to run qtests on Windows.
Patch 51 documents best practices of writing portable test cases as
we learned during the enablement of running qtest on Windows.
Based-on: <20220802075200.907360-1-bmeng...@gmail.com>
Bin Meng (41):
tests/qtest: Use g_setenv()
tests/qtest: Use g_mkdtemp()
block: Unify the get_tmp_filename() implementation
semihosting/arm-compat-semi: Avoid using hardcoded /tmp
tcg: Avoid using hardcoded /tmp
util/qemu-sockets: Use g_get_tmp_dir() to get the directory for
temporary files
tests: Avoid using hardcoded /tmp in test cases
block/vvfat: Unify the mkdir() call
fsdev/virtfs-proxy-helper: Use g_mkdir_with_parents()
hw/usb: dev-mtp: Use g_mkdir_with_parents()
qga/commands-posix-ssh: Use g_mkdir_with_parents()
tests: Use g_mkdir_with_parents()
tests/qtest: migration-test: Handle link() for win32
backends/tpm: Exclude headers and macros that don't exist on win32
tests/qtest: Adapt {m48t59,rtc}-test cases for win32
tests/qtest: Build e1000e-test for posix only
tests/qtest: Build virtio-net-test for posix only
tests/qtest: Build cases that use memory-backend-file for posix only
tests/qtest: Build test-filter-{mirror,redirector} cases for posix
only
tests/qtest: i440fx-test: Skip running request_{bios,pflash} for win32
tests/qtest: migration-test: Skip running test_migrate_fd_proto on
win32
tests/qtest: qmp-test: Skip running test_qmp_oob for win32
tests/qtest: libqtest: Exclude the *_fds APIs for win32
tests/qtest: libqtest: Install signal handler via signal()
tests: Skip iotests and qtest when '--without-default-devices'
tests/qtest: Support libqtest to build and run on Windows
tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
tests/qtest: {ahci,ide}-test: Use relative path for temporary files
tests/qtest: bios-tables-test: Adapt the case for win32
tests/qtest: device-plug-test: Reverse the usage of double/single
quotes
tests/qtest: machine-none-test: Use double quotes to pass the cpu
option
tests/qtest: migration-test: Disable IO redirection for win32
tests/qtest: npcm7xx_emc-test: Skip running test_{tx,rx} on win32
tests/qtest: microbit-test: Fix socket access for win32
tests/qtest: prom-env-test: Use double quotes to pass the prom-env
option
tests/qtest: libqtest: Replace the call to close a socket with
closesocket()
tests/qtest: libqtest: Correct the timeout unit of blocking receive
calls for win32
io/channel-watch: Drop a superfluous '#ifdef WIN32'
io/channel-watch: Fix socket watch on Windows
.gitlab-ci.d/windows.yml: Increase the timeout to the runner limit
docs/devel: testing: Document writing portable test cases
Xuzhou Cheng (10):
accel/qtest: Support qtest accelerator for Windows
tests/qtest: libqos: Drop inclusion of
tests/qtest: libqos: Rename malloc.h to libqos-malloc.h
tests/qtest: libqtest: Move global_qtest definition back to libqtest.c
tests/qtest: Use send/recv for socket communication
tests/qtest: {ahci,ide}-test: Open file in binary mode
tests/qtest: virtio-net-failover: Disable migration tests for win32
chardev/char-file: Add FILE_SHARE_WRITE when openning the file for
win32
tests/qtest: migration-test: Kill "to" after migration is canceled
hw/ppc: spapr: Use qemu_vfree() to free spapr->htab
docs/devel/testing.rst| 30
backends/tpm/tpm_ioctl.h | 4 +
include/hw/core/cpu.h | 1 +
tests/qtest/fuzz/generic_fuzz_configs.h | 8 +-
tests/qtest/libqos/generic-pcihost.h | 2 +-
.../libqos/{malloc.h => libqos-malloc.h} | 0
tests/qtest/libqos/libqos.h | 2 +-
tests/qtest/libqos/malloc-pc.h| 2 +-
tests/qtest/libqos/malloc-spapr.h | 2 +-
tests/qtest/libqos/pci-pc.h | 2 +-
tests/qtest/libqos/pci-spapr.h| 2 +-
tests/qtest/libqos/qgraph.h | 2 +-
tests/qtest/libqos/qos_external.h | 2 +-
tests/qtest/libqos/rtas.h | 2 +-
tests/qtest/libqos/virtio.h | 2 +-
tests/qtest/libqtest-single.h | 2 +-
tests/qtest/libqtest.h| 8 +
tests/qtest/migration-helpers.h | 2 +
accel/dummy-cpus.c| 14 +-
block.c
[PATCH 03/51] block: Unify the get_tmp_filename() implementation
From: Bin Meng
At present get_tmp_filename() has platform specific implementations
to get the directory to use for temporary files. Switch over to use
g_get_tmp_dir() which works on all supported platforms.
Signed-off-by: Bin Meng
---
block.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..d06df47f72 100644
--- a/block.c
+++ b/block.c
@@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
*/
int get_tmp_filename(char *filename, int size)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
int fd;
const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
+tmpdir = g_get_tmp_dir();
+
if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
return -EOVERFLOW;
}
@@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
return -errno;
}
return 0;
-#endif
}
/*
--
2.34.1
[PATCH 08/51] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir_with_parents() API and we can use
it to unify the codes without #ifdefs.
Signed-off-by: Bin Meng
---
block/vvfat.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..9c389ce5ea 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2726,13 +2726,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir_with_parents(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
[PATCH 38/51] tests/qtest: {ahci,ide}-test: Open file in binary mode
From: Xuzhou Cheng By default Windows opens file in text mode, while a POSIX compliant implementation treats text files and binary files the same. The fopen() 'mode' string can include the letter 'b' to indicate binary mode shall be used. POSIX spec says the character 'b' shall have no effect, but is allowed for ISO C standard conformance. Let's add the letter 'b' which works on both POSIX and Windows. Similar situation applies to the open() 'flags' where O_BINARY is used for binary mode. Signed-off-by: Xuzhou Cheng Signed-off-by: Bin Meng --- tests/qtest/ahci-test.c | 2 +- tests/qtest/ide-test.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c index bce9ff770c..be11508c75 100644 --- a/tests/qtest/ahci-test.c +++ b/tests/qtest/ahci-test.c @@ -1453,7 +1453,7 @@ static int prepare_iso(size_t size, unsigned char **buf, char **name) * Close the file and reopen it. */ close(fd); -fd = open(cdrom_path, O_WRONLY); +fd = open(cdrom_path, O_WRONLY | O_BINARY); g_assert(fd != -1); #endif diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index c5cad6c0be..ee03dea4fa 100644 --- a/tests/qtest/ide-test.c +++ b/tests/qtest/ide-test.c @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks) /* Prepopulate the CDROM with an interesting pattern */ generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh); g_assert_cmpint(ret, ==, patt_blocks); fclose(fh); @@ -993,7 +993,7 @@ static void test_cdrom_dma(void) prdt[0].size = cpu_to_le32(len | PRDT_EOT); generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh); g_assert_cmpint(ret, ==, 16); fclose(fh); -- 2.34.1
[PATCH 30/51] tests: Skip iotests and qtest when '--without-default-devices'
From: Bin Meng
When QEMU is configured with '--without-default-devices', we should
not build and run iotests and qtest because devices used by these
test cases are not built in.
Signed-off-by: Bin Meng
---
tests/qemu-iotests/meson.build | 5 +
tests/qtest/meson.build| 5 +
2 files changed, 10 insertions(+)
diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 323a4acb6a..38d9a874d2 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -2,6 +2,11 @@ if not have_tools or targetos == 'windows' or
get_option('gprof')
subdir_done()
endif
+# Skip iotests if configured without a default selection of devices
+if not get_option('default_devices')
+ subdir_done()
+endif
+
foreach cflag: config_host['QEMU_CFLAGS'].split()
if cflag.startswith('-fsanitize') and \
not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c97da5a062..0291b3966c 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -4,6 +4,11 @@ if not config_host.has_key('CONFIG_POSIX')
subdir_done()
endif
+# Skip QTests if configured without a default selection of devices
+if not get_option('default_devices')
+ subdir_done()
+endif
+
slow_qtests = {
'ahci-test' : 60,
'bios-tables-test' : 120,
--
2.34.1
[PATCH v5 0/2] block: add missed block_acct_setup with new block device init procedure
Commit 5f76a7aac156ca75680dad5df4a385fd0b58f6b1 is looking harmless from the first glance, but it has changed things a lot. 'libvirt' uses it to detect that it should follow new initialization way and this changes things considerably. With this procedure followed, blockdev_init() is not called anymore and thus block_acct_setup() helper is not called. This means in particular that defaults for block accounting statistics are changed and account_invalid/account_failed are actually initialized as false instead of true originally. This commit changes things to match original world. There are the following constraints: * new default value in block_acct_init() is set to true * block_acct_setup() inside blockdev_init() is called before blkconf_apply_backend_options() * thus newly created option in block device properties has precedence if specified Changes from v4: * removed hunk to QAPI which was used to test old initialization path * added R-b: Vladimir Changes from v3: * fixed accidentally wrong submission. Contains changes which should be sent as v3 Changes from v2: * called bool_from_onoffauto(account_..., true) in the first patch to preserve original semantics before patch 2 Changes from v1: * set account_invalid/account_failed to true by default * pass OnOffAuto to block_acct_init() to handle double initialization (patch 1) * changed properties on BLK device to OnOffAuto Signed-off-by: Denis V. Lunev CC: Peter Krempa CC: Markus Armbruster CC: John Snow CC: Kevin Wolf CC: Hanna Reitz CC: Vladimir Sementsov-Ogievskiy
[PATCH 1/2] block: pass OnOffAuto instead of bool to block_acct_setup()
We would have one more place for block_acct_setup() calling, which should
not corrupt original value.
Signed-off-by: Denis V. Lunev
Reviewed-by: Vladimir Sementsov-Ogievskiy
CC: Peter Krempa
CC: Markus Armbruster
CC: John Snow
CC: Kevin Wolf
CC: Hanna Reitz
---
block/accounting.c | 22 ++
blockdev.c | 17 ++---
include/block/accounting.h | 6 +++---
3 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/block/accounting.c b/block/accounting.c
index 2030851d79..6b300c5129 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -40,11 +40,25 @@ void block_acct_init(BlockAcctStats *stats)
}
}
-void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
- bool account_failed)
+static bool bool_from_onoffauto(OnOffAuto val, bool def)
{
-stats->account_invalid = account_invalid;
-stats->account_failed = account_failed;
+switch (val) {
+case ON_OFF_AUTO_AUTO:
+return def;
+case ON_OFF_AUTO_ON:
+return true;
+case ON_OFF_AUTO_OFF:
+return false;
+default:
+abort();
+}
+}
+
+void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
+ enum OnOffAuto account_failed)
+{
+stats->account_invalid = bool_from_onoffauto(account_invalid, true);
+stats->account_failed = bool_from_onoffauto(account_failed, true);
}
void block_acct_cleanup(BlockAcctStats *stats)
diff --git a/blockdev.c b/blockdev.c
index 9230888e34..392d9476e6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -455,6 +455,17 @@ static void extract_common_blockdev_options(QemuOpts
*opts, int *bdrv_flags,
}
}
+static OnOffAuto account_get_opt(QemuOpts *opts, const char *name)
+{
+if (!qemu_opt_find(opts, name)) {
+return ON_OFF_AUTO_AUTO;
+}
+if (qemu_opt_get_bool(opts, name, true)) {
+return ON_OFF_AUTO_ON;
+}
+return ON_OFF_AUTO_OFF;
+}
+
/* Takes the ownership of bs_opts */
static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
Error **errp)
@@ -462,7 +473,7 @@ static BlockBackend *blockdev_init(const char *file, QDict
*bs_opts,
const char *buf;
int bdrv_flags = 0;
int on_read_error, on_write_error;
-bool account_invalid, account_failed;
+OnOffAuto account_invalid, account_failed;
bool writethrough, read_only;
BlockBackend *blk;
BlockDriverState *bs;
@@ -496,8 +507,8 @@ static BlockBackend *blockdev_init(const char *file, QDict
*bs_opts,
/* extract parameters */
snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
-account_invalid = qemu_opt_get_bool(opts, "stats-account-invalid", true);
-account_failed = qemu_opt_get_bool(opts, "stats-account-failed", true);
+account_invalid = account_get_opt(opts, "stats-account-invalid");
+account_failed = account_get_opt(opts, "stats-account-failed");
writethrough = !qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true);
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..b9caad60d5 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -27,7 +27,7 @@
#include "qemu/timed-average.h"
#include "qemu/thread.h"
-#include "qapi/qapi-builtin-types.h"
+#include "qapi/qapi-types-common.h"
typedef struct BlockAcctTimedStats BlockAcctTimedStats;
typedef struct BlockAcctStats BlockAcctStats;
@@ -100,8 +100,8 @@ typedef struct BlockAcctCookie {
} BlockAcctCookie;
void block_acct_init(BlockAcctStats *stats);
-void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
- bool account_failed);
+void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
+ enum OnOffAuto account_failed);
void block_acct_cleanup(BlockAcctStats *stats);
void block_acct_add_interval(BlockAcctStats *stats, unsigned interval_length);
BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,
--
2.32.0
[PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
From: Bin Meng
On Windows, the MinGW provided mkstemp() API opens the file with
exclusive access, denying other processes to read/write the file.
Such behavior prevents the QEMU executable from opening the file,
(e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
This can be fixed by closing the file and reopening it.
Signed-off-by: Bin Meng
---
tests/qtest/ahci-test.c| 14 ++
tests/qtest/boot-serial-test.c | 13 +
2 files changed, 27 insertions(+)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f26cd6f86f..0e88cd0eef 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1443,6 +1443,20 @@ static int prepare_iso(size_t size, unsigned char **buf,
char **name)
int fd = mkstemp(cdrom_path);
g_assert(fd != -1);
+#ifdef _WIN32
+/*
+ * On Windows, the MinGW provided mkstemp() API opens the file with
+ * exclusive access, denying other processes to read/write the file.
+ * Such behavior prevents the QEMU executable from opening the file,
+ * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
+ *
+ * Close the file and reopen it.
+ */
+close(fd);
+fd = open(cdrom_path, O_WRONLY);
+g_assert(fd != -1);
+#endif
+
g_assert(buf);
g_assert(name);
patt = g_malloc(size);
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 404adcfa20..fb6c81bf35 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -235,6 +235,19 @@ static void test_machine(const void *data)
ser_fd = mkstemp(serialtmp);
g_assert(ser_fd != -1);
+#ifdef _WIN32
+/*
+ * On Windows, the MinGW provided mkstemp() API opens the file with
+ * exclusive access, denying other processes to read/write the file.
+ * Such behavior prevents the QEMU executable from opening the file,
+ * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
+ *
+ * Close the file and reopen it.
+ */
+close(ser_fd);
+ser_fd = open(serialtmp, O_RDONLY);
+g_assert(ser_fd != -1);
+#endif
if (test->kernel) {
code = test->kernel;
--
2.34.1
[PATCH 33/51] tests/qtest: {ahci, ide}-test: Use relative path for temporary files
From: Bin Meng
These test cases uses "blkdebug:path/to/config:path/to/image" for
testing. On Windows, absolute file paths contain the delimiter ':'
which causes the blkdebug filename parser fail to parse filenames.
Signed-off-by: Bin Meng
---
tests/qtest/ahci-test.c | 19 ---
tests/qtest/ide-test.c | 18 --
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 0e88cd0eef..bce9ff770c 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1848,7 +1848,7 @@ static void create_ahci_io_test(enum IOMode type, enum
AddrMode addr,
int main(int argc, char **argv)
{
-const char *arch;
+const char *arch, *base;
int ret;
int fd;
int c;
@@ -1886,8 +1886,21 @@ int main(int argc, char **argv)
return 0;
}
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create a temporary image */
-tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
@@ -1905,7 +1918,7 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", g_get_tmp_dir());
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
fd = mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index ebbf8e0126..c5cad6c0be 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -1011,17 +1011,31 @@ static void test_cdrom_dma(void)
int main(int argc, char **argv)
{
+const char *base;
int fd;
int ret;
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create temporary blkdebug instructions */
-debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", g_get_tmp_dir());
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
fd = mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
--
2.34.1
[PATCH 2/2] block: add missed block_acct_setup with new block device init procedure
Commit 5f76a7aac156ca75680dad5df4a385fd0b58f6b1 is looking harmless from
the first glance, but it has changed things a lot. 'libvirt' uses it to
detect that it should follow new initialization way and this changes
things considerably. With this procedure followed, blockdev_init() is
not called anymore and thus block_acct_setup() helper is not called.
This means in particular that defaults for block accounting statistics
are changed and account_invalid/account_failed are actually initialized
as false instead of true originally.
This commit changes things to match original world. There are the following
constraints:
* new default value in block_acct_init() is set to true
* block_acct_setup() inside blockdev_init() is called before
blkconf_apply_backend_options()
* thus newly created option in block device properties has precedence if
specified
Signed-off-by: Denis V. Lunev
Reviewed-by: Vladimir Sementsov-Ogievskiy
CC: Peter Krempa
CC: Markus Armbruster
CC: John Snow
CC: Kevin Wolf
CC: Hanna Reitz
---
block/accounting.c | 8 +++-
hw/block/block.c | 2 +
include/hw/block/block.h | 7 +++-
tests/qemu-iotests/172.out | 76 ++
4 files changed, 90 insertions(+), 3 deletions(-)
diff --git a/block/accounting.c b/block/accounting.c
index 6b300c5129..2829745377 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -38,6 +38,8 @@ void block_acct_init(BlockAcctStats *stats)
if (qtest_enabled()) {
clock_type = QEMU_CLOCK_VIRTUAL;
}
+stats->account_invalid = true;
+stats->account_failed = true;
}
static bool bool_from_onoffauto(OnOffAuto val, bool def)
@@ -57,8 +59,10 @@ static bool bool_from_onoffauto(OnOffAuto val, bool def)
void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
enum OnOffAuto account_failed)
{
-stats->account_invalid = bool_from_onoffauto(account_invalid, true);
-stats->account_failed = bool_from_onoffauto(account_failed, true);
+stats->account_invalid = bool_from_onoffauto(account_invalid,
+ stats->account_invalid);
+stats->account_failed = bool_from_onoffauto(account_failed,
+stats->account_failed);
}
void block_acct_cleanup(BlockAcctStats *stats)
diff --git a/hw/block/block.c b/hw/block/block.c
index 04279166ee..f9c4fe6767 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -205,6 +205,8 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool
readonly,
blk_set_enable_write_cache(blk, wce);
blk_set_on_error(blk, rerror, werror);
+block_acct_setup(blk_get_stats(blk), conf->account_invalid,
+ conf->account_failed);
return true;
}
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 5902c0440a..15fff66435 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -31,6 +31,7 @@ typedef struct BlockConf {
uint32_t lcyls, lheads, lsecs;
OnOffAuto wce;
bool share_rw;
+OnOffAuto account_invalid, account_failed;
BlockdevOnError rerror;
BlockdevOnError werror;
} BlockConf;
@@ -61,7 +62,11 @@ static inline unsigned int get_physical_block_exp(BlockConf
*conf)
_conf.discard_granularity, -1), \
DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
ON_OFF_AUTO_AUTO), \
-DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
+DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false),\
+DEFINE_PROP_ON_OFF_AUTO("account-invalid", _state, \
+_conf.account_invalid, ON_OFF_AUTO_AUTO), \
+DEFINE_PROP_ON_OFF_AUTO("account-failed", _state, \
+_conf.account_failed, ON_OFF_AUTO_AUTO)
#define DEFINE_BLOCK_PROPERTIES(_state, _conf) \
DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 9479b92185..07eebf3583 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -28,6 +28,8 @@ Formatting 'TEST_DIR/t.IMGFMT.3', fmt=IMGFMT size=737280
discard_granularity = 4294967295 (4 GiB)
write-cache = "auto"
share-rw = false
+account-invalid = "auto"
+account-failed = "auto"
drive-type = "288"
@@ -55,6 +57,8 @@ Testing: -fda TEST_DIR/t.qcow2
discard_granularity = 4294967295 (4 GiB)
write-cache = "auto"
share-rw = false
+account-invalid = "auto"
+account-failed = "auto"
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/devi
Re: [PATCH 4/4] hw/nvme: add MSI-x mask handlers for irqfd
On Aug 23 22:43, Jinhao Fan wrote: > On 8/16/2022 6:46 PM, Klaus Jensen wrote: > > Did qtest work out for you for testing? If so, it would be nice to add a > > simple test case as well. > > Since MSI-x masking handlers are only implemented for IO queues, if we want > to use qtest we need to implement utilities for controller initialization > and IO queue creation. After that we can actually test the MSI-x masking > feature. Although we may reuse some code from virtio's tests, that is still > a large amount of work. > > Is it possible to get this patch merged without testing? If not, I guess > I'll have to take the hard work to implement something like > qtest/libqos/nvme.c > I'm not too happy about code that is completely untestable (worse, right now it is actually not even runnable). What are the implications if we drop it? That is, if we go back to your version that did not include this? If it doesnt impact the kvm irqchip logic, then I'd rather that we rip it out and leave the device without masking/unmasking support, keeping irqfd support as an experimental feature until we can sort this out. signature.asc Description: PGP signature
Re: [PATCH 4/4] hw/nvme: add MSI-x mask handlers for irqfd
On 8/24/2022 7:22 PM, Klaus Jensen wrote: What are the implications if we drop it? That is, if we go back to your version that did not include this? If it doesnt impact the kvm irqchip logic, then I'd rather that we rip it out and leave the device without masking/unmasking support, keeping irqfd support as an experimental feature until we can sort this out. As far as I can think of, the implication is that MSI-X masking/unmasking does not work when irqfd and (in the future) iothread is enabled. Considering that we do not find any driver making use of this feature, it seems OK to drop this support for now.
Re: [PATCH 02/51] tests/qtest: Use g_mkdtemp()
On 24/08/2022 11.39, Bin Meng wrote: From: Bin Meng Windows does not provide a mkdtemp() API, but glib does. Replace mkdtemp() call with the glib version. Signed-off-by: Bin Meng --- tests/qtest/fuzz/generic_fuzz_configs.h | 2 +- tests/qtest/cdrom-test.c| 2 +- tests/qtest/cxl-test.c | 6 +++--- tests/qtest/ivshmem-test.c | 4 ++-- tests/qtest/libqos/virtio-9p.c | 4 ++-- tests/qtest/libqtest.c | 2 +- tests/qtest/migration-test.c| 4 ++-- tests/qtest/qmp-test.c | 4 ++-- tests/qtest/vhost-user-test.c | 4 ++-- tests/unit/test-qga.c | 2 +- 10 files changed, 17 insertions(+), 17 deletions(-) Reviewed-by: Thomas Huth
[PATCH v1 1/5] virtio-blk: decouple config size determination code from VirtIOBlock
Make it more stand-alone so that we can reuse it for other virtio-blk
devices that are not VirtIOBlock in the future commits.
Signed-off-by: Daniil Tatianin
---
hw/block/virtio-blk.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e9ba752f6b..a4162dbbf2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -49,12 +49,13 @@ static const VirtIOFeature feature_sizes[] = {
{}
};
-static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
+static size_t virtio_blk_common_get_config_size(uint64_t host_features)
{
-s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
+size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
virtio_feature_get_config_size(feature_sizes, host_features));
-assert(s->config_size <= sizeof(struct virtio_blk_config));
+assert(config_size <= sizeof(struct virtio_blk_config));
+return config_size;
}
static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
@@ -1204,7 +1205,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
-virtio_blk_set_config_size(s, s->host_features);
+s->config_size = virtio_blk_common_get_config_size(s->host_features);
virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
--
2.25.1
[PATCH v1 4/5] vhost-user-blk: make 'config_wce' part of 'host_features'
No reason to have this be a separate field. This also makes it more akin
to what the virtio-blk device does.
Signed-off-by: Daniil Tatianin
---
hw/block/vhost-user-blk.c | 6 ++
include/hw/virtio/vhost-user-blk.h | 1 -
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index e89164c358..64f3457373 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -262,9 +262,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
*vdev,
virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
virtio_add_feature(&features, VIRTIO_BLK_F_RO);
-if (s->config_wce) {
-virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-}
if (s->num_queues > 1) {
virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
}
@@ -591,7 +588,8 @@ static Property vhost_user_blk_properties[] = {
DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
VHOST_USER_BLK_AUTO_NUM_QUEUES),
DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
-DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
+DEFINE_PROP_BIT64("config-wce", VHostUserBlk, host_features,
+ VIRTIO_BLK_F_CONFIG_WCE, true),
DEFINE_PROP_BIT64("discard", VHostUserBlk, host_features,
VIRTIO_BLK_F_DISCARD, true),
DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, host_features,
diff --git a/include/hw/virtio/vhost-user-blk.h
b/include/hw/virtio/vhost-user-blk.h
index 20573dd586..6252095c45 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -34,7 +34,6 @@ struct VHostUserBlk {
struct virtio_blk_config blkcfg;
uint16_t num_queues;
uint32_t queue_size;
-uint32_t config_wce;
struct vhost_dev dev;
struct vhost_inflight *inflight;
VhostUserState vhost_user;
--
2.25.1
[PATCH v1 0/5] vhost-user-blk: dynamically resize config space based on features
This patch set attempts to align vhost-user-blk with virtio-blk in terms of backward compatibility and flexibility. In particular it adds the following things: - Ability to disable modern features like discard/write-zeroes. - Dynamic configuration space resizing based on enabled features, by reusing the code, which was already present in virtio-blk. - Makes the VHostUserBlk structure a bit less clunky by using the 'host_features' field to represent enabled features, as opposed to using a separate field per feature. This was already done for virtio-blk a long time ago. Daniil Tatianin (5): virtio-blk: decouple config size determination code from VirtIOBlock virtio-blk: move config space sizing code to virtio-blk-common vhost-user-blk: make it possible to disable write-zeroes/discard vhost-user-blk: make 'config_wce' part of 'host_features' vhost-user-blk: dynamically resize config space based on features MAINTAINERS | 4 +++ hw/block/meson.build | 4 +-- hw/block/vhost-user-blk.c | 29 +- hw/block/virtio-blk-common.c | 42 +++ hw/block/virtio-blk.c | 25 ++-- include/hw/virtio/vhost-user-blk.h| 4 ++- include/hw/virtio/virtio-blk-common.h | 21 ++ 7 files changed, 90 insertions(+), 39 deletions(-) create mode 100644 hw/block/virtio-blk-common.c create mode 100644 include/hw/virtio/virtio-blk-common.h -- 2.25.1
[PATCH v1 5/5] vhost-user-blk: dynamically resize config space based on features
Make vhost-user-blk backwards compatible when migrating from older VMs
running with modern features turned off, the same way it was done for
virtio-blk in 20764be0421c ("virtio-blk: set config size depending on the
features enabled")
It's currently impossible to migrate from an older VM with
vhost-user-blk (with disable-legacy=off) because of errors like this:
qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: 41
device: 1 cmask: ff wmask: 80 w1cmask:0
qemu-system-x86_64: Failed to load PCIDevice:config
qemu-system-x86_64: Failed to load virtio-blk:virtio
qemu-system-x86_64: error while loading state for instance 0x0 of device
':00:05.0:00.0:02.0/virtio-blk'
qemu-system-x86_64: load of migration failed: Invalid argument
This is caused by the newer (destination) VM requiring a bigger BAR0
alignment because it has to cover a bigger configuration space, which
isn't actually needed since those additional config fields are not
active (write-zeroes/discard).
Signed-off-by: Daniil Tatianin
---
hw/block/vhost-user-blk.c | 15 ---
include/hw/virtio/vhost-user-blk.h | 1 +
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 64f3457373..d18a7a2cd4 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -23,6 +23,7 @@
#include "hw/qdev-core.h"
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
+#include "hw/virtio/virtio-blk-common.h"
#include "hw/virtio/vhost.h"
#include "hw/virtio/vhost-user-blk.h"
#include "hw/virtio/virtio.h"
@@ -63,7 +64,7 @@ static void vhost_user_blk_update_config(VirtIODevice *vdev,
uint8_t *config)
/* Our num_queues overrides the device backend */
virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);
-memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
+memcpy(config, &s->blkcfg, s->config_size);
}
static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t
*config)
@@ -96,8 +97,7 @@ static int vhost_user_blk_handle_config_change(struct
vhost_dev *dev)
Error *local_err = NULL;
ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
- sizeof(struct virtio_blk_config),
- &local_err);
+ s->config_size, &local_err);
if (ret < 0) {
error_report_err(local_err);
return ret;
@@ -106,7 +106,7 @@ static int vhost_user_blk_handle_config_change(struct
vhost_dev *dev)
/* valid for resize only */
if (blkcfg.capacity != s->blkcfg.capacity) {
s->blkcfg.capacity = blkcfg.capacity;
-memcpy(dev->vdev->config, &s->blkcfg, sizeof(struct
virtio_blk_config));
+memcpy(dev->vdev->config, &s->blkcfg, s->config_size);
virtio_notify_config(dev->vdev);
}
@@ -444,7 +444,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s,
Error **errp)
assert(s->connected);
ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
- sizeof(struct virtio_blk_config), errp);
+ s->config_size, errp);
if (ret < 0) {
qemu_chr_fe_disconnect(&s->chardev);
vhost_dev_cleanup(&s->dev);
@@ -489,8 +489,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
-virtio_init(vdev, VIRTIO_ID_BLOCK,
-sizeof(struct virtio_blk_config));
+s->config_size = virtio_blk_common_get_config_size(s->host_features);
+
+virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
s->virtqs = g_new(VirtQueue *, s->num_queues);
for (i = 0; i < s->num_queues; i++) {
diff --git a/include/hw/virtio/vhost-user-blk.h
b/include/hw/virtio/vhost-user-blk.h
index 6252095c45..b7810360b9 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -52,6 +52,7 @@ struct VHostUserBlk {
bool started_vu;
uint64_t host_features;
+size_t config_size;
};
#endif
--
2.25.1
[PATCH v1 3/5] vhost-user-blk: make it possible to disable write-zeroes/discard
It is useful to have the ability to disable these features for
compatibility with older VMs that don't have these implemented.
Signed-off-by: Daniil Tatianin
---
hw/block/vhost-user-blk.c | 8 ++--
include/hw/virtio/vhost-user-blk.h | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..e89164c358 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
*vdev,
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
+features |= s->host_features;
+
/* Turn on pre-defined features */
virtio_add_feature(&features, VIRTIO_BLK_F_SIZE_MAX);
virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX);
@@ -259,8 +261,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
*vdev,
virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
virtio_add_feature(&features, VIRTIO_BLK_F_RO);
-virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
-virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
if (s->config_wce) {
virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
@@ -592,6 +592,10 @@ static Property vhost_user_blk_properties[] = {
VHOST_USER_BLK_AUTO_NUM_QUEUES),
DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
+DEFINE_PROP_BIT64("discard", VHostUserBlk, host_features,
+ VIRTIO_BLK_F_DISCARD, true),
+DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, host_features,
+ VIRTIO_BLK_F_WRITE_ZEROES, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/virtio/vhost-user-blk.h
b/include/hw/virtio/vhost-user-blk.h
index 7c91f15040..20573dd586 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -51,6 +51,8 @@ struct VHostUserBlk {
bool connected;
/* vhost_user_blk_start/vhost_user_blk_stop */
bool started_vu;
+
+uint64_t host_features;
};
#endif
--
2.25.1
Re: EBUSY when using NVMe Block Driver with multiple devices in the same IOMMU group
> On Aug 24, 2022, at 4:36 PM, Martin Oliveira > wrote: >-drive file=nvme://:26:00.0,if=none,id=drive0,format=raw >-device virtio-blk,drive=drive0,id=virtio0,serial=nvme0 Small typo above, I missed the /1, it should read: -drive file=nvme://:26:00.0/1,if=none,id=drive0,format=raw -device virtio-blk,drive=drive0,id=virtio0,serial=nvme0 Martin
Re: EBUSY when using NVMe Block Driver with multiple devices in the same IOMMU group
On Tue, Aug 23, 2022 at 10:36:00PM +, Martin Oliveira wrote: > Hello, > > I'm trying to use the QEMU NVMe userspace driver and I'm hitting an error > when trying to use more than one device from an IOMMU group: > > Failed to open VFIO group file: /dev/vfio/39: Device or resource busy > > If devices belong to different IOMMU groups, then it works as expected. > > For each device, I bind it to vfio-pci and then use something like this: > > -drive file=nvme://:26:00.0,if=none,id=drive0,format=raw > -device virtio-blk,drive=drive0,id=virtio0,serial=nvme0 > > Using the file-based protocol (file=/dev/nvme0n1) works with multiple devices > from the same group. > > My host is running a 5.19 kernel and QEMU is the latest upstream > (a8cc5842b5cb). First, multiple QEMU instances cannot access nvme:// devices sharing the same IOMMU group. I don't think this will ever be possible because it opens a backdoor around process memory isolation. However, a single QEMU (or qemu-storage-daemon) instance should be able to access multiple nvme:// devices in the same IOMMU group. Unfortunately the code currently doesn't support that. util/vfio-helpers.c:qemu_vfio_init_pci() has no logic for sharing groups/containers. Opening the group fails with EBUSY because the kernel only allows the file to be opened once at any given time. It's possible to extend the util/vfio-helpers.c code to reuse VFIO groups (and share VFIO containers), but I'm not aware of anyone who is currently working on that. Stefan signature.asc Description: PGP signature
Re: [PATCH v1 4/5] vhost-user-blk: make 'config_wce' part of 'host_features'
On Wed, Aug 24, 2022 at 12:18:36PM +0300, Daniil Tatianin wrote:
> @@ -591,7 +588,8 @@ static Property vhost_user_blk_properties[] = {
> DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
> VHOST_USER_BLK_AUTO_NUM_QUEUES),
> DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
> -DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
> +DEFINE_PROP_BIT64("config-wce", VHostUserBlk, host_features,
> + VIRTIO_BLK_F_CONFIG_WCE, true),
Please use parent_obj.host_features instead of a VHostUserBlk
host_features field.
signature.asc
Description: PGP signature
Re: [PATCH 07/51] tests: Avoid using hardcoded /tmp in test cases
* Bin Meng (bmeng...@gmail.com) wrote:
> From: Bin Meng
>
> Use g_get_tmp_dir() to get the directory to use for temporary files.
>
> Signed-off-by: Bin Meng
> ---
>
> tests/qtest/fuzz/generic_fuzz_configs.h | 6 --
> tests/qtest/ahci-test.c | 15 +++
> tests/qtest/aspeed_smc-test.c | 4 +++-
> tests/qtest/boot-serial-test.c | 8 ++--
> tests/qtest/cxl-test.c | 9 ++---
> tests/qtest/fdc-test.c | 4 +++-
> tests/qtest/fuzz/virtio_blk_fuzz.c | 2 +-
> tests/qtest/hd-geo-test.c | 8
> tests/qtest/ide-test.c | 8 ++--
> tests/qtest/libqtest.c | 10 +++---
> tests/qtest/migration-test.c| 4 +++-
> tests/qtest/pflash-cfi02-test.c | 7 +--
> tests/qtest/qmp-test.c | 4 +++-
> tests/qtest/vhost-user-blk-test.c | 3 ++-
> tests/qtest/vhost-user-test.c | 3 ++-
> tests/qtest/virtio-blk-test.c | 2 +-
> tests/qtest/virtio-scsi-test.c | 3 ++-
> tests/unit/test-image-locking.c | 6 --
> tests/unit/test-qga.c | 2 +-
> tests/vhost-user-bridge.c | 3 ++-
> 20 files changed, 76 insertions(+), 35 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> b/tests/qtest/fuzz/generic_fuzz_configs.h
> index 0775e6702b..d0f9961187 100644
> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> @@ -20,13 +20,15 @@ typedef struct generic_fuzz_config {
> } generic_fuzz_config;
>
> static inline gchar *generic_fuzzer_virtio_9p_args(void){
> -char tmpdir[] = "/tmp/qemu-fuzz.XX";
> +char *tmpdir = g_strdup_printf("%s/qemu-fuzz.XX", g_get_tmp_dir());
You might find it easier to use g_autofree in a lot of these, and then
you don't need to bother with the g_free at the end.
Dave
> g_assert_nonnull(g_mkdtemp(tmpdir));
>
> -return g_strdup_printf("-machine q35 -nodefaults "
> +gchar *args = g_strdup_printf("-machine q35 -nodefaults "
> "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> "-fsdev local,id=hshare,path=%s,security_model=mapped-xattr,"
> "writeout=immediate,fmode=0600,dmode=0700", tmpdir);
> +g_free(tmpdir);
> +return args;
> }
>
> const generic_fuzz_config predefined_configs[] = {
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index f1e510b0ac..f26cd6f86f 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -44,9 +44,9 @@
> #define TEST_IMAGE_SIZE_MB_SMALL 64
>
> /*** Globals ***/
> -static char tmp_path[] = "/tmp/qtest.XX";
> -static char debug_path[] = "/tmp/qtest-blkdebug.XX";
> -static char mig_socket[] = "/tmp/qtest-migration.XX";
> +static char *tmp_path;
> +static char *debug_path;
> +static char *mig_socket;
> static bool ahci_pedantic;
> static const char *imgfmt;
> static unsigned test_image_size_mb;
> @@ -1437,7 +1437,7 @@ static void test_ncq_simple(void)
>
> static int prepare_iso(size_t size, unsigned char **buf, char **name)
> {
> -char cdrom_path[] = "/tmp/qtest.iso.XX";
> +char *cdrom_path = g_strdup_printf("%s/qtest.iso.XX",
> g_get_tmp_dir());
> unsigned char *patt;
> ssize_t ret;
> int fd = mkstemp(cdrom_path);
> @@ -1454,6 +1454,7 @@ static int prepare_iso(size_t size, unsigned char
> **buf, char **name)
>
> *name = g_strdup(cdrom_path);
> *buf = patt;
> +g_free(cdrom_path);
> return fd;
> }
>
> @@ -1872,6 +1873,7 @@ int main(int argc, char **argv)
> }
>
> /* Create a temporary image */
> +tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
> fd = mkstemp(tmp_path);
> g_assert(fd >= 0);
> if (have_qemu_img()) {
> @@ -1889,11 +1891,13 @@ int main(int argc, char **argv)
> close(fd);
>
> /* Create temporary blkdebug instructions */
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
> g_get_tmp_dir());
> fd = mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
>
> /* Reserve a hollow file to use as a socket for migration tests */
> +mig_socket = g_strdup_printf("%s/qtest-migration.XX",
> g_get_tmp_dir());
> fd = mkstemp(mig_socket);
> g_assert(fd >= 0);
> close(fd);
> @@ -1947,8 +1951,11 @@ int main(int argc, char **argv)
>
> /* Cleanup */
> unlink(tmp_path);
> +g_free(tmp_path);
> unlink(debug_path);
> +g_free(debug_path);
> unlink(mig_socket);
> +g_free(mig_socket);
>
> return ret;
> }
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index 05ce941566..cab769459c 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -608,7 +608,7 @@ static void test_write_block_protect_bottom_bit(void)
> flash_reset();
> }
>
> -static char tmp_path[] = "/
Re: [PATCH v1 3/5] vhost-user-blk: make it possible to disable write-zeroes/discard
On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote:
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..e89164c358 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>
> +features |= s->host_features;
I think you can eliminate this if you use vdev->host_features in the
qdev properties instead of adding a separate s->host_features field.
That will simplify the code.
signature.asc
Description: PGP signature
Re: [PATCH v1 2/5] virtio-blk: move config space sizing code to virtio-blk-common
On Wed, Aug 24, 2022 at 12:18:34PM +0300, Daniil Tatianin wrote:
> +size_t virtio_blk_common_get_config_size(uint64_t host_features)
> +{
> +size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
> +virtio_feature_get_config_size(feature_sizes, host_features));
> +
> +assert(config_size <= sizeof(struct virtio_blk_config));
> +return config_size;
> +}
This logic is common to all VIRTIO devices and I think it can be moved
to virtio_feature_get_config_size(). Then
virtio_blk_common_get_config_size() is no longer necessary and the
generic virtio_feature_get_config_size() can be called directly.
The only virtio-blk common part would be the
virtio_feature_get_config_size() parameter struct that describes the
minimum and maximum config space size, as well as how the feature bits
affect the size:
size = virtio_feature_get_config_size(virtio_blk_config_size_params,
host_features)
where virtio_blk_config_size_params is:
const VirtIOConfigSizeParams virtio_blk_config_size_params = {
.min_size = offsetof(struct virtio_blk_config, max_discard_sectors),
.max_size = sizeof(struct virtio_blk_config),
.features = {
{.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
.end = endof(struct virtio_blk_config, discard_sector_alignment)},
...,
},
};
Then virtio-blk-common.h just needs to define:
extern const VirtIOConfigSizeParams virtio_blk_config_size_params;
Taking it one step further, maybe VirtioDeviceClass should include a
const VirtIOConfigSizeParams *config_size_params field so
vdev->config_size can be computed by common VIRTIO code and the devices
only need to describe the parameters.
Stefan
signature.asc
Description: PGP signature
Re: [PATCH v1 5/5] vhost-user-blk: dynamically resize config space based on features
On Wed, Aug 24, 2022 at 12:18:37PM +0300, Daniil Tatianin wrote:
> Make vhost-user-blk backwards compatible when migrating from older VMs
> running with modern features turned off, the same way it was done for
> virtio-blk in 20764be0421c ("virtio-blk: set config size depending on the
> features enabled")
>
> It's currently impossible to migrate from an older VM with
> vhost-user-blk (with disable-legacy=off) because of errors like this:
>
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: 41
> device: 1 cmask: ff wmask: 80 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load virtio-blk:virtio
> qemu-system-x86_64: error while loading state for instance 0x0 of device
> ':00:05.0:00.0:02.0/virtio-blk'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> This is caused by the newer (destination) VM requiring a bigger BAR0
> alignment because it has to cover a bigger configuration space, which
> isn't actually needed since those additional config fields are not
> active (write-zeroes/discard).
>
> Signed-off-by: Daniil Tatianin
> ---
> hw/block/vhost-user-blk.c | 15 ---
> include/hw/virtio/vhost-user-blk.h | 1 +
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 64f3457373..d18a7a2cd4 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -23,6 +23,7 @@
> #include "hw/qdev-core.h"
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> +#include "hw/virtio/virtio-blk-common.h"
> #include "hw/virtio/vhost.h"
> #include "hw/virtio/vhost-user-blk.h"
> #include "hw/virtio/virtio.h"
> @@ -63,7 +64,7 @@ static void vhost_user_blk_update_config(VirtIODevice
> *vdev, uint8_t *config)
> /* Our num_queues overrides the device backend */
> virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);
>
> -memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
> +memcpy(config, &s->blkcfg, s->config_size);
Please drop the s->config_size field introduced in this patch and use
vdev->config_len instead. When the same value is stored in multiple
places it's hard to be sure each copy remains identical and bugs can
creep in.
For example, if vdev->config_len is used consistently then it's clear
that buffer overflows and information leaks are prevented by common
code:
uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
uint8_t val;
if (addr + sizeof(val) > vdev->config_len) {
return (uint32_t)-1;
}
k->get_config(vdev, vdev->config);
It's safe because vdev->config is g_malloc0(vdev->config_len).
Buf if I see s->config_size, I don't really know whether it's safe and I
need to audit the code to be sure.
signature.asc
Description: PGP signature
Re: [PATCH v1 3/5] vhost-user-blk: make it possible to disable write-zeroes/discard
On 8/24/22 9:00 PM, Stefan Hajnoczi wrote:
On Wed, Aug 24, 2022 at 12:18:35PM +0300, Daniil Tatianin wrote:
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..e89164c358 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -251,6 +251,8 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
*vdev,
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
+features |= s->host_features;
I think you can eliminate this if you use vdev->host_features in the
qdev properties instead of adding a separate s->host_features field.
That will simplify the code.
Indeed, thanks for spotting that. I wonder why every virtio device
implementation I've looked at has chosen to add their own host_features
field (net/blk)?
Re: [PATCH v1 2/5] virtio-blk: move config space sizing code to virtio-blk-common
On 8/24/22 9:13 PM, Stefan Hajnoczi wrote:
On Wed, Aug 24, 2022 at 12:18:34PM +0300, Daniil Tatianin wrote:
+size_t virtio_blk_common_get_config_size(uint64_t host_features)
+{
+size_t config_size = MAX(VIRTIO_BLK_CFG_SIZE,
+virtio_feature_get_config_size(feature_sizes, host_features));
+
+assert(config_size <= sizeof(struct virtio_blk_config));
+return config_size;
+}
This logic is common to all VIRTIO devices and I think it can be moved
to virtio_feature_get_config_size(). Then
virtio_blk_common_get_config_size() is no longer necessary and the
generic virtio_feature_get_config_size() can be called directly.
The only virtio-blk common part would be the
virtio_feature_get_config_size() parameter struct that describes the
minimum and maximum config space size, as well as how the feature bits
affect the size:
size = virtio_feature_get_config_size(virtio_blk_config_size_params,
host_features)
where virtio_blk_config_size_params is:
const VirtIOConfigSizeParams virtio_blk_config_size_params = {
.min_size = offsetof(struct virtio_blk_config, max_discard_sectors),
.max_size = sizeof(struct virtio_blk_config),
.features = {
{.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
.end = endof(struct virtio_blk_config, discard_sector_alignment)},
...,
},
};
Then virtio-blk-common.h just needs to define:
extern const VirtIOConfigSizeParams virtio_blk_config_size_params;
Taking it one step further, maybe VirtioDeviceClass should include a
const VirtIOConfigSizeParams *config_size_params field so
vdev->config_size can be computed by common VIRTIO code and the devices
only need to describe the parameters.
I think that's a great idea! Do you think it should be done
automatically in 'virtio_init' if this field is not NULL? One problem I
see with that is that you would have to make all virtio devices use
'parent_obj.host_features' for feature properties, which is currently
far from true, but then again this is very much opt-in. Another thing
you could do is add a separate helper for that, which maybe defeats the
purpose a little bit?
Stefan
Re: [PATCH v7 4/8] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls
On 2022/08/22 21:12, Sam Li wrote:
> Stefan Hajnoczi 于2022年8月23日周二 08:49写道：
>>
>> On Tue, Aug 16, 2022 at 02:25:18PM +0800, Sam Li wrote:
>>> By adding zone management operations in BlockDriver, storage controller
>>> emulation can use the new block layer APIs including Report Zone and
>>> four zone management operations (open, close, finish, reset).
>>>
>>> Add zoned storage commands of the device: zone_report(zrp), zone_open(zo),
>>> zone_close(zc), zone_reset(zrs), zone_finish(zf).
>>>
>>> For example, to test zone_report, use following command:
>>> $ ./build/qemu-io --image-opts driver=zoned_host_device,
>>> filename=/dev/nullb0
>>> -c "zrp offset nr_zones"
>>>
>>> Signed-off-by: Sam Li
>>> Reviewed-by: Hannes Reinecke
>>> ---
>>> block/block-backend.c | 50 +
>>> block/file-posix.c| 341 +-
>>> block/io.c| 41
>>> include/block/block-common.h | 1 -
>>> include/block/block-io.h | 13 ++
>>> include/block/block_int-common.h | 22 +-
>>> include/block/raw-aio.h | 6 +-
>>> include/sysemu/block-backend-io.h | 6 +
>>> meson.build | 1 +
>>> qapi/block-core.json | 8 +-
>>> qemu-io-cmds.c| 143 +
>>> 11 files changed, 625 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index d4a5df2ac2..fc639b0cd7 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>> @@ -1775,6 +1775,56 @@ int coroutine_fn blk_co_flush(BlockBackend *blk)
>>> return ret;
>>> }
>>>
>>> +/*
>>> + * Send a zone_report command.
>>> + * offset is a byte offset from the start of the device. No alignment
>>> + * required for offset.
>>> + * nr_zones represents IN maximum and OUT actual.
>>> + */
>>> +int coroutine_fn blk_co_zone_report(BlockBackend *blk, int64_t offset,
>>> +unsigned int *nr_zones,
>>> +BlockZoneDescriptor *zones)
>>> +{
>>> +int ret;
>>> +IO_CODE();
>>> +
>>> +blk_inc_in_flight(blk); /* increase before waiting */
>>> +blk_wait_while_drained(blk);
>>> +if (!blk_is_available(blk)) {
>>> +blk_dec_in_flight(blk);
>>> +return -ENOMEDIUM;
>>> +}
>>> +ret = bdrv_co_zone_report(blk_bs(blk), offset, nr_zones, zones);
>>> +blk_dec_in_flight(blk);
>>> +return ret;
>>> +}
>>> +
>>> +/*
>>> + * Send a zone_management command.
>>> + * offset is the starting zone specified as a sector offset.
>>> + * len is the maximum number of sectors the command should operate on.
>>> + */
>>> +int coroutine_fn blk_co_zone_mgmt(BlockBackend *blk, BlockZoneOp op,
>>> +int64_t offset, int64_t len)
>>> +{
>>> +int ret;
>>> +IO_CODE();
>>> +
>>> +ret = blk_check_byte_request(blk, offset, len);
>>> +if (ret < 0) {
>>> +return ret;
>>> +}
>>
>> blk_check_byte_request() calls blk_is_available() and returns -ENOMEDIUM
>> when it fails. You can therefore move this down and replace "if
>> (!blk_is_available(blk)) {".
>>
>>> +blk_inc_in_flight(blk);
>>> +blk_wait_while_drained(blk);
>>> +if (!blk_is_available(blk)) {
>>> +blk_dec_in_flight(blk);
>>> +return -ENOMEDIUM;
>>> +}
>>> +ret = bdrv_co_zone_mgmt(blk_bs(blk), op, offset, len);
>>> +blk_dec_in_flight(blk);
>>> +return ret;
>>> +}
>>> +
>>> void blk_drain(BlockBackend *blk)
>>> {
>>> BlockDriverState *bs = blk_bs(blk);
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index 727389488c..29f67082d9 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -67,6 +67,9 @@
>>> #include
>>> #include
>>> #include
>>> +#if defined(CONFIG_BLKZONED)
>>> +#include
>>> +#endif
>>> #include
>>> #include
>>> #include
>>> @@ -216,6 +219,13 @@ typedef struct RawPosixAIOData {
>>> PreallocMode prealloc;
>>> Error **errp;
>>> } truncate;
>>> +struct {
>>> +unsigned int *nr_zones;
>>> +BlockZoneDescriptor *zones;
>>> +} zone_report;
>>> +struct {
>>> +unsigned long ioctl_op;
>>> +} zone_mgmt;
>>> };
>>> } RawPosixAIOData;
>>>
>>> @@ -1328,7 +1338,7 @@ static void raw_refresh_limits(BlockDriverState *bs,
>>> Error **errp)
>>> #endif
>>>
>>> if (bs->sg || S_ISBLK(st.st_mode)) {
>>> -int ret = hdev_get_max_hw_transfer(s->fd, &st);
>>> +ret = hdev_get_max_hw_transfer(s->fd, &st);
>>>
>>> if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
>>> bs->bl.max_hw_transfer = ret;
>>> @@ -1340,11 +1350,32 @@ static void raw_refresh_limits(BlockDriverState
>>> *bs, Error **errp)
>>> }
>>> }
>>>
>>> -ret = get_sysfs_zoned_model(s->fd, &st, &zoned);
>>> +ret = get_sysfs_zoned_model(&st, &zoned);
>>> if (ret < 0) {
>>> zoned = BLK_Z_NONE;
>>>
Re: [PATCH v7 4/8] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls
On 2022/08/24 16:46, Damien Le Moal wrote:
> On 2022/08/22 21:12, Sam Li wrote:
>> Stefan Hajnoczi 于2022年8月23日周二 08:49写道：
>>>
>>> On Tue, Aug 16, 2022 at 02:25:18PM +0800, Sam Li wrote:
[...] +blkz = (struct blk_zone *)(rep + 1);
+while (n < nrz) {
+memset(rep, 0, rep_size);
+rep->sector = sector;
+rep->nr_zones = nrz - n;
+
+ret = ioctl(fd, BLKREPORTZONE, rep);
>>>
>>> Does this ioctl() need "do { ... } while (ret == -1 && errno == EINTR)"?
>>
>> No? We discussed this before. I guess even EINTR should be propagated
>> back to the guest. Maybe Damien can talk more about why.
>
> In the kernel, completion of zone management IO requests are waited for using
> wait_for_completion_io() which uses TASK_UNINTERRUPTIBLE. So a signal will not
> abort anything. So I do not think that the do { } while() loop is necessary.
Note: I do not think the loop to be necessary, but it will not hurt :)
--
Damien Le Moal
Western Digital Research
[PATCH] hw/nvme: set DNR on compare failure
From: Klaus Jensen
Even if the host is somehow using compare to do compare-and-write, the
host should be notified immediately about the compare failure and not
have to wait for the driver to potentially retry the command.
Reported-by: Jim Harris
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 87aeba056499..299cbb1f2205 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2242,7 +2242,7 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
for (bufp = buf; mbufp < end; bufp += ns->lbaf.ms, mbufp +=
ns->lbaf.ms) {
if (memcmp(bufp + pil, mbufp + pil, ns->lbaf.ms - pil)) {
-req->status = NVME_CMP_FAILURE;
+req->status = NVME_CMP_FAILURE | NVME_DNR;
goto out;
}
}
@@ -2251,7 +2251,7 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
}
if (memcmp(buf, ctx->mdata.bounce, ctx->mdata.iov.size)) {
-req->status = NVME_CMP_FAILURE;
+req->status = NVME_CMP_FAILURE | NVME_DNR;
goto out;
}
@@ -2300,7 +2300,7 @@ static void nvme_compare_data_cb(void *opaque, int ret)
}
if (memcmp(buf, ctx->data.bounce, ctx->data.iov.size)) {
-req->status = NVME_CMP_FAILURE;
+req->status = NVME_CMP_FAILURE | NVME_DNR;
goto out;
}
--
2.37.2
