From 064f8ee7aa6e7734f12cc1f3fbbcf61949006e0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:55:54 +0200 Subject: [PATCH 01/38] meson.build: Add comments to clarify code organization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-2-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- meson.build | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/meson.build b/meson.build index 17c89c87c6..68eaf8ce2f 100644 --- a/meson.build +++ b/meson.build @@ -1446,6 +1446,10 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', capture: true, command: [undefsym, nm, '@INPUT@']) +######################## +# Library dependencies # +######################## + block_ss = block_ss.apply(config_host, strict: false) libblock = static_library('block', block_ss.sources() + genh, dependencies: block_ss.dependencies(), @@ -1465,6 +1469,10 @@ libqmp = static_library('qmp', qmp_ss.sources() + genh, qmp = declare_dependency(link_whole: [libqmp]) +########### +# Targets # +########### + foreach m : block_mods + softmmu_mods shared_module(m.name(), name_prefix: '', From 7e2b888f62bbf0def0978448b27c1141e15359d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:55:55 +0200 Subject: [PATCH 02/38] meson.build: Sort sourcesets alphabetically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-3-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- meson.build | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/meson.build b/meson.build index 68eaf8ce2f..adcfb494a8 100644 --- a/meson.build +++ b/meson.build @@ -1182,19 +1182,19 @@ sphinx_extn_depends = [ meson.source_root() / 'docs/sphinx/depfile.py', # Collect sourcesets. -util_ss = ss.source_set() +blockdev_ss = ss.source_set() +block_ss = ss.source_set() +bsd_user_ss = ss.source_set() +common_ss = ss.source_set() +linux_user_ss = ss.source_set() +qmp_ss = ss.source_set() +softmmu_ss = ss.source_set() +specific_fuzz_ss = ss.source_set() +specific_ss = ss.source_set() stub_ss = ss.source_set() trace_ss = ss.source_set() -block_ss = ss.source_set() -blockdev_ss = ss.source_set() -qmp_ss = ss.source_set() -common_ss = ss.source_set() -softmmu_ss = ss.source_set() user_ss = ss.source_set() -bsd_user_ss = ss.source_set() -linux_user_ss = ss.source_set() -specific_ss = ss.source_set() -specific_fuzz_ss = ss.source_set() +util_ss = ss.source_set() modules = {} hw_arch = {} From e28ab096bf897310fc7a37fdc087e66a24e6e977 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:55:56 +0200 Subject: [PATCH 03/38] hw/core: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-4-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- hw/core/meson.build | 6 ------ meson.build | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/hw/core/meson.build b/hw/core/meson.build index fc91f98075..4a744f3b5e 100644 --- a/hw/core/meson.build +++ b/hw/core/meson.build @@ -14,12 +14,6 @@ hwcore_files = files( 'qdev-clock.c', ) -libhwcore = static_library('hwcore', sources: hwcore_files + genh, - name_suffix: 'fa', - build_by_default: false) -hwcore = declare_dependency(link_whole: libhwcore) -common_ss.add(hwcore) - common_ss.add(files('cpu.c')) common_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c')) common_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: files('generic-loader.c')) diff --git a/meson.build b/meson.build index adcfb494a8..6c760b4163 100644 --- a/meson.build +++ b/meson.build @@ -1469,6 +1469,12 @@ libqmp = static_library('qmp', qmp_ss.sources() + genh, qmp = declare_dependency(link_whole: [libqmp]) +libhwcore = static_library('hwcore', sources: hwcore_files + genh, + name_suffix: 'fa', + build_by_default: false) +hwcore = declare_dependency(link_whole: libhwcore) +common_ss.add(hwcore) + ########### # Targets # ########### From c2306d713e4cfc95c25b395133e00a31d4e022c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:55:57 +0200 Subject: [PATCH 04/38] chardev: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-5-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- chardev/meson.build | 6 ------ meson.build | 7 +++++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/chardev/meson.build b/chardev/meson.build index 54e88d0310..dd2699a11b 100644 --- a/chardev/meson.build +++ b/chardev/meson.build @@ -1,4 +1,3 @@ -chardev_ss = ss.source_set() chardev_ss.add(files( 'char-fe.c', 'char-file.c', @@ -25,11 +24,6 @@ chardev_ss.add(when: 'CONFIG_WIN32', if_true: files( )) chardev_ss = chardev_ss.apply(config_host, strict: false) -libchardev = static_library('chardev', chardev_ss.sources() + genh, - name_suffix: 'fa', - build_by_default: false) - -chardev = declare_dependency(link_whole: libchardev) softmmu_ss.add(files('chardev-sysemu.c', 'msmouse.c', 'wctablet.c', 'testdev.c')) softmmu_ss.add(when: ['CONFIG_SPICE', spice], if_true: files('spice.c')) diff --git a/meson.build b/meson.build index 6c760b4163..39fc074977 100644 --- a/meson.build +++ b/meson.build @@ -1185,6 +1185,7 @@ sphinx_extn_depends = [ meson.source_root() / 'docs/sphinx/depfile.py', blockdev_ss = ss.source_set() block_ss = ss.source_set() bsd_user_ss = ss.source_set() +chardev_ss = ss.source_set() common_ss = ss.source_set() linux_user_ss = ss.source_set() qmp_ss = ss.source_set() @@ -1469,6 +1470,12 @@ libqmp = static_library('qmp', qmp_ss.sources() + genh, qmp = declare_dependency(link_whole: [libqmp]) +libchardev = static_library('chardev', chardev_ss.sources() + genh, + name_suffix: 'fa', + build_by_default: false) + +chardev = declare_dependency(link_whole: libchardev) + libhwcore = static_library('hwcore', sources: hwcore_files + genh, name_suffix: 'fa', build_by_default: false) From 7e6edef3f8cd1176d267c86151c9cf2fc0777781 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:55:58 +0200 Subject: [PATCH 05/38] migration: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-6-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- meson.build | 7 +++++++ migration/meson.build | 8 +------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/meson.build b/meson.build index 39fc074977..dce48d8304 100644 --- a/meson.build +++ b/meson.build @@ -1451,6 +1451,13 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', # Library dependencies # ######################## +libmigration = static_library('migration', sources: migration_files + genh, + name_suffix: 'fa', + build_by_default: false) +migration = declare_dependency(link_with: libmigration, + dependencies: [zlib, qom, io]) +softmmu_ss.add(migration) + block_ss = block_ss.apply(config_host, strict: false) libblock = static_library('block', block_ss.sources() + genh, dependencies: block_ss.dependencies(), diff --git a/migration/meson.build b/migration/meson.build index b5b71c8060..980e37865c 100644 --- a/migration/meson.build +++ b/migration/meson.build @@ -8,13 +8,7 @@ migration_files = files( 'qemu-file.c', 'qjson.c', ) - -libmigration = static_library('migration', sources: migration_files + genh, - name_suffix: 'fa', - build_by_default: false) -migration = declare_dependency(link_with: libmigration, - dependencies: [zlib, qom, io]) -softmmu_ss.add(migration) +softmmu_ss.add(migration_files) softmmu_ss.add(files( 'block-dirty-bitmap.c', From f78536b149b7c4caa7e33e7f6b2871c12b14e9bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:55:59 +0200 Subject: [PATCH 06/38] io: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-7-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- io/meson.build | 10 ---------- meson.build | 10 ++++++++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/io/meson.build b/io/meson.build index 768c1b5ec3..bcd8b1e737 100644 --- a/io/meson.build +++ b/io/meson.build @@ -1,4 +1,3 @@ -io_ss = ss.source_set() io_ss.add(genh) io_ss.add(files( 'channel-buffer.c', @@ -14,12 +13,3 @@ io_ss.add(files( 'net-listener.c', 'task.c', )) - -io_ss = io_ss.apply(config_host, strict: false) -libio = static_library('io', io_ss.sources() + genh, - dependencies: [io_ss.dependencies()], - link_with: libqemuutil, - name_suffix: 'fa', - build_by_default: false) - -io = declare_dependency(link_whole: libio, dependencies: [crypto, qom]) diff --git a/meson.build b/meson.build index dce48d8304..28f31e6022 100644 --- a/meson.build +++ b/meson.build @@ -1187,6 +1187,7 @@ block_ss = ss.source_set() bsd_user_ss = ss.source_set() chardev_ss = ss.source_set() common_ss = ss.source_set() +io_ss = ss.source_set() linux_user_ss = ss.source_set() qmp_ss = ss.source_set() softmmu_ss = ss.source_set() @@ -1451,6 +1452,15 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', # Library dependencies # ######################## +io_ss = io_ss.apply(config_host, strict: false) +libio = static_library('io', io_ss.sources() + genh, + dependencies: [io_ss.dependencies()], + link_with: libqemuutil, + name_suffix: 'fa', + build_by_default: false) + +io = declare_dependency(link_whole: libio, dependencies: [crypto, qom]) + libmigration = static_library('migration', sources: migration_files + genh, name_suffix: 'fa', build_by_default: false) From 2389304a720e8034aee63da3f5adfeab94d9bfcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:56:00 +0200 Subject: [PATCH 07/38] crypto: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-8-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- crypto/meson.build | 10 ---------- meson.build | 10 ++++++++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/crypto/meson.build b/crypto/meson.build index f6f5ce1ecd..7f37b5d335 100644 --- a/crypto/meson.build +++ b/crypto/meson.build @@ -1,4 +1,3 @@ -crypto_ss = ss.source_set() crypto_ss.add(genh) crypto_ss.add(files( 'afsplit.c', @@ -52,15 +51,6 @@ if 'CONFIG_GNUTLS' in config_host endif -crypto_ss = crypto_ss.apply(config_host, strict: false) -libcrypto = static_library('crypto', crypto_ss.sources() + genh, - dependencies: [crypto_ss.dependencies()], - name_suffix: 'fa', - build_by_default: false) - -crypto = declare_dependency(link_whole: libcrypto, - dependencies: [authz, qom]) - util_ss.add(files('aes.c')) util_ss.add(files('init.c')) diff --git a/meson.build b/meson.build index 28f31e6022..9c199195bf 100644 --- a/meson.build +++ b/meson.build @@ -1187,6 +1187,7 @@ block_ss = ss.source_set() bsd_user_ss = ss.source_set() chardev_ss = ss.source_set() common_ss = ss.source_set() +crypto_ss = ss.source_set() io_ss = ss.source_set() linux_user_ss = ss.source_set() qmp_ss = ss.source_set() @@ -1452,6 +1453,15 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', # Library dependencies # ######################## +crypto_ss = crypto_ss.apply(config_host, strict: false) +libcrypto = static_library('crypto', crypto_ss.sources() + genh, + dependencies: [crypto_ss.dependencies()], + name_suffix: 'fa', + build_by_default: false) + +crypto = declare_dependency(link_whole: libcrypto, + dependencies: [authz, qom]) + io_ss = io_ss.apply(config_host, strict: false) libio = static_library('io', io_ss.sources() + genh, dependencies: [io_ss.dependencies()], From 555678917697b6bef70e2d07c8a19d199a53b592 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:56:01 +0200 Subject: [PATCH 08/38] authz: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-9-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- authz/meson.build | 10 ---------- meson.build | 10 ++++++++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/authz/meson.build b/authz/meson.build index 516d71f2e2..88fa7769cb 100644 --- a/authz/meson.build +++ b/authz/meson.build @@ -1,4 +1,3 @@ -authz_ss = ss.source_set() authz_ss.add(genh) authz_ss.add(files( 'base.c', @@ -8,12 +7,3 @@ authz_ss.add(files( )) authz_ss.add(when: ['CONFIG_AUTH_PAM', pam], if_true: files('pamacct.c')) - -authz_ss = authz_ss.apply(config_host, strict: false) -libauthz = static_library('authz', authz_ss.sources() + genh, - dependencies: [authz_ss.dependencies()], - name_suffix: 'fa', - build_by_default: false) - -authz = declare_dependency(link_whole: libauthz, - dependencies: qom) diff --git a/meson.build b/meson.build index 9c199195bf..2736f74b8f 100644 --- a/meson.build +++ b/meson.build @@ -1182,6 +1182,7 @@ sphinx_extn_depends = [ meson.source_root() / 'docs/sphinx/depfile.py', # Collect sourcesets. +authz_ss = ss.source_set() blockdev_ss = ss.source_set() block_ss = ss.source_set() bsd_user_ss = ss.source_set() @@ -1453,6 +1454,15 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', # Library dependencies # ######################## +authz_ss = authz_ss.apply(config_host, strict: false) +libauthz = static_library('authz', authz_ss.sources() + genh, + dependencies: [authz_ss.dependencies()], + name_suffix: 'fa', + build_by_default: false) + +authz = declare_dependency(link_whole: libauthz, + dependencies: qom) + crypto_ss = crypto_ss.apply(config_host, strict: false) libcrypto = static_library('crypto', crypto_ss.sources() + genh, dependencies: [crypto_ss.dependencies()], From da33fc09873af7ee5f60dbd0b09d30926f9cb851 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 14:56:02 +0200 Subject: [PATCH 09/38] qom: Move the creation of the library to the main meson.build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be consistent creating all the libraries in the main meson.build file. Suggested-by: Paolo Bonzini Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20201006125602.2311423-10-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- meson.build | 8 ++++++++ qom/meson.build | 8 -------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/meson.build b/meson.build index 2736f74b8f..f4ef3b83f3 100644 --- a/meson.build +++ b/meson.build @@ -1192,6 +1192,7 @@ crypto_ss = ss.source_set() io_ss = ss.source_set() linux_user_ss = ss.source_set() qmp_ss = ss.source_set() +qom_ss = ss.source_set() softmmu_ss = ss.source_set() specific_fuzz_ss = ss.source_set() specific_ss = ss.source_set() @@ -1454,6 +1455,13 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', # Library dependencies # ######################## +qom_ss = qom_ss.apply(config_host, strict: false) +libqom = static_library('qom', qom_ss.sources() + genh, + dependencies: [qom_ss.dependencies()], + name_suffix: 'fa') + +qom = declare_dependency(link_whole: libqom) + authz_ss = authz_ss.apply(config_host, strict: false) libauthz = static_library('authz', authz_ss.sources() + genh, dependencies: [authz_ss.dependencies()], diff --git a/qom/meson.build b/qom/meson.build index a1cd03c82c..062a3789d8 100644 --- a/qom/meson.build +++ b/qom/meson.build @@ -1,4 +1,3 @@ -qom_ss = ss.source_set() qom_ss.add(genh) qom_ss.add(files( 'container.c', @@ -9,10 +8,3 @@ qom_ss.add(files( qmp_ss.add(files('qom-qmp-cmds.c')) softmmu_ss.add(files('qom-hmp-cmds.c')) - -qom_ss = qom_ss.apply(config_host, strict: false) -libqom = static_library('qom', qom_ss.sources() + genh, - dependencies: [qom_ss.dependencies()], - name_suffix: 'fa') - -qom = declare_dependency(link_whole: libqom) From 8d0bceba24cc0917d9c6c5252d78db3d9fd5c58d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 6 Oct 2020 13:19:09 +0200 Subject: [PATCH 10/38] hw/nvram: Always register FW_CFG_DATA_GENERATOR_INTERFACE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While the FW_CFG_DATA_GENERATOR_INTERFACE is only consumed by a device only available using system-mode (fw_cfg), it is implemented by a crypto component (tls-cipher-suites) which is always available when crypto is used. Commit 69699f3055 introduced the following error in the qemu-storage-daemon binary: $ echo -e \ '{"execute": "qmp_capabilities"}\r\n{"execute": "qom-list-types"}\r\n{"execute": "quit"}\r\n' \ | storage-daemon/qemu-storage-daemon --chardev stdio,id=qmp0 --monitor qmp0 {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 5}, "package": ""}, "capabilities": ["oob"]}} {"return": {}} missing interface 'fw_cfg-data-generator' for object 'tls-creds' Aborted (core dumped) Since QOM dependencies are resolved at runtime, this issue could not be triggered at linktime, and we don't have test running the qemu-storage-daemon binary. Fix by always registering the QOM interface. Reported-by: Kevin Wolf Fixes: 69699f3055 ("crypto/tls-cipher-suites: Produce fw_cfg consumable blob") Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Laszlo Ersek Reviewed-by: Paolo Bonzini Message-Id: <20201006111909.2302081-2-philmd@redhat.com> Signed-off-by: Paolo Bonzini --- MAINTAINERS | 2 +- hw/nvram/fw_cfg-interface.c | 23 +++++++++++++++++++++++ hw/nvram/fw_cfg.c | 7 ------- hw/nvram/meson.build | 3 +++ 4 files changed, 27 insertions(+), 8 deletions(-) create mode 100644 hw/nvram/fw_cfg-interface.c diff --git a/MAINTAINERS b/MAINTAINERS index e9d85cc873..8d50b96c33 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2055,7 +2055,7 @@ R: Laszlo Ersek R: Gerd Hoffmann S: Supported F: docs/specs/fw_cfg.txt -F: hw/nvram/fw_cfg.c +F: hw/nvram/fw_cfg*.c F: stubs/fw_cfg.c F: include/hw/nvram/fw_cfg.h F: include/standard-headers/linux/qemu_fw_cfg.h diff --git a/hw/nvram/fw_cfg-interface.c b/hw/nvram/fw_cfg-interface.c new file mode 100644 index 0000000000..0e93feeae5 --- /dev/null +++ b/hw/nvram/fw_cfg-interface.c @@ -0,0 +1,23 @@ +/* + * QEMU Firmware configuration device emulation (QOM interfaces) + * + * Copyright 2020 Red Hat, Inc. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/nvram/fw_cfg.h" + +static const TypeInfo fw_cfg_data_generator_interface_info = { + .parent = TYPE_INTERFACE, + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, + .class_size = sizeof(FWCfgDataGeneratorClass), +}; + +static void fw_cfg_register_interfaces(void) +{ + type_register_static(&fw_cfg_data_generator_interface_info); +} + +type_init(fw_cfg_register_interfaces) diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 0e95d057fd..08539a1aab 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -1360,18 +1360,11 @@ static const TypeInfo fw_cfg_mem_info = { .class_init = fw_cfg_mem_class_init, }; -static const TypeInfo fw_cfg_data_generator_interface_info = { - .parent = TYPE_INTERFACE, - .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, - .class_size = sizeof(FWCfgDataGeneratorClass), -}; - static void fw_cfg_register_types(void) { type_register_static(&fw_cfg_info); type_register_static(&fw_cfg_io_info); type_register_static(&fw_cfg_mem_info); - type_register_static(&fw_cfg_data_generator_interface_info); } type_init(fw_cfg_register_types) diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build index 1f2ed013b2..fd2951a860 100644 --- a/hw/nvram/meson.build +++ b/hw/nvram/meson.build @@ -1,3 +1,6 @@ +# QOM interfaces must be available anytime QOM is used. +qom_ss.add(files('fw_cfg-interface.c')) + softmmu_ss.add(files('fw_cfg.c')) softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c')) softmmu_ss.add(when: 'CONFIG_DS1225Y', if_true: files('ds1225y.c')) From 800d4deda04be016a95fbbf397c830a2d14ff9f6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 09:01:22 +0200 Subject: [PATCH 11/38] softmmu: move more files to softmmu/ Keep most softmmu_ss files into the system-emulation-specific directory. Signed-off-by: Paolo Bonzini --- MAINTAINERS | 9 +++++---- meson.build | 10 ---------- bootdevice.c => softmmu/bootdevice.c | 0 device_tree.c => softmmu/device_tree.c | 0 dma-helpers.c => softmmu/dma-helpers.c | 0 softmmu/meson.build | 10 ++++++++++ qdev-monitor.c => softmmu/qdev-monitor.c | 0 qemu-seccomp.c => softmmu/qemu-seccomp.c | 0 tpm.c => softmmu/tpm.c | 0 9 files changed, 15 insertions(+), 14 deletions(-) rename bootdevice.c => softmmu/bootdevice.c (100%) rename device_tree.c => softmmu/device_tree.c (100%) rename dma-helpers.c => softmmu/dma-helpers.c (100%) rename qdev-monitor.c => softmmu/qdev-monitor.c (100%) rename qemu-seccomp.c => softmmu/qemu-seccomp.c (100%) rename tpm.c => softmmu/tpm.c (100%) diff --git a/MAINTAINERS b/MAINTAINERS index 8d50b96c33..dda54f000d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2235,7 +2235,7 @@ Device Tree M: Alistair Francis R: David Gibson S: Maintained -F: device_tree.c +F: softmmu/device_tree.c F: include/sysemu/device_tree.h Dump @@ -2281,6 +2281,7 @@ F: include/exec/memop.h F: include/exec/memory.h F: include/exec/ram_addr.h F: include/exec/ramblock.h +F: softmmu/dma-helpers.c F: softmmu/ioport.c F: softmmu/memory.c F: include/exec/memory-internal.h @@ -2461,7 +2462,7 @@ F: include/monitor/qdev.h F: include/qom/ F: qapi/qom.json F: qapi/qdev.json -F: qdev-monitor.c +F: softmmu/qdev-monitor.c F: qom/ F: tests/check-qom-interface.c F: tests/check-qom-proplist.c @@ -2591,7 +2592,7 @@ F: docs/interop/dbus-vmstate.rst Seccomp M: Eduardo Otubo S: Supported -F: qemu-seccomp.c +F: softmmu/qemu-seccomp.c F: include/sysemu/seccomp.h Cryptography @@ -2957,7 +2958,7 @@ T: git https://github.com/stefanha/qemu.git block Bootdevice M: Gonglei S: Maintained -F: bootdevice.c +F: softmmu/bootdevice.c Quorum M: Alberto Garcia diff --git a/meson.build b/meson.build index f4ef3b83f3..a280e4cf21 100644 --- a/meson.build +++ b/meson.build @@ -1365,17 +1365,7 @@ blockdev_ss.add(files( # os-win32.c does not blockdev_ss.add(when: 'CONFIG_POSIX', if_true: files('os-posix.c')) softmmu_ss.add(when: 'CONFIG_WIN32', if_true: [files('os-win32.c')]) - softmmu_ss.add_all(blockdev_ss) -softmmu_ss.add(files( - 'bootdevice.c', - 'dma-helpers.c', - 'qdev-monitor.c', -), sdl) - -softmmu_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c')) -softmmu_ss.add(when: 'CONFIG_SECCOMP', if_true: [files('qemu-seccomp.c'), seccomp]) -softmmu_ss.add(when: fdt, if_true: files('device_tree.c')) common_ss.add(files('cpus-common.c')) diff --git a/bootdevice.c b/softmmu/bootdevice.c similarity index 100% rename from bootdevice.c rename to softmmu/bootdevice.c diff --git a/device_tree.c b/softmmu/device_tree.c similarity index 100% rename from device_tree.c rename to softmmu/device_tree.c diff --git a/dma-helpers.c b/softmmu/dma-helpers.c similarity index 100% rename from dma-helpers.c rename to softmmu/dma-helpers.c diff --git a/softmmu/meson.build b/softmmu/meson.build index 36c96e7b15..862ab24878 100644 --- a/softmmu/meson.build +++ b/softmmu/meson.build @@ -14,3 +14,13 @@ specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: [files( specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: [files( 'icount.c' )]) + +softmmu_ss.add(files( + 'bootdevice.c', + 'dma-helpers.c', + 'qdev-monitor.c', +), sdl) + +softmmu_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c')) +softmmu_ss.add(when: 'CONFIG_SECCOMP', if_true: [files('qemu-seccomp.c'), seccomp]) +softmmu_ss.add(when: fdt, if_true: files('device_tree.c')) diff --git a/qdev-monitor.c b/softmmu/qdev-monitor.c similarity index 100% rename from qdev-monitor.c rename to softmmu/qdev-monitor.c diff --git a/qemu-seccomp.c b/softmmu/qemu-seccomp.c similarity index 100% rename from qemu-seccomp.c rename to softmmu/qemu-seccomp.c diff --git a/tpm.c b/softmmu/tpm.c similarity index 100% rename from tpm.c rename to softmmu/tpm.c From d9f24bf57241453e078dba28d16fe3a430f06da1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 09:05:29 +0200 Subject: [PATCH 12/38] exec: split out non-softmmu-specific parts Over the years, most parts of exec.c that were not specific to softmmu have been moved to accel/tcg; what's left is mostly the low-level part of the memory API, which includes RAMBlock and AddressSpaceDispatch. However exec.c also hosts 4-500 lines of code for the target specific parts of the CPU QOM object, plus a few functions for user-mode emulation that do not have a better place (they are not TCG-specific so accel/tcg/user-exec.c is not a good place either). Move these parts to a new file, so that exec.c can be moved to softmmu/physmem.c. Signed-off-by: Paolo Bonzini --- MAINTAINERS | 4 +- cpu.c | 452 +++++++++++++++++++++++++++++++++++ include/exec/cpu-common.h | 3 + meson.build | 2 +- softmmu/meson.build | 3 +- exec.c => softmmu/physmem.c | 454 +----------------------------------- 6 files changed, 467 insertions(+), 451 deletions(-) create mode 100644 cpu.c rename exec.c => softmmu/physmem.c (91%) diff --git a/MAINTAINERS b/MAINTAINERS index dda54f000d..7ef459a33c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -117,7 +117,6 @@ R: Paolo Bonzini S: Maintained F: softmmu/cpus.c F: cpus-common.c -F: exec.c F: accel/tcg/ F: accel/stubs/tcg-stub.c F: scripts/decodetree.py @@ -1525,6 +1524,7 @@ Machine core M: Eduardo Habkost M: Marcel Apfelbaum S: Supported +F: cpu.c F: hw/core/cpu.c F: hw/core/machine-qmp-cmds.c F: hw/core/machine.c @@ -2284,8 +2284,8 @@ F: include/exec/ramblock.h F: softmmu/dma-helpers.c F: softmmu/ioport.c F: softmmu/memory.c +F: softmmu/physmem.c F: include/exec/memory-internal.h -F: exec.c F: scripts/coccinelle/memory-region-housekeeping.cocci SPICE diff --git a/cpu.c b/cpu.c new file mode 100644 index 0000000000..0be5dcb6f3 --- /dev/null +++ b/cpu.c @@ -0,0 +1,452 @@ +/* + * Target-specific parts of the CPU object + * + * Copyright (c) 2003 Fabrice Bellard + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * 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 "qemu-common.h" +#include "qapi/error.h" + +#include "exec/target_page.h" +#include "hw/qdev-core.h" +#include "hw/qdev-properties.h" +#include "qemu/error-report.h" +#include "migration/vmstate.h" +#ifdef CONFIG_USER_ONLY +#include "qemu.h" +#else +#include "exec/address-spaces.h" +#endif +#include "sysemu/tcg.h" +#include "sysemu/kvm.h" +#include "sysemu/replay.h" +#include "translate-all.h" +#include "exec/log.h" + +uintptr_t qemu_host_page_size; +intptr_t qemu_host_page_mask; + +#ifndef CONFIG_USER_ONLY +static int cpu_common_post_load(void *opaque, int version_id) +{ + CPUState *cpu = opaque; + + /* 0x01 was CPU_INTERRUPT_EXIT. This line can be removed when the + version_id is increased. */ + cpu->interrupt_request &= ~0x01; + tlb_flush(cpu); + + /* loadvm has just updated the content of RAM, bypassing the + * usual mechanisms that ensure we flush TBs for writes to + * memory we've translated code from. So we must flush all TBs, + * which will now be stale. + */ + tb_flush(cpu); + + return 0; +} + +static int cpu_common_pre_load(void *opaque) +{ + CPUState *cpu = opaque; + + cpu->exception_index = -1; + + return 0; +} + +static bool cpu_common_exception_index_needed(void *opaque) +{ + CPUState *cpu = opaque; + + return tcg_enabled() && cpu->exception_index != -1; +} + +static const VMStateDescription vmstate_cpu_common_exception_index = { + .name = "cpu_common/exception_index", + .version_id = 1, + .minimum_version_id = 1, + .needed = cpu_common_exception_index_needed, + .fields = (VMStateField[]) { + VMSTATE_INT32(exception_index, CPUState), + VMSTATE_END_OF_LIST() + } +}; + +static bool cpu_common_crash_occurred_needed(void *opaque) +{ + CPUState *cpu = opaque; + + return cpu->crash_occurred; +} + +static const VMStateDescription vmstate_cpu_common_crash_occurred = { + .name = "cpu_common/crash_occurred", + .version_id = 1, + .minimum_version_id = 1, + .needed = cpu_common_crash_occurred_needed, + .fields = (VMStateField[]) { + VMSTATE_BOOL(crash_occurred, CPUState), + VMSTATE_END_OF_LIST() + } +}; + +const VMStateDescription vmstate_cpu_common = { + .name = "cpu_common", + .version_id = 1, + .minimum_version_id = 1, + .pre_load = cpu_common_pre_load, + .post_load = cpu_common_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT32(halted, CPUState), + VMSTATE_UINT32(interrupt_request, CPUState), + VMSTATE_END_OF_LIST() + }, + .subsections = (const VMStateDescription*[]) { + &vmstate_cpu_common_exception_index, + &vmstate_cpu_common_crash_occurred, + NULL + } +}; +#endif + +void cpu_exec_unrealizefn(CPUState *cpu) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + + tlb_destroy(cpu); + cpu_list_remove(cpu); + +#ifdef CONFIG_USER_ONLY + assert(cc->vmsd == NULL); +#else + if (cc->vmsd != NULL) { + vmstate_unregister(NULL, cc->vmsd, cpu); + } + if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { + vmstate_unregister(NULL, &vmstate_cpu_common, cpu); + } + tcg_iommu_free_notifier_list(cpu); +#endif +} + +Property cpu_common_props[] = { +#ifndef CONFIG_USER_ONLY + /* Create a memory property for softmmu CPU object, + * so users can wire up its memory. (This can't go in hw/core/cpu.c + * because that file is compiled only once for both user-mode + * and system builds.) The default if no link is set up is to use + * the system address space. + */ + DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION, + MemoryRegion *), +#endif + DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false), + DEFINE_PROP_END_OF_LIST(), +}; + +void cpu_exec_initfn(CPUState *cpu) +{ + cpu->as = NULL; + cpu->num_ases = 0; + +#ifndef CONFIG_USER_ONLY + cpu->thread_id = qemu_get_thread_id(); + cpu->memory = get_system_memory(); + object_ref(OBJECT(cpu->memory)); +#endif +} + +void cpu_exec_realizefn(CPUState *cpu, Error **errp) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + static bool tcg_target_initialized; + + cpu_list_add(cpu); + + if (tcg_enabled() && !tcg_target_initialized) { + tcg_target_initialized = true; + cc->tcg_initialize(); + } + tlb_init(cpu); + + qemu_plugin_vcpu_init_hook(cpu); + +#ifdef CONFIG_USER_ONLY + assert(cc->vmsd == NULL); +#else /* !CONFIG_USER_ONLY */ + if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { + vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); + } + if (cc->vmsd != NULL) { + vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); + } + + tcg_iommu_init_notifier_list(cpu); +#endif +} + +const char *parse_cpu_option(const char *cpu_option) +{ + ObjectClass *oc; + CPUClass *cc; + gchar **model_pieces; + const char *cpu_type; + + model_pieces = g_strsplit(cpu_option, ",", 2); + if (!model_pieces[0]) { + error_report("-cpu option cannot be empty"); + exit(1); + } + + oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]); + if (oc == NULL) { + error_report("unable to find CPU model '%s'", model_pieces[0]); + g_strfreev(model_pieces); + exit(EXIT_FAILURE); + } + + cpu_type = object_class_get_name(oc); + cc = CPU_CLASS(oc); + cc->parse_features(cpu_type, model_pieces[1], &error_fatal); + g_strfreev(model_pieces); + return cpu_type; +} + +#if defined(CONFIG_USER_ONLY) +void tb_invalidate_phys_addr(target_ulong addr) +{ + mmap_lock(); + tb_invalidate_phys_page_range(addr, addr + 1); + mmap_unlock(); +} + +static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) +{ + tb_invalidate_phys_addr(pc); +} +#else +void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs) +{ + ram_addr_t ram_addr; + MemoryRegion *mr; + hwaddr l = 1; + + if (!tcg_enabled()) { + return; + } + + RCU_READ_LOCK_GUARD(); + mr = address_space_translate(as, addr, &addr, &l, false, attrs); + if (!(memory_region_is_ram(mr) + || memory_region_is_romd(mr))) { + return; + } + ram_addr = memory_region_get_ram_addr(mr) + addr; + tb_invalidate_phys_page_range(ram_addr, ram_addr + 1); +} + +static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) +{ + /* + * There may not be a virtual to physical translation for the pc + * right now, but there may exist cached TB for this pc. + * Flush the whole TB cache to force re-translation of such TBs. + * This is heavyweight, but we're debugging anyway. + */ + tb_flush(cpu); +} +#endif + +/* Add a breakpoint. */ +int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, + CPUBreakpoint **breakpoint) +{ + CPUBreakpoint *bp; + + bp = g_malloc(sizeof(*bp)); + + bp->pc = pc; + bp->flags = flags; + + /* keep all GDB-injected breakpoints in front */ + if (flags & BP_GDB) { + QTAILQ_INSERT_HEAD(&cpu->breakpoints, bp, entry); + } else { + QTAILQ_INSERT_TAIL(&cpu->breakpoints, bp, entry); + } + + breakpoint_invalidate(cpu, pc); + + if (breakpoint) { + *breakpoint = bp; + } + return 0; +} + +/* Remove a specific breakpoint. */ +int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) +{ + CPUBreakpoint *bp; + + QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) { + if (bp->pc == pc && bp->flags == flags) { + cpu_breakpoint_remove_by_ref(cpu, bp); + return 0; + } + } + return -ENOENT; +} + +/* Remove a specific breakpoint by reference. */ +void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint) +{ + QTAILQ_REMOVE(&cpu->breakpoints, breakpoint, entry); + + breakpoint_invalidate(cpu, breakpoint->pc); + + g_free(breakpoint); +} + +/* Remove all matching breakpoints. */ +void cpu_breakpoint_remove_all(CPUState *cpu, int mask) +{ + CPUBreakpoint *bp, *next; + + QTAILQ_FOREACH_SAFE(bp, &cpu->breakpoints, entry, next) { + if (bp->flags & mask) { + cpu_breakpoint_remove_by_ref(cpu, bp); + } + } +} + +/* enable or disable single step mode. EXCP_DEBUG is returned by the + CPU loop after each instruction */ +void cpu_single_step(CPUState *cpu, int enabled) +{ + if (cpu->singlestep_enabled != enabled) { + cpu->singlestep_enabled = enabled; + if (kvm_enabled()) { + kvm_update_guest_debug(cpu, 0); + } else { + /* must flush all the translated code to avoid inconsistencies */ + /* XXX: only flush what is necessary */ + tb_flush(cpu); + } + } +} + +void cpu_abort(CPUState *cpu, const char *fmt, ...) +{ + va_list ap; + va_list ap2; + + va_start(ap, fmt); + va_copy(ap2, ap); + fprintf(stderr, "qemu: fatal: "); + vfprintf(stderr, fmt, ap); + fprintf(stderr, "\n"); + cpu_dump_state(cpu, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP); + if (qemu_log_separate()) { + FILE *logfile = qemu_log_lock(); + qemu_log("qemu: fatal: "); + qemu_log_vprintf(fmt, ap2); + qemu_log("\n"); + log_cpu_state(cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP); + qemu_log_flush(); + qemu_log_unlock(logfile); + qemu_log_close(); + } + va_end(ap2); + va_end(ap); + replay_finish(); +#if defined(CONFIG_USER_ONLY) + { + struct sigaction act; + sigfillset(&act.sa_mask); + act.sa_handler = SIG_DFL; + act.sa_flags = 0; + sigaction(SIGABRT, &act, NULL); + } +#endif + abort(); +} + +/* physical memory access (slow version, mainly for debug) */ +#if defined(CONFIG_USER_ONLY) +int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, + void *ptr, target_ulong len, bool is_write) +{ + int flags; + target_ulong l, page; + void * p; + uint8_t *buf = ptr; + + while (len > 0) { + page = addr & TARGET_PAGE_MASK; + l = (page + TARGET_PAGE_SIZE) - addr; + if (l > len) + l = len; + flags = page_get_flags(page); + if (!(flags & PAGE_VALID)) + return -1; + if (is_write) { + if (!(flags & PAGE_WRITE)) + return -1; + /* XXX: this code should not depend on lock_user */ + if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) + return -1; + memcpy(p, buf, l); + unlock_user(p, addr, l); + } else { + if (!(flags & PAGE_READ)) + return -1; + /* XXX: this code should not depend on lock_user */ + if (!(p = lock_user(VERIFY_READ, addr, l, 1))) + return -1; + memcpy(buf, p, l); + unlock_user(p, addr, 0); + } + len -= l; + buf += l; + addr += l; + } + return 0; +} +#endif + +bool target_words_bigendian(void) +{ +#if defined(TARGET_WORDS_BIGENDIAN) + return true; +#else + return false; +#endif +} + +void page_size_init(void) +{ + /* NOTE: we can always suppose that qemu_host_page_size >= + TARGET_PAGE_SIZE */ + if (qemu_host_page_size == 0) { + qemu_host_page_size = qemu_real_host_page_size; + } + if (qemu_host_page_size < TARGET_PAGE_SIZE) { + qemu_host_page_size = TARGET_PAGE_SIZE; + } + qemu_host_page_mask = -(intptr_t)qemu_host_page_size; +} diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index d5e285d2b5..19805ed6db 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -14,6 +14,9 @@ void cpu_list_unlock(void); void tcg_flush_softmmu_tlb(CPUState *cs); +void tcg_iommu_init_notifier_list(CPUState *cpu); +void tcg_iommu_free_notifier_list(CPUState *cpu); + #if !defined(CONFIG_USER_ONLY) enum device_endian { diff --git a/meson.build b/meson.build index a280e4cf21..26230614ba 100644 --- a/meson.build +++ b/meson.build @@ -1372,7 +1372,7 @@ common_ss.add(files('cpus-common.c')) subdir('softmmu') common_ss.add(capstone) -specific_ss.add(files('disas.c', 'exec.c', 'gdbstub.c'), capstone, libpmem, libdaxctl) +specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone) specific_ss.add(files('exec-vary.c')) specific_ss.add(when: 'CONFIG_TCG', if_true: files( 'fpu/softfloat.c', diff --git a/softmmu/meson.build b/softmmu/meson.build index 862ab24878..8f7210b4f0 100644 --- a/softmmu/meson.build +++ b/softmmu/meson.build @@ -3,6 +3,7 @@ specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: [files( 'balloon.c', 'cpus.c', 'cpu-throttle.c', + 'physmem.c', 'ioport.c', 'memory.c', 'memory_mapping.c', @@ -19,7 +20,7 @@ softmmu_ss.add(files( 'bootdevice.c', 'dma-helpers.c', 'qdev-monitor.c', -), sdl) +), sdl, libpmem, libdaxctl) softmmu_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c')) softmmu_ss.add(when: 'CONFIG_SECCOMP', if_true: [files('qemu-seccomp.c'), seccomp]) diff --git a/exec.c b/softmmu/physmem.c similarity index 91% rename from exec.c rename to softmmu/physmem.c index bca441f7fd..e319fb2a1e 100644 --- a/exec.c +++ b/softmmu/physmem.c @@ -1,5 +1,5 @@ /* - * Virtual page mapping + * RAM allocation and memory access * * Copyright (c) 2003 Fabrice Bellard * @@ -28,10 +28,8 @@ #include "tcg/tcg.h" #include "hw/qdev-core.h" #include "hw/qdev-properties.h" -#if !defined(CONFIG_USER_ONLY) #include "hw/boards.h" #include "hw/xen/xen.h" -#endif #include "sysemu/kvm.h" #include "sysemu/sysemu.h" #include "sysemu/tcg.h" @@ -40,9 +38,6 @@ #include "qemu/config-file.h" #include "qemu/error-report.h" #include "qemu/qemu-print.h" -#if defined(CONFIG_USER_ONLY) -#include "qemu.h" -#else /* !CONFIG_USER_ONLY */ #include "exec/memory.h" #include "exec/ioport.h" #include "sysemu/dma.h" @@ -56,7 +51,6 @@ #include #endif -#endif #include "qemu/rcu_queue.h" #include "qemu/main-loop.h" #include "translate-all.h" @@ -83,7 +77,6 @@ //#define DEBUG_SUBPAGE -#if !defined(CONFIG_USER_ONLY) /* ram_list is read under rcu_read_lock()/rcu_read_unlock(). Writes * are protected by the ramlist lock. */ @@ -96,12 +89,6 @@ AddressSpace address_space_io; AddressSpace address_space_memory; static MemoryRegion io_mem_unassigned; -#endif - -uintptr_t qemu_host_page_size; -intptr_t qemu_host_page_mask; - -#if !defined(CONFIG_USER_ONLY) typedef struct PhysPageEntry PhysPageEntry; @@ -179,10 +166,6 @@ struct DirtyBitmapSnapshot { unsigned long dirty[]; }; -#endif - -#if !defined(CONFIG_USER_ONLY) - static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes) { static unsigned alloc_hint = 16; @@ -661,7 +644,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu, } } -static void tcg_iommu_free_notifier_list(CPUState *cpu) +void tcg_iommu_free_notifier_list(CPUState *cpu) { /* Destroy the CPU's notifier list */ int i; @@ -675,6 +658,11 @@ static void tcg_iommu_free_notifier_list(CPUState *cpu) g_array_free(cpu->iommu_notifiers, true); } +void tcg_iommu_init_notifier_list(CPUState *cpu) +{ + cpu->iommu_notifiers = g_array_new(false, true, sizeof(TCGIOMMUNotifier *)); +} + /* Called from RCU critical section */ MemoryRegionSection * address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr, @@ -732,91 +720,6 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr, translate_fail: return &d->map.sections[PHYS_SECTION_UNASSIGNED]; } -#endif - -#if !defined(CONFIG_USER_ONLY) - -static int cpu_common_post_load(void *opaque, int version_id) -{ - CPUState *cpu = opaque; - - /* 0x01 was CPU_INTERRUPT_EXIT. This line can be removed when the - version_id is increased. */ - cpu->interrupt_request &= ~0x01; - tlb_flush(cpu); - - /* loadvm has just updated the content of RAM, bypassing the - * usual mechanisms that ensure we flush TBs for writes to - * memory we've translated code from. So we must flush all TBs, - * which will now be stale. - */ - tb_flush(cpu); - - return 0; -} - -static int cpu_common_pre_load(void *opaque) -{ - CPUState *cpu = opaque; - - cpu->exception_index = -1; - - return 0; -} - -static bool cpu_common_exception_index_needed(void *opaque) -{ - CPUState *cpu = opaque; - - return tcg_enabled() && cpu->exception_index != -1; -} - -static const VMStateDescription vmstate_cpu_common_exception_index = { - .name = "cpu_common/exception_index", - .version_id = 1, - .minimum_version_id = 1, - .needed = cpu_common_exception_index_needed, - .fields = (VMStateField[]) { - VMSTATE_INT32(exception_index, CPUState), - VMSTATE_END_OF_LIST() - } -}; - -static bool cpu_common_crash_occurred_needed(void *opaque) -{ - CPUState *cpu = opaque; - - return cpu->crash_occurred; -} - -static const VMStateDescription vmstate_cpu_common_crash_occurred = { - .name = "cpu_common/crash_occurred", - .version_id = 1, - .minimum_version_id = 1, - .needed = cpu_common_crash_occurred_needed, - .fields = (VMStateField[]) { - VMSTATE_BOOL(crash_occurred, CPUState), - VMSTATE_END_OF_LIST() - } -}; - -const VMStateDescription vmstate_cpu_common = { - .name = "cpu_common", - .version_id = 1, - .minimum_version_id = 1, - .pre_load = cpu_common_pre_load, - .post_load = cpu_common_post_load, - .fields = (VMStateField[]) { - VMSTATE_UINT32(halted, CPUState), - VMSTATE_UINT32(interrupt_request, CPUState), - VMSTATE_END_OF_LIST() - }, - .subsections = (const VMStateDescription*[]) { - &vmstate_cpu_common_exception_index, - &vmstate_cpu_common_crash_occurred, - NULL - } -}; void cpu_address_space_init(CPUState *cpu, int asidx, const char *prefix, MemoryRegion *mr) @@ -860,155 +763,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx) /* Return the AddressSpace corresponding to the specified index */ return cpu->cpu_ases[asidx].as; } -#endif -void cpu_exec_unrealizefn(CPUState *cpu) -{ - CPUClass *cc = CPU_GET_CLASS(cpu); - - tlb_destroy(cpu); - cpu_list_remove(cpu); - - if (cc->vmsd != NULL) { - vmstate_unregister(NULL, cc->vmsd, cpu); - } - if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { - vmstate_unregister(NULL, &vmstate_cpu_common, cpu); - } -#ifndef CONFIG_USER_ONLY - tcg_iommu_free_notifier_list(cpu); -#endif -} - -Property cpu_common_props[] = { -#ifndef CONFIG_USER_ONLY - /* Create a memory property for softmmu CPU object, - * so users can wire up its memory. (This can't go in hw/core/cpu.c - * because that file is compiled only once for both user-mode - * and system builds.) The default if no link is set up is to use - * the system address space. - */ - DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION, - MemoryRegion *), -#endif - DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false), - DEFINE_PROP_END_OF_LIST(), -}; - -void cpu_exec_initfn(CPUState *cpu) -{ - cpu->as = NULL; - cpu->num_ases = 0; - -#ifndef CONFIG_USER_ONLY - cpu->thread_id = qemu_get_thread_id(); - cpu->memory = system_memory; - object_ref(OBJECT(cpu->memory)); -#endif -} - -void cpu_exec_realizefn(CPUState *cpu, Error **errp) -{ - CPUClass *cc = CPU_GET_CLASS(cpu); - static bool tcg_target_initialized; - - cpu_list_add(cpu); - - if (tcg_enabled() && !tcg_target_initialized) { - tcg_target_initialized = true; - cc->tcg_initialize(); - } - tlb_init(cpu); - - qemu_plugin_vcpu_init_hook(cpu); - -#ifdef CONFIG_USER_ONLY - assert(cc->vmsd == NULL); -#else /* !CONFIG_USER_ONLY */ - if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); - } - if (cc->vmsd != NULL) { - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); - } - - cpu->iommu_notifiers = g_array_new(false, true, sizeof(TCGIOMMUNotifier *)); -#endif -} - -const char *parse_cpu_option(const char *cpu_option) -{ - ObjectClass *oc; - CPUClass *cc; - gchar **model_pieces; - const char *cpu_type; - - model_pieces = g_strsplit(cpu_option, ",", 2); - if (!model_pieces[0]) { - error_report("-cpu option cannot be empty"); - exit(1); - } - - oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]); - if (oc == NULL) { - error_report("unable to find CPU model '%s'", model_pieces[0]); - g_strfreev(model_pieces); - exit(EXIT_FAILURE); - } - - cpu_type = object_class_get_name(oc); - cc = CPU_CLASS(oc); - cc->parse_features(cpu_type, model_pieces[1], &error_fatal); - g_strfreev(model_pieces); - return cpu_type; -} - -#if defined(CONFIG_USER_ONLY) -void tb_invalidate_phys_addr(target_ulong addr) -{ - mmap_lock(); - tb_invalidate_phys_page_range(addr, addr + 1); - mmap_unlock(); -} - -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) -{ - tb_invalidate_phys_addr(pc); -} -#else -void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs) -{ - ram_addr_t ram_addr; - MemoryRegion *mr; - hwaddr l = 1; - - if (!tcg_enabled()) { - return; - } - - RCU_READ_LOCK_GUARD(); - mr = address_space_translate(as, addr, &addr, &l, false, attrs); - if (!(memory_region_is_ram(mr) - || memory_region_is_romd(mr))) { - return; - } - ram_addr = memory_region_get_ram_addr(mr) + addr; - tb_invalidate_phys_page_range(ram_addr, ram_addr + 1); -} - -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) -{ - /* - * There may not be a virtual to physical translation for the pc - * right now, but there may exist cached TB for this pc. - * Flush the whole TB cache to force re-translation of such TBs. - * This is heavyweight, but we're debugging anyway. - */ - tb_flush(cpu); -} -#endif - -#ifndef CONFIG_USER_ONLY /* Add a watchpoint. */ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, int flags, CPUWatchpoint **watchpoint) @@ -1117,123 +872,7 @@ int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len) } return ret; } -#endif /* !CONFIG_USER_ONLY */ -/* Add a breakpoint. */ -int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, - CPUBreakpoint **breakpoint) -{ - CPUBreakpoint *bp; - - bp = g_malloc(sizeof(*bp)); - - bp->pc = pc; - bp->flags = flags; - - /* keep all GDB-injected breakpoints in front */ - if (flags & BP_GDB) { - QTAILQ_INSERT_HEAD(&cpu->breakpoints, bp, entry); - } else { - QTAILQ_INSERT_TAIL(&cpu->breakpoints, bp, entry); - } - - breakpoint_invalidate(cpu, pc); - - if (breakpoint) { - *breakpoint = bp; - } - return 0; -} - -/* Remove a specific breakpoint. */ -int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) -{ - CPUBreakpoint *bp; - - QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) { - if (bp->pc == pc && bp->flags == flags) { - cpu_breakpoint_remove_by_ref(cpu, bp); - return 0; - } - } - return -ENOENT; -} - -/* Remove a specific breakpoint by reference. */ -void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint) -{ - QTAILQ_REMOVE(&cpu->breakpoints, breakpoint, entry); - - breakpoint_invalidate(cpu, breakpoint->pc); - - g_free(breakpoint); -} - -/* Remove all matching breakpoints. */ -void cpu_breakpoint_remove_all(CPUState *cpu, int mask) -{ - CPUBreakpoint *bp, *next; - - QTAILQ_FOREACH_SAFE(bp, &cpu->breakpoints, entry, next) { - if (bp->flags & mask) { - cpu_breakpoint_remove_by_ref(cpu, bp); - } - } -} - -/* enable or disable single step mode. EXCP_DEBUG is returned by the - CPU loop after each instruction */ -void cpu_single_step(CPUState *cpu, int enabled) -{ - if (cpu->singlestep_enabled != enabled) { - cpu->singlestep_enabled = enabled; - if (kvm_enabled()) { - kvm_update_guest_debug(cpu, 0); - } else { - /* must flush all the translated code to avoid inconsistencies */ - /* XXX: only flush what is necessary */ - tb_flush(cpu); - } - } -} - -void cpu_abort(CPUState *cpu, const char *fmt, ...) -{ - va_list ap; - va_list ap2; - - va_start(ap, fmt); - va_copy(ap2, ap); - fprintf(stderr, "qemu: fatal: "); - vfprintf(stderr, fmt, ap); - fprintf(stderr, "\n"); - cpu_dump_state(cpu, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP); - if (qemu_log_separate()) { - FILE *logfile = qemu_log_lock(); - qemu_log("qemu: fatal: "); - qemu_log_vprintf(fmt, ap2); - qemu_log("\n"); - log_cpu_state(cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP); - qemu_log_flush(); - qemu_log_unlock(logfile); - qemu_log_close(); - } - va_end(ap2); - va_end(ap); - replay_finish(); -#if defined(CONFIG_USER_ONLY) - { - struct sigaction act; - sigfillset(&act.sa_mask); - act.sa_handler = SIG_DFL; - act.sa_flags = 0; - sigaction(SIGABRT, &act, NULL); - } -#endif - abort(); -} - -#if !defined(CONFIG_USER_ONLY) /* Called from RCU critical section */ static RAMBlock *qemu_get_ram_block(ram_addr_t addr) { @@ -1420,9 +1059,6 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu, AddressSpaceDispatch *d = flatview_to_dispatch(section->fv); return section - d->map.sections; } -#endif /* defined(CONFIG_USER_ONLY) */ - -#if !defined(CONFIG_USER_ONLY) static int subpage_register(subpage_t *mmio, uint32_t start, uint32_t end, uint16_t section); @@ -3023,52 +2659,6 @@ MemoryRegion *get_system_io(void) return system_io; } -#endif /* !defined(CONFIG_USER_ONLY) */ - -/* physical memory access (slow version, mainly for debug) */ -#if defined(CONFIG_USER_ONLY) -int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, - void *ptr, target_ulong len, bool is_write) -{ - int flags; - target_ulong l, page; - void * p; - uint8_t *buf = ptr; - - while (len > 0) { - page = addr & TARGET_PAGE_MASK; - l = (page + TARGET_PAGE_SIZE) - addr; - if (l > len) - l = len; - flags = page_get_flags(page); - if (!(flags & PAGE_VALID)) - return -1; - if (is_write) { - if (!(flags & PAGE_WRITE)) - return -1; - /* XXX: this code should not depend on lock_user */ - if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) - return -1; - memcpy(p, buf, l); - unlock_user(p, addr, l); - } else { - if (!(flags & PAGE_READ)) - return -1; - /* XXX: this code should not depend on lock_user */ - if (!(p = lock_user(VERIFY_READ, addr, l, 1))) - return -1; - memcpy(buf, p, l); - unlock_user(p, addr, 0); - } - len -= l; - buf += l; - addr += l; - } - return 0; -} - -#else - static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr, hwaddr length) { @@ -3857,18 +3447,7 @@ int qemu_target_page_bits_min(void) { return TARGET_PAGE_BITS_MIN; } -#endif -bool target_words_bigendian(void) -{ -#if defined(TARGET_WORDS_BIGENDIAN) - return true; -#else - return false; -#endif -} - -#ifndef CONFIG_USER_ONLY bool cpu_physical_memory_is_io(hwaddr phys_addr) { MemoryRegion*mr; @@ -3998,23 +3577,6 @@ bool ramblock_is_pmem(RAMBlock *rb) return rb->flags & RAM_PMEM; } -#endif - -void page_size_init(void) -{ - /* NOTE: we can always suppose that qemu_host_page_size >= - TARGET_PAGE_SIZE */ - if (qemu_host_page_size == 0) { - qemu_host_page_size = qemu_real_host_page_size; - } - if (qemu_host_page_size < TARGET_PAGE_SIZE) { - qemu_host_page_size = TARGET_PAGE_SIZE; - } - qemu_host_page_mask = -(intptr_t)qemu_host_page_size; -} - -#if !defined(CONFIG_USER_ONLY) - static void mtree_print_phys_entries(int start, int end, int skip, int ptr) { if (start == end - 1) { @@ -4147,5 +3709,3 @@ bool ram_block_discard_is_required(void) { return qatomic_read(&ram_block_discard_disabled) < 0; } - -#endif From 22fb6eb571387172f41878866f4438b6bae21f0e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 7 Oct 2020 12:23:56 -0400 Subject: [PATCH 13/38] qom: fix objects with improper parent type Some objects accidentally inherit ObjectClass instead of Object. They compile silently but may crash after downcasting. In this patch, we introduce a coccinelle script to find broken declarations and fix them manually with proper base type. Signed-off-by: Sergey Nizovtsev Signed-off-by: Paolo Bonzini --- MAINTAINERS | 1 + include/hw/acpi/vmgenid.h | 2 +- include/hw/misc/vmcoreinfo.h | 2 +- include/net/can_host.h | 2 +- scripts/coccinelle/qom-parent-type.cocci | 26 ++++++++++++++++++++++++ 5 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 scripts/coccinelle/qom-parent-type.cocci diff --git a/MAINTAINERS b/MAINTAINERS index 7ef459a33c..fcb2c03c2b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2462,6 +2462,7 @@ F: include/monitor/qdev.h F: include/qom/ F: qapi/qom.json F: qapi/qdev.json +F: scripts/coccinelle/qom-parent-type.cocci F: softmmu/qdev-monitor.c F: qom/ F: tests/check-qom-interface.c diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h index d50fbacb8e..cb4ad37fc5 100644 --- a/include/hw/acpi/vmgenid.h +++ b/include/hw/acpi/vmgenid.h @@ -19,7 +19,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(VmGenIdState, VMGENID) struct VmGenIdState { - DeviceClass parent_obj; + DeviceState parent_obj; QemuUUID guid; /* The 128-bit GUID seen by the guest */ uint8_t vmgenid_addr_le[8]; /* Address of the GUID (little-endian) */ }; diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index ebada6617a..0b7b55d400 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -24,7 +24,7 @@ DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO, typedef struct fw_cfg_vmcoreinfo FWCfgVMCoreInfo; struct VMCoreInfoState { - DeviceClass parent_obj; + DeviceState parent_obj; bool has_vmcoreinfo; FWCfgVMCoreInfo vmcoreinfo; diff --git a/include/net/can_host.h b/include/net/can_host.h index 4e3ce3f954..caab71bdda 100644 --- a/include/net/can_host.h +++ b/include/net/can_host.h @@ -35,7 +35,7 @@ OBJECT_DECLARE_TYPE(CanHostState, CanHostClass, CAN_HOST) struct CanHostState { - ObjectClass oc; + Object oc; CanBusState *bus; CanBusClientState bus_client; diff --git a/scripts/coccinelle/qom-parent-type.cocci b/scripts/coccinelle/qom-parent-type.cocci new file mode 100644 index 0000000000..9afb3edd97 --- /dev/null +++ b/scripts/coccinelle/qom-parent-type.cocci @@ -0,0 +1,26 @@ +// Highlight object declarations that don't look like object class but +// accidentally inherit from it. + +@match@ +identifier obj_t, fld; +type parent_t =~ ".*Class$"; +@@ +struct obj_t { + parent_t fld; + ... +}; + +@script:python filter depends on match@ +obj_t << match.obj_t; +@@ +is_class_obj = obj_t.endswith('Class') +cocci.include_match(not is_class_obj) + +@replacement depends on filter@ +identifier match.obj_t, match.fld; +type match.parent_t; +@@ +struct obj_t { +* parent_t fld; + ... +}; From a738a50e62303f9dfb345790ffd8d01f9e20527a Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Mon, 5 Oct 2020 16:52:26 -0400 Subject: [PATCH 14/38] docs: Move QTest documentation to its own document The qtest and libqtest doc comments will be parsed to generate API documentation, so move QTest documentation to its own document where the API and format documentation and will be included. Signed-off-by: Eduardo Habkost Acked-by: Thomas Huth Message-Id: <20201005205228.697463-2-ehabkost@redhat.com> Signed-off-by: Paolo Bonzini --- docs/devel/index.rst | 1 + docs/devel/qtest.rst | 58 ++++++++++++++++++++++++++++++++++++++++++ docs/devel/testing.rst | 47 ++-------------------------------- 3 files changed, 61 insertions(+), 45 deletions(-) create mode 100644 docs/devel/qtest.rst diff --git a/docs/devel/index.rst b/docs/devel/index.rst index 5fda2d3509..77baae5c77 100644 --- a/docs/devel/index.rst +++ b/docs/devel/index.rst @@ -21,6 +21,7 @@ Contents: atomics stable-process testing + qtest decodetree secure-coding-practices tcg diff --git a/docs/devel/qtest.rst b/docs/devel/qtest.rst new file mode 100644 index 0000000000..86dec84a0b --- /dev/null +++ b/docs/devel/qtest.rst @@ -0,0 +1,58 @@ +======================================== +QTest Device Emulation Testing Framework +======================================== + +QTest is a device emulation testing framework. It can be very useful to test +device models; it could also control certain aspects of QEMU (such as virtual +clock stepping), with a special purpose "qtest" protocol. Refer to the +documentation in ``qtest.c`` for more details of the protocol. + +QTest cases can be executed with + +.. code:: + + make check-qtest + +The QTest library is implemented by ``tests/qtest/libqtest.c`` and the API is +defined in ``tests/qtest/libqtest.h``. + +Consider adding a new QTest case when you are introducing a new virtual +hardware, or extending one if you are adding functionalities to an existing +virtual device. + +On top of libqtest, a higher level library, ``libqos``, was created to +encapsulate common tasks of device drivers, such as memory management and +communicating with system buses or devices. Many virtual device tests use +libqos instead of directly calling into libqtest. + +Steps to add a new QTest case are: + +1. Create a new source file for the test. (More than one file can be added as + necessary.) For example, ``tests/qtest/foo-test.c``. + +2. Write the test code with the glib and libqtest/libqos API. See also existing + tests and the library headers for reference. + +3. Register the new test in ``tests/qtest/Makefile.include``. Add the test + executable name to an appropriate ``check-qtest-*-y`` variable. For example: + + ``check-qtest-generic-y = tests/qtest/foo-test$(EXESUF)`` + +4. Add object dependencies of the executable in the Makefile, including the + test source file(s) and other interesting objects. For example: + + ``tests/qtest/foo-test$(EXESUF): tests/qtest/foo-test.o $(libqos-obj-y)`` + +Debugging a QTest failure is slightly harder than the unit test because the +tests look up QEMU program names in the environment variables, such as +``QTEST_QEMU_BINARY`` and ``QTEST_QEMU_IMG``, and also because it is not easy +to attach gdb to the QEMU process spawned from the test. But manual invoking +and using gdb on the test is still simple to do: find out the actual command +from the output of + +.. code:: + + make check-qtest V=1 + +which you can run manually. + diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 8875a40a2b..e58389b29f 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -70,8 +70,8 @@ QTest QTest is a device emulation testing framework. It can be very useful to test device models; it could also control certain aspects of QEMU (such as virtual -clock stepping), with a special purpose "qtest" protocol. Refer to the -documentation in ``qtest.c`` for more details of the protocol. +clock stepping), with a special purpose "qtest" protocol. Refer to +:doc:`qtest` for more details. QTest cases can be executed with @@ -79,49 +79,6 @@ QTest cases can be executed with make check-qtest -The QTest library is implemented by ``tests/qtest/libqtest.c`` and the API is -defined in ``tests/qtest/libqtest.h``. - -Consider adding a new QTest case when you are introducing a new virtual -hardware, or extending one if you are adding functionalities to an existing -virtual device. - -On top of libqtest, a higher level library, ``libqos``, was created to -encapsulate common tasks of device drivers, such as memory management and -communicating with system buses or devices. Many virtual device tests use -libqos instead of directly calling into libqtest. - -Steps to add a new QTest case are: - -1. Create a new source file for the test. (More than one file can be added as - necessary.) For example, ``tests/qtest/foo-test.c``. - -2. Write the test code with the glib and libqtest/libqos API. See also existing - tests and the library headers for reference. - -3. Register the new test in ``tests/qtest/Makefile.include``. Add the test - executable name to an appropriate ``check-qtest-*-y`` variable. For example: - - ``check-qtest-generic-y = tests/qtest/foo-test$(EXESUF)`` - -4. Add object dependencies of the executable in the Makefile, including the - test source file(s) and other interesting objects. For example: - - ``tests/qtest/foo-test$(EXESUF): tests/qtest/foo-test.o $(libqos-obj-y)`` - -Debugging a QTest failure is slightly harder than the unit test because the -tests look up QEMU program names in the environment variables, such as -``QTEST_QEMU_BINARY`` and ``QTEST_QEMU_IMG``, and also because it is not easy -to attach gdb to the QEMU process spawned from the test. But manual invoking -and using gdb on the test is still simple to do: find out the actual command -from the output of - -.. code:: - - make check-qtest V=1 - -which you can run manually. - QAPI schema tests ----------------- From f59c6de7f00e664358a9aad49248094ab1b29461 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Mon, 5 Oct 2020 16:52:27 -0400 Subject: [PATCH 15/38] docs/devel/qtest: Include protocol spec in document Include the QTest Protocol doc string in docs/devel/qtest.rst, after converting it to use Sphinx syntax. Signed-off-by: Eduardo Habkost Acked-by: Thomas Huth Message-Id: <20201005205228.697463-3-ehabkost@redhat.com> Signed-off-by: Paolo Bonzini --- docs/devel/qtest.rst | 12 ++++++-- softmmu/qtest.c | 71 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/docs/devel/qtest.rst b/docs/devel/qtest.rst index 86dec84a0b..3bf9ebee7f 100644 --- a/docs/devel/qtest.rst +++ b/docs/devel/qtest.rst @@ -4,8 +4,8 @@ QTest Device Emulation Testing Framework QTest is a device emulation testing framework. It can be very useful to test device models; it could also control certain aspects of QEMU (such as virtual -clock stepping), with a special purpose "qtest" protocol. Refer to the -documentation in ``qtest.c`` for more details of the protocol. +clock stepping), with a special purpose "qtest" protocol. Refer to +:ref:`qtest-protocol` for more details of the protocol. QTest cases can be executed with @@ -56,3 +56,11 @@ from the output of which you can run manually. + +.. _qtest-protocol: + +QTest Protocol +-------------- + +.. kernel-doc:: softmmu/qtest.c + :doc: QTest Protocol diff --git a/softmmu/qtest.c b/softmmu/qtest.c index 0d43cf8883..2c6e8dc858 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -49,92 +49,139 @@ static void *qtest_server_send_opaque; #define FMT_timeval "%ld.%06ld" /** - * QTest Protocol + * DOC: QTest Protocol * * Line based protocol, request/response based. Server can send async messages * so clients should always handle many async messages before the response * comes in. * * Valid requests + * ^^^^^^^^^^^^^^ * * Clock management: + * """"""""""""""""" * * The qtest client is completely in charge of the QEMU_CLOCK_VIRTUAL. qtest commands * let you adjust the value of the clock (monotonically). All the commands * return the current value of the clock in nanoseconds. * + * .. code-block:: none + * * > clock_step * < OK VALUE * - * Advance the clock to the next deadline. Useful when waiting for - * asynchronous events. + * Advance the clock to the next deadline. Useful when waiting for + * asynchronous events. + * + * .. code-block:: none * * > clock_step NS * < OK VALUE * - * Advance the clock by NS nanoseconds. + * Advance the clock by NS nanoseconds. + * + * .. code-block:: none * * > clock_set NS * < OK VALUE * - * Advance the clock to NS nanoseconds (do nothing if it's already past). + * Advance the clock to NS nanoseconds (do nothing if it's already past). * * PIO and memory access: + * """""""""""""""""""""" + * + * .. code-block:: none * * > outb ADDR VALUE * < OK * + * .. code-block:: none + * * > outw ADDR VALUE * < OK * + * .. code-block:: none + * * > outl ADDR VALUE * < OK * + * .. code-block:: none + * * > inb ADDR * < OK VALUE * + * .. code-block:: none + * * > inw ADDR * < OK VALUE * + * .. code-block:: none + * * > inl ADDR * < OK VALUE * + * .. code-block:: none + * * > writeb ADDR VALUE * < OK * + * .. code-block:: none + * * > writew ADDR VALUE * < OK * + * .. code-block:: none + * * > writel ADDR VALUE * < OK * + * .. code-block:: none + * * > writeq ADDR VALUE * < OK * + * .. code-block:: none + * * > readb ADDR * < OK VALUE * + * .. code-block:: none + * * > readw ADDR * < OK VALUE * + * .. code-block:: none + * * > readl ADDR * < OK VALUE * + * .. code-block:: none + * * > readq ADDR * < OK VALUE * + * .. code-block:: none + * * > read ADDR SIZE * < OK DATA * + * .. code-block:: none + * * > write ADDR SIZE DATA * < OK * + * .. code-block:: none + * * > b64read ADDR SIZE * < OK B64_DATA * + * .. code-block:: none + * * > b64write ADDR SIZE B64_DATA * < OK * + * .. code-block:: none + * * > memset ADDR SIZE VALUE * < OK * @@ -149,16 +196,21 @@ static void *qtest_server_send_opaque; * If the sizes do not match, the data will be truncated. * * IRQ management: + * """"""""""""""" + * + * .. code-block:: none * * > irq_intercept_in QOM-PATH * < OK * + * .. code-block:: none + * * > irq_intercept_out QOM-PATH * < OK * * Attach to the gpio-in (resp. gpio-out) pins exported by the device at * QOM-PATH. When the pin is triggered, one of the following async messages - * will be printed to the qtest stream: + * will be printed to the qtest stream:: * * IRQ raise NUM * IRQ lower NUM @@ -168,12 +220,15 @@ static void *qtest_server_send_opaque; * NUM=0 even though it is remapped to GSI 2). * * Setting interrupt level: + * """""""""""""""""""""""" + * + * .. code-block:: none * * > set_irq_in QOM-PATH NAME NUM LEVEL * < OK * - * where NAME is the name of the irq/gpio list, NUM is an IRQ number and - * LEVEL is an signed integer IRQ level. + * where NAME is the name of the irq/gpio list, NUM is an IRQ number and + * LEVEL is an signed integer IRQ level. * * Forcibly set the given interrupt pin to the given level. * From 51c778edd347452ce22146bd004bc7adab5f3a1a Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Mon, 5 Oct 2020 16:52:28 -0400 Subject: [PATCH 16/38] docs/devel/qtest: Include libqtest API reference Signed-off-by: Eduardo Habkost Acked-by: Thomas Huth Message-Id: <20201005205228.697463-4-ehabkost@redhat.com> Signed-off-by: Paolo Bonzini --- docs/devel/qtest.rst | 6 ++++++ tests/qtest/libqos/libqtest.h | 20 ++++++++++---------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/devel/qtest.rst b/docs/devel/qtest.rst index 3bf9ebee7f..075fe5f7d5 100644 --- a/docs/devel/qtest.rst +++ b/docs/devel/qtest.rst @@ -64,3 +64,9 @@ QTest Protocol .. kernel-doc:: softmmu/qtest.c :doc: QTest Protocol + + +libqtest API reference +---------------------- + +.. kernel-doc:: tests/qtest/libqos/libqtest.h diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h index a6ee1654f2..209fcf6973 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -24,7 +24,7 @@ typedef struct QTestState QTestState; /** * qtest_initf: - * @fmt...: Format for creating other arguments to pass to QEMU, formatted + * @fmt: Format for creating other arguments to pass to QEMU, formatted * like sprintf(). * * Convenience wrapper around qtest_init(). @@ -87,7 +87,7 @@ void qtest_quit(QTestState *s); * @s: #QTestState instance to operate on. * @fds: array of file descriptors * @fds_num: number of elements in @fds - * @fmt...: QMP message to send to qemu, formatted like + * @fmt: QMP message to send to qemu, formatted like * qobject_from_jsonf_nofail(). See parse_escape() for what's * supported after '%'. * @@ -100,7 +100,7 @@ QDict *qtest_qmp_fds(QTestState *s, int *fds, size_t fds_num, /** * qtest_qmp: * @s: #QTestState instance to operate on. - * @fmt...: QMP message to send to qemu, formatted like + * @fmt: QMP message to send to qemu, formatted like * qobject_from_jsonf_nofail(). See parse_escape() for what's * supported after '%'. * @@ -112,7 +112,7 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...) /** * qtest_qmp_send: * @s: #QTestState instance to operate on. - * @fmt...: QMP message to send to qemu, formatted like + * @fmt: QMP message to send to qemu, formatted like * qobject_from_jsonf_nofail(). See parse_escape() for what's * supported after '%'. * @@ -124,7 +124,7 @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...) /** * qtest_qmp_send_raw: * @s: #QTestState instance to operate on. - * @fmt...: text to send, formatted like sprintf() + * @fmt: text to send, formatted like sprintf() * * Sends text to the QMP monitor verbatim. Need not be valid JSON; * this is useful for negative tests. @@ -201,7 +201,7 @@ QDict *qtest_qmp_receive(QTestState *s); /** * qtest_qmp_eventwait: * @s: #QTestState instance to operate on. - * @s: #event event to wait for. + * @event: event to wait for. * * Continuously polls for QMP responses until it receives the desired event. */ @@ -210,7 +210,7 @@ void qtest_qmp_eventwait(QTestState *s, const char *event); /** * qtest_qmp_eventwait_ref: * @s: #QTestState instance to operate on. - * @s: #event event to wait for. + * @event: event to wait for. * * Continuously polls for QMP responses until it receives the desired event. * Returns a copy of the event for further investigation. @@ -237,7 +237,7 @@ QDict *qtest_qmp_receive_success(QTestState *s, /** * qtest_hmp: * @s: #QTestState instance to operate on. - * @fmt...: HMP command to send to QEMU, formats arguments like sprintf(). + * @fmt: HMP command to send to QEMU, formats arguments like sprintf(). * * Send HMP command to QEMU via QMP's human-monitor-command. * QMP events are discarded. @@ -629,7 +629,7 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data); /** * qtest_qmp_assert_success: * @qts: QTestState instance to operate on - * @fmt...: QMP message to send to qemu, formatted like + * @fmt: QMP message to send to qemu, formatted like * qobject_from_jsonf_nofail(). See parse_escape() for what's * supported after '%'. * @@ -676,7 +676,7 @@ void qtest_qmp_device_add_qdict(QTestState *qts, const char *drv, * @qts: QTestState instance to operate on * @driver: Name of the device that should be added * @id: Identification string - * @fmt...: QMP message to send to qemu, formatted like + * @fmt: QMP message to send to qemu, formatted like * qobject_from_jsonf_nofail(). See parse_escape() for what's * supported after '%'. * From da00d067eaf24968cc5bf7c8065c7b8b1070e16c Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 08:44:00 -0400 Subject: [PATCH 17/38] qtest: unify extra_qtest_srcs and extra_qtest_deps Currently the extra sources and extra dependencies of qtests are held in two separate dictionaries. Use the same trick as tests/meson.build to combine them into one. This will make it easier to update the documentation for unit tests and qtests. Signed-off-by: Paolo Bonzini --- tests/qtest/meson.build | 61 ++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 0f32ca0895..28bafc02a2 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -193,34 +193,24 @@ qos_test_ss.add( qos_test_ss.add(when: 'CONFIG_VIRTFS', if_true: files('virtio-9p-test.c')) qos_test_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-test.c')) -extra_qtest_deps = { - 'bios-tables-test': [io], - 'ivshmem-test': [rt], - 'qos-test': [chardev, io], - 'tpm-crb-swtpm-test': [io], - 'tpm-crb-test': [io], - 'tpm-tis-swtpm-test': [io], - 'tpm-tis-test': [io], - 'tpm-tis-device-swtpm-test': [io], - 'tpm-tis-device-test': [io], -} -extra_qtest_srcs = { - 'bios-tables-test': files('boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'), - 'pxe-test': files('boot-sector.c'), - 'cdrom-test': files('boot-sector.c'), - 'migration-test': files('migration-helpers.c'), - 'ivshmem-test': files('../../contrib/ivshmem-server/ivshmem-server.c'), - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, - 'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'), - 'tpm-crb-swtpm-test': files('tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'), - 'tpm-crb-test': files('tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'), - 'tpm-tis-device-swtpm-test': files('tpm-emu.c', 'tpm-util.c', 'tpm-tis-util.c', 'tpm-tests.c'), - 'tpm-tis-device-test': files('tpm-emu.c', 'tpm-util.c', 'tpm-tis-util.c', 'tpm-tests.c'), - 'tpm-tis-swtpm-test': files('tpm-emu.c', 'tpm-util.c', 'tpm-tis-util.c', 'tpm-tests.c'), - 'tpm-tis-test': files('tpm-emu.c', 'tpm-util.c', 'tpm-tis-util.c', 'tpm-tests.c'), - 'qos-test': qos_test_ss.apply(config_host, strict: false).sources() -} +tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] +qtests = { + 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], + 'cdrom-test': files('boot-sector.c'), + 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, + 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], + 'migration-test': files('migration-helpers.c'), + 'pxe-test': files('boot-sector.c'), + 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], + 'tpm-crb-swtpm-test': [io, tpmemu_files], + 'tpm-crb-test': [io, tpmemu_files], + 'tpm-tis-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'], + 'tpm-tis-test': [io, tpmemu_files, 'tpm-tis-util.c'], + 'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'], + 'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'], + 'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'), +} qtest_executables = {} foreach dir : target_dirs @@ -230,7 +220,7 @@ foreach dir : target_dirs target_base = dir.split('-')[0] qtest_emulator = emulators['qemu-system-' + target_base] - qtests = get_variable('qtests_' + target_base, []) + qtests_generic + target_qtests = get_variable('qtests_' + target_base, []) + qtests_generic test_deps = [] qtest_env = environment() @@ -241,14 +231,21 @@ foreach dir : target_dirs 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) - foreach test : qtests + foreach test : target_qtests # Executables are shared across targets, declare them only the first time we # encounter them if not qtest_executables.has_key(test) + src = [test + '.c'] + deps = [qemuutil, qos] + if test in qtests + # use a sourceset to quickly separate sources and deps + test_ss = ss.source_set() + test_ss.add(qtests[test]) + src += test_ss.all_sources() + deps += test_ss.all_dependencies() + endif qtest_executables += { - test: executable(test, - files(test + '.c') + extra_qtest_srcs.get(test, []), - dependencies: [qemuutil, qos] + extra_qtest_deps.get(test, [])) + test: executable(test, src, dependencies: deps) } endif # FIXME: missing dependency on the emulator binary and qemu-img From bab88ead6fcbc7097ed75981622cce7850da1cc7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 08:49:55 -0400 Subject: [PATCH 18/38] docs/devel: update instruction on how to add new unit tests Signed-off-by: Paolo Bonzini --- docs/devel/qtest.rst | 24 ++++++++++++++++++------ docs/devel/testing.rst | 17 +++++++++-------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/docs/devel/qtest.rst b/docs/devel/qtest.rst index 075fe5f7d5..97c5a75626 100644 --- a/docs/devel/qtest.rst +++ b/docs/devel/qtest.rst @@ -33,15 +33,27 @@ Steps to add a new QTest case are: 2. Write the test code with the glib and libqtest/libqos API. See also existing tests and the library headers for reference. -3. Register the new test in ``tests/qtest/Makefile.include``. Add the test - executable name to an appropriate ``check-qtest-*-y`` variable. For example: +3. Register the new test in ``tests/qtest/meson.build``. Add the test + executable name to an appropriate ``qtests_*`` variable. There is + one variable per architecture, plus ``qtests_generic`` for tests + that can be run for all architectures. For example:: - ``check-qtest-generic-y = tests/qtest/foo-test$(EXESUF)`` + qtests_generic = [ + ... + 'foo-test', + ... + ] -4. Add object dependencies of the executable in the Makefile, including the - test source file(s) and other interesting objects. For example: +4. If the test has more than one source file or needs to be linked with any + dependency other than ``qemuutil`` and ``qos``, list them in the ``qtests`` + dictionary. For example a test that needs to use the ``QIO`` library + will have an entry like:: - ``tests/qtest/foo-test$(EXESUF): tests/qtest/foo-test.o $(libqos-obj-y)`` + { + ... + 'foo-test': [io], + ... + } Debugging a QTest failure is slightly harder than the unit test because the tests look up QEMU program names in the environment variables, such as diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index e58389b29f..0c3e79d31c 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -41,15 +41,16 @@ add a new unit test: test. The test code should be organized with the glib testing framework. Copying and modifying an existing test is usually a good idea. -3. Add the test to ``tests/Makefile.include``. First, name the unit test - program and add it to ``$(check-unit-y)``; then add a rule to build the - executable. For example: +3. Add the test to ``tests/meson.build``. The unit tests are listed in a + dictionary called ``tests``. The values are any additional sources and + dependencies to be linked with the test. For a simple test whose source + is in ``tests/foo-test.c``, it is enough to add an entry like:: -.. code:: - - check-unit-y += tests/foo-test$(EXESUF) - tests/foo-test$(EXESUF): tests/foo-test.o $(test-util-obj-y) - ... + { + ... + 'foo-test': [], + ... + } Since unit tests don't require environment variables, the simplest way to debug a unit test failure is often directly invoking it or even running it under From 242fb750bd3246cd17eea1455eb7c898a4843e77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Tue, 29 Sep 2020 18:36:54 +0400 Subject: [PATCH 19/38] build-sys: fix git version from -version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typo introduced with the script. Fixes: 2c273f32d3 ("meson: generate qemu-version.h") Signed-off-by: Marc-André Lureau Reported-by: Laszlo Ersek Reviewed-by: Eric Blake Tested-by: Laszlo Ersek Message-Id: <20200929143654.518157-1-marcandre.lureau@redhat.com> Signed-off-by: Paolo Bonzini --- scripts/qemu-version.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qemu-version.sh b/scripts/qemu-version.sh index 03128c56a2..3f6e7e6d41 100755 --- a/scripts/qemu-version.sh +++ b/scripts/qemu-version.sh @@ -9,7 +9,7 @@ version="$3" if [ -z "$pkgversion" ]; then cd "$dir" if [ -e .git ]; then - pkgversion=$(git describe --match 'v*' --dirty | echo "") + pkgversion=$(git describe --match 'v*' --dirty) || : fi fi From fbc5884ce2774232ea5a1a7d9b4e605aefc16a2b Mon Sep 17 00:00:00 2001 From: Huacai Chen Date: Wed, 7 Oct 2020 16:39:28 +0800 Subject: [PATCH 20/38] meson.build: Re-enable KVM support for MIPS After converting from configure to meson, KVM support is lost for MIPS, so re-enable it in meson.build. Fixes: fdb75aeff7c212e1afaaa3a43 ("configure: remove target configuration") Fixes: 8a19980e3fc42239aae054bc9 ("configure: move accelerator logic to meson") Cc: aolo Bonzini Signed-off-by: Huacai Chen Message-Id: <1602059975-10115-3-git-send-email-chenhc@lemote.com> Signed-off-by: Paolo Bonzini --- meson.build | 2 ++ 1 file changed, 2 insertions(+) diff --git a/meson.build b/meson.build index 26230614ba..0c676af194 100644 --- a/meson.build +++ b/meson.build @@ -59,6 +59,8 @@ elif cpu == 's390x' kvm_targets = ['s390x-softmmu'] elif cpu in ['ppc', 'ppc64'] kvm_targets = ['ppc-softmmu', 'ppc64-softmmu'] +elif cpu in ['mips', 'mips64'] + kvm_targets = ['mips-softmmu', 'mipsel-softmmu', 'mips64-softmmu', 'mips64el-softmmu'] else kvm_targets = [] endif From 1c3e2a38de4e3094dfaf1e4dd73b1e5a91df8fe9 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 14:38:52 +0200 Subject: [PATCH 21/38] qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict In the next patch a new version of qtest_qmp_receive will be reintroduced that will buffer received qmp events for later consumption in qtest_qmp_eventwait_ref No functional change intended. Suggested-by: Paolo Bonzini Signed-off-by: Maxim Levitsky Signed-off-by: Paolo Bonzini --- tests/qtest/ahci-test.c | 4 ++-- tests/qtest/device-plug-test.c | 2 +- tests/qtest/drive_del-test.c | 2 +- tests/qtest/libqos/libqtest.h | 4 ++-- tests/qtest/libqtest.c | 16 ++++++++-------- tests/qtest/pvpanic-test.c | 2 +- tests/qtest/qmp-test.c | 18 +++++++++--------- 7 files changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c index 5e1954852e..d42ebaeb4c 100644 --- a/tests/qtest/ahci-test.c +++ b/tests/qtest/ahci-test.c @@ -1590,7 +1590,7 @@ static void test_atapi_tray(void) qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-open-tray', " "'arguments': {'id': 'cd0'}}"); atapi_wait_tray(ahci, true); - rsp = qtest_qmp_receive(ahci->parent->qts); + rsp = qtest_qmp_receive_dict(ahci->parent->qts); qobject_unref(rsp); qmp_discard_response(ahci->parent->qts, @@ -1620,7 +1620,7 @@ static void test_atapi_tray(void) qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', " "'arguments': {'id': 'cd0'}}"); atapi_wait_tray(ahci, false); - rsp = qtest_qmp_receive(ahci->parent->qts); + rsp = qtest_qmp_receive_dict(ahci->parent->qts); qobject_unref(rsp); /* Now, to convince ATAPI we understand the media has changed... */ diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c index 9214892741..a2247856be 100644 --- a/tests/qtest/device-plug-test.c +++ b/tests/qtest/device-plug-test.c @@ -23,7 +23,7 @@ static void device_del_start(QTestState *qtest, const char *id) static void device_del_finish(QTestState *qtest) { - QDict *resp = qtest_qmp_receive(qtest); + QDict *resp = qtest_qmp_receive_dict(qtest); g_assert(qdict_haskey(resp, "return")); qobject_unref(resp); diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c index 2d765865ce..ba0cd77445 100644 --- a/tests/qtest/drive_del-test.c +++ b/tests/qtest/drive_del-test.c @@ -41,7 +41,7 @@ static void device_del(QTestState *qts) /* Complication: ignore DEVICE_DELETED event */ qmp_discard_response(qts, "{'execute': 'device_del'," " 'arguments': { 'id': 'dev0' } }"); - response = qtest_qmp_receive(qts); + response = qtest_qmp_receive_dict(qts); g_assert(response); g_assert(qdict_haskey(response, "return")); qobject_unref(response); diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h index 209fcf6973..9b3f99b322 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -191,12 +191,12 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0); /** - * qtest_receive: + * qtest_qmp_receive_dict: * @s: #QTestState instance to operate on. * * Reads a QMP message from QEMU and returns the response. */ -QDict *qtest_qmp_receive(QTestState *s); +QDict *qtest_qmp_receive_dict(QTestState *s); /** * qtest_qmp_eventwait: diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 58f58e1ece..dadc465825 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -322,7 +322,7 @@ QTestState *qtest_init(const char *extra_args) QDict *greeting; /* Read the QMP greeting and then do the handshake */ - greeting = qtest_qmp_receive(s); + greeting = qtest_qmp_receive_dict(s); qobject_unref(greeting); qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }")); @@ -603,7 +603,7 @@ QDict *qmp_fd_receive(int fd) return qmp.response; } -QDict *qtest_qmp_receive(QTestState *s) +QDict *qtest_qmp_receive_dict(QTestState *s) { return qmp_fd_receive(s->qmp_fd); } @@ -678,7 +678,7 @@ QDict *qtest_vqmp_fds(QTestState *s, int *fds, size_t fds_num, qtest_qmp_vsend_fds(s, fds, fds_num, fmt, ap); /* Receive reply */ - return qtest_qmp_receive(s); + return qtest_qmp_receive_dict(s); } QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap) @@ -686,7 +686,7 @@ QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap) qtest_qmp_vsend(s, fmt, ap); /* Receive reply */ - return qtest_qmp_receive(s); + return qtest_qmp_receive_dict(s); } QDict *qmp_fd(int fd, const char *fmt, ...) @@ -776,7 +776,7 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event) QDict *response; for (;;) { - response = qtest_qmp_receive(s); + response = qtest_qmp_receive_dict(s); if ((qdict_haskey(response, "event")) && (strcmp(qdict_get_str(response, "event"), event) == 0)) { return response; @@ -807,7 +807,7 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap) while (ret == NULL && qdict_get_try_str(resp, "event")) { /* Ignore asynchronous QMP events */ qobject_unref(resp); - resp = qtest_qmp_receive(s); + resp = qtest_qmp_receive_dict(s); ret = g_strdup(qdict_get_try_str(resp, "return")); } g_assert(ret); @@ -1255,7 +1255,7 @@ QDict *qtest_qmp_receive_success(QTestState *s, const char *event; for (;;) { - response = qtest_qmp_receive(s); + response = qtest_qmp_receive_dict(s); g_assert(!qdict_haskey(response, "error")); ret = qdict_get_qdict(response, "return"); if (ret) { @@ -1345,7 +1345,7 @@ void qtest_qmp_device_del(QTestState *qts, const char *id) rsp = qtest_qmp_receive_success(qts, device_deleted_cb, &got_event); qobject_unref(rsp); if (!got_event) { - rsp = qtest_qmp_receive(qts); + rsp = qtest_qmp_receive_dict(qts); g_assert_cmpstr(qdict_get_try_str(rsp, "event"), ==, "DEVICE_DELETED"); qobject_unref(rsp); diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c index e57639481e..b0b20ad340 100644 --- a/tests/qtest/pvpanic-test.c +++ b/tests/qtest/pvpanic-test.c @@ -24,7 +24,7 @@ static void test_panic(void) qtest_outb(qts, 0x505, 0x1); - response = qtest_qmp_receive(qts); + response = qtest_qmp_receive_dict(qts); g_assert(qdict_haskey(response, "event")); g_assert_cmpstr(qdict_get_str(response, "event"), ==, "GUEST_PANICKED"); g_assert(qdict_haskey(response, "data")); diff --git a/tests/qtest/qmp-test.c b/tests/qtest/qmp-test.c index e1032c5a21..eb1cd8abb8 100644 --- a/tests/qtest/qmp-test.c +++ b/tests/qtest/qmp-test.c @@ -47,37 +47,37 @@ static void test_malformed(QTestState *qts) /* syntax error */ qtest_qmp_send_raw(qts, "{]\n"); - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); qmp_expect_error_and_unref(resp, "GenericError"); assert_recovered(qts); /* lexical error: impossible byte outside string */ qtest_qmp_send_raw(qts, "{\xFF"); - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); qmp_expect_error_and_unref(resp, "GenericError"); assert_recovered(qts); /* lexical error: funny control character outside string */ qtest_qmp_send_raw(qts, "{\x01"); - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); qmp_expect_error_and_unref(resp, "GenericError"); assert_recovered(qts); /* lexical error: impossible byte in string */ qtest_qmp_send_raw(qts, "{'bad \xFF"); - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); qmp_expect_error_and_unref(resp, "GenericError"); assert_recovered(qts); /* lexical error: control character in string */ qtest_qmp_send_raw(qts, "{'execute': 'nonexistent', 'id':'\n"); - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); qmp_expect_error_and_unref(resp, "GenericError"); assert_recovered(qts); /* lexical error: interpolation */ qtest_qmp_send_raw(qts, "%%p"); - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); qmp_expect_error_and_unref(resp, "GenericError"); assert_recovered(qts); @@ -111,7 +111,7 @@ static void test_qmp_protocol(void) qts = qtest_init_without_qmp_handshake(common_args); /* Test greeting */ - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); q = qdict_get_qdict(resp, "QMP"); g_assert(q); test_version(qdict_get(q, "version")); @@ -205,7 +205,7 @@ static void send_oob_cmd_that_fails(QTestState *s, const char *id) static void recv_cmd_id(QTestState *s, const char *id) { - QDict *resp = qtest_qmp_receive(s); + QDict *resp = qtest_qmp_receive_dict(s); g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, id); qobject_unref(resp); @@ -222,7 +222,7 @@ static void test_qmp_oob(void) qts = qtest_init_without_qmp_handshake(common_args); /* Check the greeting message. */ - resp = qtest_qmp_receive(qts); + resp = qtest_qmp_receive_dict(qts); q = qdict_get_qdict(resp, "QMP"); g_assert(q); capabilities = qdict_get_qlist(q, "capabilities"); From c22045bfe6d5ceebd414ff53ff23fff7ad5930d1 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:38:53 +0300 Subject: [PATCH 22/38] qtest: Reintroduce qtest_qmp_receive with QMP event buffering The new qtest_qmp_receive buffers all the received qmp events, allowing qtest_qmp_eventwait_ref to return them. This is intended to solve the race in regard to ordering of qmp events vs qmp responses, as soon as the callers start using the new interface. In addition to that, define qtest_qmp_event_ref a function which only scans the buffer that qtest_qmp_receive stores the events to. This is intended for callers that are only interested in events that were received during the last call to the qtest_qmp_receive. Suggested-by: Paolo Bonzini Signed-off-by: Maxim Levitsky Message-Id: <20201006123904.610658-3-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- tests/qtest/libqos/libqtest.h | 23 ++++++++++++++++ tests/qtest/libqtest.c | 49 ++++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h index 9b3f99b322..b7a776068c 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -198,6 +198,16 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap) */ QDict *qtest_qmp_receive_dict(QTestState *s); +/** + * qtest_qmp_receive: + * @s: #QTestState instance to operate on. + * + * Reads a QMP message from QEMU and returns the response. + * Buffers all the events received meanwhile, until a + * call to qtest_qmp_eventwait + */ +QDict *qtest_qmp_receive(QTestState *s); + /** * qtest_qmp_eventwait: * @s: #QTestState instance to operate on. @@ -217,6 +227,19 @@ void qtest_qmp_eventwait(QTestState *s, const char *event); */ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event); +/** + * qtest_qmp_event_ref: + * @s: #QTestState instance to operate on. + * @event: event to return. + * + * Removes non-matching events from the buffer that was set by + * qtest_qmp_receive, until an event bearing the given name is found, + * and returns it. + * If no event matches, clears the buffer and returns NULL. + * + */ +QDict *qtest_qmp_event_ref(QTestState *s, const char *event); + /** * qtest_qmp_receive_success: * @s: #QTestState instance to operate on diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index dadc465825..d4c49a52ff 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -63,6 +63,7 @@ struct QTestState bool irq_level[MAX_IRQ]; GString *rx; QTestTransportOps ops; + GList *pending_events; }; static GHookList abrt_hooks; @@ -279,6 +280,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) g_test_message("starting QEMU: %s", command); + s->pending_events = NULL; s->wstatus = 0; s->expected_status = 0; s->qemu_pid = fork(); @@ -386,6 +388,13 @@ void qtest_quit(QTestState *s) close(s->fd); close(s->qmp_fd); g_string_free(s->rx, true); + + for (GList *it = s->pending_events; it != NULL; it = it->next) { + qobject_unref((QDict *)it->data); + } + + g_list_free(s->pending_events); + g_free(s); } @@ -603,6 +612,19 @@ QDict *qmp_fd_receive(int fd) return qmp.response; } +QDict *qtest_qmp_receive(QTestState *s) +{ + while (true) { + QDict *response = qtest_qmp_receive_dict(s); + + if (!qdict_get_try_str(response, "event")) { + return response; + } + /* Stash the event for a later consumption */ + s->pending_events = g_list_prepend(s->pending_events, response); + } +} + QDict *qtest_qmp_receive_dict(QTestState *s) { return qmp_fd_receive(s->qmp_fd); @@ -771,9 +793,33 @@ void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...) va_end(ap); } +QDict *qtest_qmp_event_ref(QTestState *s, const char *event) +{ + GList *next = NULL; + QDict *response; + + for (GList *it = s->pending_events; it != NULL; it = next) { + + next = it->next; + response = (QDict *)it->data; + + s->pending_events = g_list_remove_link(s->pending_events, it); + + if (!strcmp(qdict_get_str(response, "event"), event)) { + return response; + } + qobject_unref(response); + } + return NULL; +} + QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event) { - QDict *response; + QDict *response = qtest_qmp_event_ref(s, event); + + if (response) { + return response; + } for (;;) { response = qtest_qmp_receive_dict(s); @@ -1403,6 +1449,7 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch, { QTestState *qts; qts = g_new0(QTestState, 1); + qts->pending_events = NULL; *s = qts; /* Expose qts early on, since the query endianness relies on it */ qts->wstatus = 0; for (int i = 0; i < MAX_IRQ; i++) { From 5e34005571af53b73e4a10cb2c6e0712cf6b8d2c Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 08:59:32 -0400 Subject: [PATCH 23/38] qtest: remove qtest_qmp_receive_success The purpose of qtest_qmp_receive_success was mostly to process events that arrived between the issueing of a command and the "return" line from QMP. This is now handled by the buffering of events that libqtest performs automatically. Signed-off-by: Paolo Bonzini Signed-off-by: Maxim Levitsky --- tests/qtest/libqos/libqtest.h | 17 ----------- tests/qtest/libqtest.c | 53 ++++----------------------------- tests/qtest/migration-helpers.c | 25 ++++++++++++---- 3 files changed, 25 insertions(+), 70 deletions(-) diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h index b7a776068c..5c959f1853 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -240,23 +240,6 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event); */ QDict *qtest_qmp_event_ref(QTestState *s, const char *event); -/** - * qtest_qmp_receive_success: - * @s: #QTestState instance to operate on - * @event_cb: Event callback - * @opaque: Argument for @event_cb - * - * Poll QMP messages until a command success response is received. - * If @event_cb, call it for each event received, passing @opaque, - * the event's name and data. - * Return the success response's "return" member. - */ -QDict *qtest_qmp_receive_success(QTestState *s, - void (*event_cb)(void *opaque, - const char *name, - QDict *data), - void *opaque); - /** * qtest_hmp: * @s: #QTestState instance to operate on. diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index d4c49a52ff..baac667b8d 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -1291,35 +1291,6 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine), qobject_unref(response); } -QDict *qtest_qmp_receive_success(QTestState *s, - void (*event_cb)(void *opaque, - const char *event, - QDict *data), - void *opaque) -{ - QDict *response, *ret, *data; - const char *event; - - for (;;) { - response = qtest_qmp_receive_dict(s); - g_assert(!qdict_haskey(response, "error")); - ret = qdict_get_qdict(response, "return"); - if (ret) { - break; - } - event = qdict_get_str(response, "event"); - data = qdict_get_qdict(response, "data"); - if (event_cb) { - event_cb(opaque, event, data); - } - qobject_unref(response); - } - - qobject_ref(ret); - qobject_unref(response); - return ret; -} - /* * Generic hot-plugging test via the device_add QMP commands. */ @@ -1355,13 +1326,6 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id, qobject_unref(args); } -static void device_deleted_cb(void *opaque, const char *name, QDict *data) -{ - bool *got_event = opaque; - - g_assert_cmpstr(name, ==, "DEVICE_DELETED"); - *got_event = true; -} /* * Generic hot-unplugging test via the device_del QMP command. @@ -1378,24 +1342,17 @@ static void device_deleted_cb(void *opaque, const char *name, QDict *data) * and this one: * * {"return": {}} - * - * But the order of arrival may vary - so we've got to detect both. */ void qtest_qmp_device_del(QTestState *qts, const char *id) { - bool got_event = false; QDict *rsp; - qtest_qmp_send(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}", - id); - rsp = qtest_qmp_receive_success(qts, device_deleted_cb, &got_event); + rsp = qtest_qmp(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}", + id); + + g_assert(qdict_haskey(rsp, "return")); qobject_unref(rsp); - if (!got_event) { - rsp = qtest_qmp_receive_dict(qts); - g_assert_cmpstr(qdict_get_try_str(rsp, "event"), - ==, "DEVICE_DELETED"); - qobject_unref(rsp); - } + qtest_qmp_eventwait(qts, "DEVICE_DELETED"); } bool qmp_rsp_is_err(QDict *rsp) diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index 516093b39a..b799dbafb7 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -17,10 +17,12 @@ bool got_stop; -static void stop_cb(void *opaque, const char *name, QDict *data) +static void check_stop_event(QTestState *who) { - if (!strcmp(name, "STOP")) { + QDict *event = qtest_qmp_event_ref(who, "STOP"); + if (event) { got_stop = true; + qobject_unref(event); } } @@ -30,12 +32,19 @@ static void stop_cb(void *opaque, const char *name, QDict *data) QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...) { va_list ap; + QDict *resp; va_start(ap, command); qtest_qmp_vsend_fds(who, &fd, 1, command, ap); va_end(ap); - return qtest_qmp_receive_success(who, stop_cb, NULL); + resp = qtest_qmp_receive(who); + check_stop_event(who); + + g_assert(!qdict_haskey(resp, "error")); + g_assert(qdict_haskey(resp, "return")); + + return qdict_get_qdict(resp, "return"); } /* @@ -44,12 +53,18 @@ QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...) QDict *wait_command(QTestState *who, const char *command, ...) { va_list ap; + QDict *resp; va_start(ap, command); - qtest_qmp_vsend(who, command, ap); + resp = qtest_vqmp(who, command, ap); va_end(ap); - return qtest_qmp_receive_success(who, stop_cb, NULL); + check_stop_event(who); + + g_assert(!qdict_haskey(resp, "error")); + g_assert(qdict_haskey(resp, "return")); + + return qdict_get_qdict(resp, "return"); } /* From c45a70d8c271056896a057fbcdc7743a2942d0ec Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 7 Oct 2020 07:37:41 -0400 Subject: [PATCH 24/38] device-plug-test: use qtest_qmp to send the device_del command Simplify the code now that events are buffered. There is no need anymore to separate sending the command and retrieving the response. Signed-off-by: Paolo Bonzini --- tests/qtest/device-plug-test.c | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c index a2247856be..559d47727a 100644 --- a/tests/qtest/device-plug-test.c +++ b/tests/qtest/device-plug-test.c @@ -15,26 +15,17 @@ #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" -static void device_del_start(QTestState *qtest, const char *id) +static void device_del(QTestState *qtest, const char *id) { - qtest_qmp_send(qtest, - "{'execute': 'device_del', 'arguments': { 'id': %s } }", id); -} + QDict *resp; -static void device_del_finish(QTestState *qtest) -{ - QDict *resp = qtest_qmp_receive_dict(qtest); + resp = qtest_qmp(qtest, + "{'execute': 'device_del', 'arguments': { 'id': %s } }", id); g_assert(qdict_haskey(resp, "return")); qobject_unref(resp); } -static void device_del_request(QTestState *qtest, const char *id) -{ - device_del_start(qtest, id); - device_del_finish(qtest); -} - static void system_reset(QTestState *qtest) { QDict *resp; @@ -79,7 +70,7 @@ static void test_pci_unplug_request(void) * be processed. However during system reset, the removal will be * handled, removing the device. */ - device_del_request(qtest, "dev0"); + device_del(qtest, "dev0"); system_reset(qtest); wait_device_deleted_event(qtest, "dev0"); @@ -90,13 +81,8 @@ static void test_ccw_unplug(void) { QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0"); - /* - * The DEVICE_DELETED events will be sent before the command - * completes. - */ - device_del_start(qtest, "dev0"); + device_del(qtest, "dev0"); wait_device_deleted_event(qtest, "dev0"); - device_del_finish(qtest); qtest_quit(qtest); } @@ -109,7 +95,7 @@ static void test_spapr_cpu_unplug_request(void) "-device power9_v2.0-spapr-cpu-core,core-id=1,id=dev0"); /* similar to test_pci_unplug_request */ - device_del_request(qtest, "dev0"); + device_del(qtest, "dev0"); system_reset(qtest); wait_device_deleted_event(qtest, "dev0"); @@ -125,7 +111,7 @@ static void test_spapr_memory_unplug_request(void) "-device pc-dimm,id=dev0,memdev=mem0"); /* similar to test_pci_unplug_request */ - device_del_request(qtest, "dev0"); + device_del(qtest, "dev0"); system_reset(qtest); wait_device_deleted_event(qtest, "dev0"); @@ -139,7 +125,7 @@ static void test_spapr_phb_unplug_request(void) qtest = qtest_initf("-device spapr-pci-host-bridge,index=1,id=dev0"); /* similar to test_pci_unplug_request */ - device_del_request(qtest, "dev0"); + device_del(qtest, "dev0"); system_reset(qtest); wait_device_deleted_event(qtest, "dev0"); From bb1a5b97f75ae209d8707f698da23088d7b9bbb5 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:38:53 +0300 Subject: [PATCH 25/38] qtest: switch users back to qtest_qmp_receive Let test use the new functionality for buffering events. The only remaining users of qtest_qmp_receive_dict are tests that fuzz the QMP protocol. Tested with 'make check-qtest'. Signed-off-by: Maxim Levitsky Message-Id: <20201006123904.610658-4-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- tests/qtest/ahci-test.c | 4 ++-- tests/qtest/drive_del-test.c | 9 +++------ tests/qtest/libqtest.c | 12 +++--------- tests/qtest/pvpanic-test.c | 4 +--- tests/qtest/tpm-util.c | 8 ++++++-- 5 files changed, 15 insertions(+), 22 deletions(-) diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c index d42ebaeb4c..5e1954852e 100644 --- a/tests/qtest/ahci-test.c +++ b/tests/qtest/ahci-test.c @@ -1590,7 +1590,7 @@ static void test_atapi_tray(void) qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-open-tray', " "'arguments': {'id': 'cd0'}}"); atapi_wait_tray(ahci, true); - rsp = qtest_qmp_receive_dict(ahci->parent->qts); + rsp = qtest_qmp_receive(ahci->parent->qts); qobject_unref(rsp); qmp_discard_response(ahci->parent->qts, @@ -1620,7 +1620,7 @@ static void test_atapi_tray(void) qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', " "'arguments': {'id': 'cd0'}}"); atapi_wait_tray(ahci, false); - rsp = qtest_qmp_receive_dict(ahci->parent->qts); + rsp = qtest_qmp_receive(ahci->parent->qts); qobject_unref(rsp); /* Now, to convince ATAPI we understand the media has changed... */ diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c index ba0cd77445..9d20a1ed8b 100644 --- a/tests/qtest/drive_del-test.c +++ b/tests/qtest/drive_del-test.c @@ -15,9 +15,6 @@ #include "libqos/virtio.h" #include "qapi/qmp/qdict.h" -/* TODO actually test the results and get rid of this */ -#define qmp_discard_response(q, ...) qobject_unref(qtest_qmp(q, __VA_ARGS__)) - static void drive_add(QTestState *qts) { char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0"); @@ -38,13 +35,13 @@ static void device_del(QTestState *qts) { QDict *response; - /* Complication: ignore DEVICE_DELETED event */ - qmp_discard_response(qts, "{'execute': 'device_del'," + response = qtest_qmp(qts, "{'execute': 'device_del'," " 'arguments': { 'id': 'dev0' } }"); - response = qtest_qmp_receive_dict(qts); g_assert(response); g_assert(qdict_haskey(response, "return")); qobject_unref(response); + + qtest_qmp_eventwait(qts, "DEVICE_DELETED"); } static void test_drive_without_dev(void) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index baac667b8d..08929f5ff6 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -324,7 +324,7 @@ QTestState *qtest_init(const char *extra_args) QDict *greeting; /* Read the QMP greeting and then do the handshake */ - greeting = qtest_qmp_receive_dict(s); + greeting = qtest_qmp_receive(s); qobject_unref(greeting); qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }")); @@ -700,7 +700,7 @@ QDict *qtest_vqmp_fds(QTestState *s, int *fds, size_t fds_num, qtest_qmp_vsend_fds(s, fds, fds_num, fmt, ap); /* Receive reply */ - return qtest_qmp_receive_dict(s); + return qtest_qmp_receive(s); } QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap) @@ -708,7 +708,7 @@ QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap) qtest_qmp_vsend(s, fmt, ap); /* Receive reply */ - return qtest_qmp_receive_dict(s); + return qtest_qmp_receive(s); } QDict *qmp_fd(int fd, const char *fmt, ...) @@ -850,12 +850,6 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap) " 'arguments': {'command-line': %s}}", cmd); ret = g_strdup(qdict_get_try_str(resp, "return")); - while (ret == NULL && qdict_get_try_str(resp, "event")) { - /* Ignore asynchronous QMP events */ - qobject_unref(resp); - resp = qtest_qmp_receive_dict(s); - ret = g_strdup(qdict_get_try_str(resp, "return")); - } g_assert(ret); qobject_unref(resp); g_free(cmd); diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c index b0b20ad340..0657de797f 100644 --- a/tests/qtest/pvpanic-test.c +++ b/tests/qtest/pvpanic-test.c @@ -24,9 +24,7 @@ static void test_panic(void) qtest_outb(qts, 0x505, 0x1); - response = qtest_qmp_receive_dict(qts); - g_assert(qdict_haskey(response, "event")); - g_assert_cmpstr(qdict_get_str(response, "event"), ==, "GUEST_PANICKED"); + response = qtest_qmp_eventwait_ref(qts, "GUEST_PANICKED"); g_assert(qdict_haskey(response, "data")); data = qdict_get_qdict(response, "data"); g_assert(qdict_haskey(data, "action")); diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c index 3ed6c8548a..5a33a6ef0f 100644 --- a/tests/qtest/tpm-util.c +++ b/tests/qtest/tpm-util.c @@ -237,12 +237,16 @@ void tpm_util_migrate(QTestState *who, const char *uri) void tpm_util_wait_for_migration_complete(QTestState *who) { while (true) { + QDict *rsp; QDict *rsp_return; bool completed; const char *status; - qtest_qmp_send(who, "{ 'execute': 'query-migrate' }"); - rsp_return = qtest_qmp_receive_success(who, NULL, NULL); + rsp = qtest_qmp(who, "{ 'execute': 'query-migrate' }"); + g_assert(qdict_haskey(rsp, "return")); + rsp_return = qdict_get_qdict(rsp, "return"); + + g_assert(!qdict_haskey(rsp_return, "error")); status = qdict_get_str(rsp_return, "status"); completed = strcmp(status, "completed") == 0; g_assert_cmpstr(status, !=, "failed"); From 9a613ddccce125e4cc3a4a23c294837c906440d6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 7 Oct 2020 05:50:22 -0400 Subject: [PATCH 26/38] qtest: check that drives are really appearing and disappearing Do not just trust the HMP commands to create and delete the drive, use query-block to check that this is actually the case. Reviewed-by: Kevin Wolf Signed-off-by: Paolo Bonzini --- tests/qtest/drive_del-test.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c index 9d20a1ed8b..ff772b3671 100644 --- a/tests/qtest/drive_del-test.c +++ b/tests/qtest/drive_del-test.c @@ -14,20 +14,49 @@ #include "libqos/libqtest.h" #include "libqos/virtio.h" #include "qapi/qmp/qdict.h" +#include "qapi/qmp/qlist.h" + +static bool has_drive(QTestState *qts) +{ + QDict *response; + QList *ret; + QListEntry *entry; + bool found; + + response = qtest_qmp(qts, "{'execute': 'query-block'}"); + g_assert(response && qdict_haskey(response, "return")); + ret = qdict_get_qlist(response, "return"); + + found = false; + QLIST_FOREACH_ENTRY(ret, entry) { + QDict *entry_dict = qobject_to(QDict, entry->value); + if (!strcmp(qdict_get_str(entry_dict, "device"), "drive0")) { + found = true; + break; + } + } + + qobject_unref(response); + return found; +} static void drive_add(QTestState *qts) { char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0"); g_assert_cmpstr(resp, ==, "OK\r\n"); + g_assert(has_drive(qts)); g_free(resp); } static void drive_del(QTestState *qts) { - char *resp = qtest_hmp(qts, "drive_del drive0"); + char *resp; + g_assert(has_drive(qts)); + resp = qtest_hmp(qts, "drive_del drive0"); g_assert_cmpstr(resp, ==, ""); + g_assert(!has_drive(qts)); g_free(resp); } @@ -130,6 +159,7 @@ static void test_drive_del_device_del(void) */ drive_del(qts); device_del(qts); + g_assert(!has_drive(qts)); qtest_quit(qts); } From d8a18da56df93b7f778fb97ba370031597d19ffd Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 7 Oct 2020 06:43:03 -0400 Subject: [PATCH 27/38] qemu-iotests, qtest: rewrite test 067 as a qtest Test 067 from qemu-iotests is executing QMP commands to hotplug and hot-unplug disks, devices and blockdevs. Because the power of the text-based test harness is limited, it is actually limiting the checks that it does, for example by skipping DEVICE_DELETED events. tests/qtest already has a similar test, drive_del-test.c. We can merge them, and even reuse some of the existing code in drive_del-test.c. This will improve the quality of the test by covering DEVICE_DELETED events and testing multiple architectures (therefore covering multiple PCI hotplug mechanisms as well as s390x virtio-ccw). The only difference is that the new test will always use null-co:// for the medium rather than qcow2 or raw, but this should be irrelevant for what the test is covering. For example there are no "qemu-img check" runs in 067 that would check that the file is properly closed. The new tests requires PCI hot-plug support, so drive_del-test is moved from qemu-system-ppc to qemu-system-ppc64. Reviewed-by: Kevin Wolf Signed-off-by: Paolo Bonzini --- .gitlab-ci.yml | 2 +- tests/qemu-iotests/067 | 157 ------------- tests/qemu-iotests/067.out | 414 ----------------------------------- tests/qemu-iotests/group | 2 +- tests/qtest/drive_del-test.c | 211 ++++++++++++++++-- tests/qtest/meson.build | 4 +- 6 files changed, 191 insertions(+), 599 deletions(-) delete mode 100755 tests/qemu-iotests/067 delete mode 100644 tests/qemu-iotests/067.out diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a51c89554f..a4cf87481e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -232,7 +232,7 @@ build-tcg-disabled: - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048 052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163 170 171 183 184 192 194 197 208 215 221 222 226 227 236 253 277 - - ./check -qcow2 028 051 056 057 058 065 067 068 082 085 091 095 096 102 122 + - ./check -qcow2 028 051 056 057 058 065 068 082 085 091 095 096 102 122 124 132 139 142 144 145 151 152 155 157 165 194 196 197 200 202 208 209 215 216 218 222 227 234 246 247 248 250 254 255 257 258 260 261 262 263 264 270 272 273 277 279 diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067 deleted file mode 100755 index a63be9cabf..0000000000 --- a/tests/qemu-iotests/067 +++ /dev/null @@ -1,157 +0,0 @@ -#!/usr/bin/env bash -# -# Test automatic deletion of BDSes created by -drive/drive_add -# -# Copyright (C) 2013 Red Hat, Inc. -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program 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 General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see . -# - -# creator -owner=kwolf@redhat.com - -seq=`basename $0` -echo "QA output created by $seq" - -status=1 # failure is the default! - -# get standard environment, filters and checks -. ./common.rc -. ./common.filter - -_supported_fmt qcow2 -_supported_proto file -# Because anything other than 16 would change the output of query-block, -# and external data files would change the output of -# query-named-block-nodes -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file - -do_run_qemu() -{ - echo Testing: "$@" - $QEMU -nographic -qmp-pretty stdio -serial none "$@" - echo -} - -# Remove QMP events from (pretty-printed) output. Doesn't handle -# nested dicts correctly, but we don't get any of those in this test. -_filter_qmp_events() -{ - tr '\n' '\t' | sed -e \ - 's/{\s*"timestamp":\s*{[^}]*},\s*"event":[^,}]*\(,\s*"data":\s*{[^}]*}\)\?\s*}\s*//g' \ - | tr '\t' '\n' -} - -run_qemu() -{ - do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu \ - | _filter_actual_image_size \ - | _filter_generated_node_ids | _filter_qmp_events \ - | _filter_img_info -} - -size=128M - -_make_test_img $size - -echo -echo === -drive/-device and device_del === -echo - -run_qemu -drive file=$TEST_IMG,format=$IMGFMT,if=none,id=disk -device virtio-blk,drive=disk,id=virtio0 <value); - if (!strcmp(qdict_get_str(entry_dict, "device"), "drive0")) { + if (!strcmp(qdict_get_str(entry_dict, key), "drive0")) { found = true; break; } @@ -40,6 +40,38 @@ static bool has_drive(QTestState *qts) return found; } +static bool has_drive(QTestState *qts) +{ + return look_for_drive0(qts, "query-block", "device"); +} + +static bool has_blockdev(QTestState *qts) +{ + return look_for_drive0(qts, "query-named-block-nodes", "node-name"); +} + +static void blockdev_add_with_media(QTestState *qts) +{ + QDict *response; + + response = qtest_qmp(qts, + "{ 'execute': 'blockdev-add'," + " 'arguments': {" + " 'driver': 'raw'," + " 'node-name': 'drive0'," + " 'file': {" + " 'driver': 'null-co'," + " 'read-zeroes': true" + " }" + " }" + "}"); + + g_assert(response); + g_assert(qdict_haskey(response, "return")); + qobject_unref(response); + g_assert(has_blockdev(qts)); +} + static void drive_add(QTestState *qts) { char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0"); @@ -49,6 +81,17 @@ static void drive_add(QTestState *qts) g_free(resp); } +static void drive_add_with_media(QTestState *qts) +{ + char *resp = qtest_hmp(qts, + "drive_add 0 if=none,id=drive0,file=null-co://," + "file.read-zeroes=on,format=raw"); + + g_assert_cmpstr(resp, ==, "OK\r\n"); + g_assert(has_drive(qts)); + g_free(resp); +} + static void drive_del(QTestState *qts) { char *resp; @@ -60,7 +103,43 @@ static void drive_del(QTestState *qts) g_free(resp); } -static void device_del(QTestState *qts) +/* + * qvirtio_get_dev_type: + * Returns: the preferred virtio bus/device type for the current architecture. + * TODO: delete this + */ +static const char *qvirtio_get_dev_type(void) +{ + const char *arch = qtest_get_arch(); + + if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) { + return "device"; /* for virtio-mmio */ + } else if (g_str_equal(arch, "s390x")) { + return "ccw"; + } else { + return "pci"; + } +} + +static void device_add(QTestState *qts) +{ + QDict *response; + char driver[32]; + snprintf(driver, sizeof(driver), "virtio-blk-%s", + qvirtio_get_dev_type()); + + response = qtest_qmp(qts, "{'execute': 'device_add'," + " 'arguments': {" + " 'driver': %s," + " 'drive': 'drive0'," + " 'id': 'dev0'" + "}}", driver); + g_assert(response); + g_assert(qdict_haskey(response, "return")); + qobject_unref(response); +} + +static void device_del(QTestState *qts, bool and_reset) { QDict *response; @@ -70,6 +149,13 @@ static void device_del(QTestState *qts) g_assert(qdict_haskey(response, "return")); qobject_unref(response); + if (and_reset) { + response = qtest_qmp(qts, "{'execute': 'system_reset' }"); + g_assert(response); + g_assert(qdict_haskey(response, "return")); + qobject_unref(response); + } + qtest_qmp_eventwait(qts, "DEVICE_DELETED"); } @@ -91,24 +177,6 @@ static void test_drive_without_dev(void) qtest_quit(qts); } -/* - * qvirtio_get_dev_type: - * Returns: the preferred virtio bus/device type for the current architecture. - * TODO: delete this - */ -static const char *qvirtio_get_dev_type(void) -{ - const char *arch = qtest_get_arch(); - - if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) { - return "device"; /* for virtio-mmio */ - } else if (g_str_equal(arch, "s390x")) { - return "ccw"; - } else { - return "pci"; - } -} - static void test_after_failed_device_add(void) { char driver[32]; @@ -158,12 +226,97 @@ static void test_drive_del_device_del(void) * Doing it in this order takes notoriously tricky special paths */ drive_del(qts); - device_del(qts); + device_del(qts, false); g_assert(!has_drive(qts)); qtest_quit(qts); } +static void test_cli_device_del(void) +{ + QTestState *qts; + + /* + * -drive/-device and device_del. Start with a drive used by a + * device that unplugs after reset. + */ + qts = qtest_initf("-drive if=none,id=drive0,file=null-co://," + "file.read-zeroes=on,format=raw" + " -device virtio-blk-%s,drive=drive0,id=dev0", + qvirtio_get_dev_type()); + + device_del(qts, true); + g_assert(!has_drive(qts)); + + qtest_quit(qts); +} + +static void test_empty_device_del(void) +{ + QTestState *qts; + + /* device_del with no drive plugged. */ + qts = qtest_initf("-device virtio-scsi-%s -device scsi-cd,id=dev0", + qvirtio_get_dev_type()); + + device_del(qts, false); + qtest_quit(qts); +} + +static void test_device_add_and_del(void) +{ + QTestState *qts; + + /* + * -drive/device_add and device_del. Start with a drive used by a + * device that unplugs after reset. + */ + qts = qtest_init("-drive if=none,id=drive0,file=null-co://," + "file.read-zeroes=on,format=raw"); + + device_add(qts); + device_del(qts, true); + g_assert(!has_drive(qts)); + + qtest_quit(qts); +} + +static void test_drive_add_device_add_and_del(void) +{ + QTestState *qts; + + qts = qtest_init(""); + + /* + * drive_add/device_add and device_del. The drive is used by a + * device that unplugs after reset. + */ + drive_add_with_media(qts); + device_add(qts); + device_del(qts, true); + g_assert(!has_drive(qts)); + + qtest_quit(qts); +} + +static void test_blockdev_add_device_add_and_del(void) +{ + QTestState *qts; + + qts = qtest_init(""); + + /* + * blockdev_add/device_add and device_del. The it drive is used by a + * device that unplugs after reset, but it doesn't go away. + */ + blockdev_add_with_media(qts); + device_add(qts); + device_del(qts, true); + g_assert(has_blockdev(qts)); + + qtest_quit(qts); +} + int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); @@ -173,8 +326,18 @@ int main(int argc, char **argv) if (qvirtio_get_dev_type() != NULL) { qtest_add_func("/drive_del/after_failed_device_add", test_after_failed_device_add); - qtest_add_func("/blockdev/drive_del_device_del", + qtest_add_func("/drive_del/drive_del_device_del", test_drive_del_device_del); + qtest_add_func("/device_del/drive/cli_device", + test_cli_device_del); + qtest_add_func("/device_del/drive/device_add", + test_device_add_and_del); + qtest_add_func("/device_del/drive/drive_add_device_add", + test_drive_add_device_add_and_del); + qtest_add_func("/device_del/empty", + test_empty_device_del); + qtest_add_func("/device_del/blockdev", + test_blockdev_add_device_add_and_del); } return g_test_run(); diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 28bafc02a2..1c4b87e3e2 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -111,7 +111,7 @@ qtests_moxie = [ 'boot-serial-test' ] qtests_ppc = \ (config_all_devices.has_key('CONFIG_ISA_TESTDEV') ? ['endianness-test'] : []) + \ (config_all_devices.has_key('CONFIG_M48T59') ? ['m48t59-test'] : []) + \ - ['boot-order-test', 'prom-env-test', 'drive_del-test', 'boot-serial-test'] \ + ['boot-order-test', 'prom-env-test', 'boot-serial-test'] \ qtests_ppc64 = \ (config_all_devices.has_key('CONFIG_PSERIES') ? ['device-plug-test'] : []) + \ @@ -121,7 +121,7 @@ qtests_ppc64 = \ (config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : []) + \ (config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ? ['usb-hcd-xhci-test'] : []) + \ (config_host.has_key('CONFIG_POSIX') ? ['test-filter-mirror'] : []) + \ - qtests_pci + ['migration-test', 'numa-test', 'cpu-plug-test'] + qtests_pci + ['migration-test', 'numa-test', 'cpu-plug-test', 'drive_del-test'] qtests_sh4 = (config_all_devices.has_key('CONFIG_ISA_TESTDEV') ? ['endianness-test'] : []) qtests_sh4eb = (config_all_devices.has_key('CONFIG_ISA_TESTDEV') ? ['endianness-test'] : []) From bb755ba47f3747251c0eadf681ee68b9033309b8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 15:38:55 +0300 Subject: [PATCH 28/38] qdev: add "check if address free" callback for buses Check if an address is free on the bus before plugging in the device. This makes it possible to do the check without any side effects, and to detect the problem early without having to do it in the realize callback. Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-5-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/core/qdev.c | 17 +++++++++++++++-- hw/net/virtio-net.c | 2 +- hw/sd/core.c | 3 ++- include/hw/qdev-core.h | 13 ++++++++++++- 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 96772a15bd..74db78df36 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -94,13 +94,23 @@ static void bus_add_child(BusState *bus, DeviceState *child) 0); } -void qdev_set_parent_bus(DeviceState *dev, BusState *bus) +static bool bus_check_address(BusState *bus, DeviceState *child, Error **errp) +{ + BusClass *bc = BUS_GET_CLASS(bus); + return !bc->check_address || bc->check_address(bus, child, errp); +} + +bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp) { BusState *old_parent_bus = dev->parent_bus; DeviceClass *dc = DEVICE_GET_CLASS(dev); assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)); + if (!bus_check_address(bus, dev, errp)) { + return false; + } + if (old_parent_bus) { trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)), old_parent_bus, object_get_typename(OBJECT(old_parent_bus)), @@ -126,6 +136,7 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus) object_unref(OBJECT(old_parent_bus)); object_unref(OBJECT(dev)); } + return true; } DeviceState *qdev_new(const char *name) @@ -371,7 +382,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp) assert(!dev->realized && !dev->parent_bus); if (bus) { - qdev_set_parent_bus(dev, bus); + if (!qdev_set_parent_bus(dev, bus, errp)) { + return false; + } } else { assert(!DEVICE_GET_CLASS(dev)->bus_type); } diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index a160a9da9c..277289d56e 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -3138,7 +3138,7 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp) error_setg(errp, "virtio_net: couldn't find primary bus"); return false; } - qdev_set_parent_bus(n->primary_dev, n->primary_bus); + qdev_set_parent_bus(n->primary_dev, n->primary_bus, &error_abort); n->primary_should_be_hidden = false; if (!qemu_opt_set_bool(n->primary_device_opts, "partially_hotplugged", true, errp)) { diff --git a/hw/sd/core.c b/hw/sd/core.c index 957d116f1a..08c93b5903 100644 --- a/hw/sd/core.c +++ b/hw/sd/core.c @@ -23,6 +23,7 @@ #include "hw/qdev-core.h" #include "hw/sd/sd.h" #include "qemu/module.h" +#include "qapi/error.h" #include "trace.h" static inline const char *sdbus_name(SDBus *sdbus) @@ -240,7 +241,7 @@ void sdbus_reparent_card(SDBus *from, SDBus *to) readonly = sc->get_readonly(card); sdbus_set_inserted(from, false); - qdev_set_parent_bus(DEVICE(card), &to->qbus); + qdev_set_parent_bus(DEVICE(card), &to->qbus, &error_abort); sdbus_set_inserted(to, true); sdbus_set_readonly(to, readonly); } diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 72064f4dd4..14d476c587 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -210,13 +210,24 @@ struct BusClass { /* FIXME first arg should be BusState */ void (*print_dev)(Monitor *mon, DeviceState *dev, int indent); char *(*get_dev_path)(DeviceState *dev); + /* * This callback is used to create Open Firmware device path in accordance * with OF spec http://forthworks.com/standards/of1275.pdf. Individual bus * bindings can be found at http://playground.sun.com/1275/bindings/. */ char *(*get_fw_dev_path)(DeviceState *dev); + void (*reset)(BusState *bus); + + /* + * Return whether the device can be added to @bus, + * based on the address that was set (via device properties) + * before realize. If not, on return @errp contains the + * human-readable error message. + */ + bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp); + BusRealize realize; BusUnrealize unrealize; @@ -788,7 +799,7 @@ const char *qdev_fw_name(DeviceState *dev); Object *qdev_get_machine(void); /* FIXME: make this a link<> */ -void qdev_set_parent_bus(DeviceState *dev, BusState *bus); +bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp); extern bool qdev_hotplug; extern bool qdev_hot_removed; From 7a8202c521a5d1ac9e289d5c2b5125a9310af178 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 14:38:57 +0200 Subject: [PATCH 29/38] scsi/scsi_bus: switch search direction in scsi_device_find This change will allow us to convert the bus children list to RCU, while not changing the logic of this function Signed-off-by: Maxim Levitsky Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-2-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 3284a5d1fb..6b1ed7ae9a 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -1572,7 +1572,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) BusChild *kid; SCSIDevice *target_dev = NULL; - QTAILQ_FOREACH_REVERSE(kid, &bus->qbus.children, sibling) { + QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) { DeviceState *qdev = kid->child; SCSIDevice *dev = SCSI_DEVICE(qdev); @@ -1580,7 +1580,15 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) if (dev->lun == lun) { return dev; } - target_dev = dev; + + /* + * If we don't find exact match (channel/bus/lun), + * we will return the first device which matches channel/bus + */ + + if (!target_dev) { + target_dev = dev; + } } } return target_dev; From 7bed89958bfbf40df9ca681cefbdca63abdde39d Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 14:38:58 +0200 Subject: [PATCH 30/38] device_core: use drain_call_rcu in in qmp_device_add Soon, a device removal might only happen on RCU callback execution. This is okay for device-del which provides a DEVICE_DELETED event, but not for the failure case of device-add. To avoid changing monitor semantics, just drain all pending RCU callbacks on error. Signed-off-by: Maxim Levitsky Suggested-by: Stefan Hajnoczi Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com> [Don't use it in qmp_device_del. - Paolo] Signed-off-by: Paolo Bonzini --- softmmu/qdev-monitor.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index e9b7228480..bcfb90a08f 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) return; } dev = qdev_device_add(opts, errp); + + /* + * Drain all pending RCU callbacks. This is done because + * some bus related operations can delay a device removal + * (in this case this can happen if device is added and then + * removed due to a configuration error) + * to a RCU callback, but user might expect that this interface + * will finish its job completely once qmp command returns result + * to the user + */ + drain_call_rcu(); + if (!dev) { qemu_opts_del(opts); return; From 2d24a64661549732fc77f632928318dd52f5bce5 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:38:59 +0300 Subject: [PATCH 31/38] device-core: use RCU for list of children of a bus This fixes the race between device emulation code that tries to find a child device to dispatch the request to (e.g a scsi disk), and hotplug of a new device to that bus. Note that this doesn't convert all the readers of the list but only these that might go over that list without BQL held. This is a very small first step to make this code thread safe. Suggested-by: Paolo Bonzini Signed-off-by: Maxim Levitsky Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-5-mlevitsk@redhat.com> [Use RCU_READ_LOCK_GUARD in more places, adjust testcase now that the delay in DEVICE_DELETED due to RCU is more consistent. - Paolo] Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-9-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/core/bus.c | 28 +++++++++++++++++----------- hw/core/qdev.c | 37 +++++++++++++++++++++++-------------- hw/scsi/scsi-bus.c | 12 +++++++++--- hw/scsi/virtio-scsi.c | 6 +++++- include/hw/qdev-core.h | 9 +++++++++ 5 files changed, 63 insertions(+), 29 deletions(-) diff --git a/hw/core/bus.c b/hw/core/bus.c index 6b987b6946..a0483859ae 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -49,12 +49,14 @@ int qbus_walk_children(BusState *bus, } } - QTAILQ_FOREACH(kid, &bus->children, sibling) { - err = qdev_walk_children(kid->child, - pre_devfn, pre_busfn, - post_devfn, post_busfn, opaque); - if (err < 0) { - return err; + WITH_RCU_READ_LOCK_GUARD() { + QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) { + err = qdev_walk_children(kid->child, + pre_devfn, pre_busfn, + post_devfn, post_busfn, opaque); + if (err < 0) { + return err; + } } } @@ -90,8 +92,10 @@ static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb, BusState *bus = BUS(obj); BusChild *kid; - QTAILQ_FOREACH(kid, &bus->children, sibling) { - cb(OBJECT(kid->child), opaque, type); + WITH_RCU_READ_LOCK_GUARD() { + QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) { + cb(OBJECT(kid->child), opaque, type); + } } } @@ -194,9 +198,11 @@ static void bus_set_realized(Object *obj, bool value, Error **errp) /* TODO: recursive realization */ } else if (!value && bus->realized) { - QTAILQ_FOREACH(kid, &bus->children, sibling) { - DeviceState *dev = kid->child; - qdev_unrealize(dev); + WITH_RCU_READ_LOCK_GUARD() { + QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) { + DeviceState *dev = kid->child; + qdev_unrealize(dev); + } } if (bc->unrealize) { bc->unrealize(bus); diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 74db78df36..59e5e710b7 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -51,6 +51,12 @@ const VMStateDescription *qdev_get_vmsd(DeviceState *dev) return dc->vmsd; } +static void bus_free_bus_child(BusChild *kid) +{ + object_unref(OBJECT(kid->child)); + g_free(kid); +} + static void bus_remove_child(BusState *bus, DeviceState *child) { BusChild *kid; @@ -60,15 +66,16 @@ static void bus_remove_child(BusState *bus, DeviceState *child) char name[32]; snprintf(name, sizeof(name), "child[%d]", kid->index); - QTAILQ_REMOVE(&bus->children, kid, sibling); + QTAILQ_REMOVE_RCU(&bus->children, kid, sibling); bus->num_children--; /* This gives back ownership of kid->child back to us. */ object_property_del(OBJECT(bus), name); - object_unref(OBJECT(kid->child)); - g_free(kid); - return; + + /* free the bus kid, when it is safe to do so*/ + call_rcu(kid, bus_free_bus_child, rcu); + break; } } } @@ -83,7 +90,7 @@ static void bus_add_child(BusState *bus, DeviceState *child) kid->child = child; object_ref(OBJECT(kid->child)); - QTAILQ_INSERT_HEAD(&bus->children, kid, sibling); + QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling); /* This transfers ownership of kid->child to the property. */ snprintf(name, sizeof(name), "child[%d]", kid->index); @@ -672,17 +679,19 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id) DeviceState *ret; BusState *child; - QTAILQ_FOREACH(kid, &bus->children, sibling) { - DeviceState *dev = kid->child; + WITH_RCU_READ_LOCK_GUARD() { + QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) { + DeviceState *dev = kid->child; - if (dev->id && strcmp(dev->id, id) == 0) { - return dev; - } + if (dev->id && strcmp(dev->id, id) == 0) { + return dev; + } - QLIST_FOREACH(child, &dev->child_bus, sibling) { - ret = qdev_find_recursive(child, id); - if (ret) { - return ret; + QLIST_FOREACH(child, &dev->child_bus, sibling) { + ret = qdev_find_recursive(child, id); + if (ret) { + return ret; + } } } } diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 6b1ed7ae9a..4cf1f404b4 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -400,7 +400,10 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) id = r->req.dev->id; found_lun0 = false; n = 0; - QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) { + + RCU_READ_LOCK_GUARD(); + + QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { DeviceState *qdev = kid->child; SCSIDevice *dev = SCSI_DEVICE(qdev); @@ -421,7 +424,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) memset(r->buf, 0, len); stl_be_p(&r->buf[0], n); i = found_lun0 ? 8 : 16; - QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) { + QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { DeviceState *qdev = kid->child; SCSIDevice *dev = SCSI_DEVICE(qdev); @@ -430,6 +433,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) i += 8; } } + assert(i == n + 8); r->len = len; return true; @@ -1572,7 +1576,8 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) BusChild *kid; SCSIDevice *target_dev = NULL; - QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) { + RCU_READ_LOCK_GUARD(); + QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) { DeviceState *qdev = kid->child; SCSIDevice *dev = SCSI_DEVICE(qdev); @@ -1591,6 +1596,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) } } } + return target_dev; } diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 3a71ea7097..971afbb217 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -367,12 +367,16 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: target = req->req.tmf.lun[1]; s->resetting++; - QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) { + + rcu_read_lock(); + QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) { d = SCSI_DEVICE(kid->child); if (d->channel == 0 && d->id == target) { qdev_reset_all(&d->qdev); } } + rcu_read_unlock(); + s->resetting--; break; diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 14d476c587..2c6307e3ed 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -3,6 +3,8 @@ #include "qemu/queue.h" #include "qemu/bitmap.h" +#include "qemu/rcu.h" +#include "qemu/rcu_queue.h" #include "qom/object.h" #include "hw/hotplug.h" #include "hw/resettable.h" @@ -238,6 +240,7 @@ struct BusClass { }; typedef struct BusChild { + struct rcu_head rcu; DeviceState *child; int index; QTAILQ_ENTRY(BusChild) sibling; @@ -258,6 +261,12 @@ struct BusState { int max_index; bool realized; int num_children; + + /* + * children is a RCU QTAILQ, thus readers must use RCU to access it, + * and writers must hold the big qemu lock + */ + QTAILQ_HEAD(, BusChild) children; QLIST_ENTRY(BusState) sibling; ResettableState reset; From 42a90a899e70f5fbef2b5a117535acaa0bc1f5ad Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 15:38:56 +0300 Subject: [PATCH 32/38] scsi: switch to bus->check_address Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-6-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 136 +++++++++++++++++++++++++++------------------ 1 file changed, 82 insertions(+), 54 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 4cf1f404b4..4ab9811cd8 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -22,33 +22,6 @@ static void scsi_req_dequeue(SCSIRequest *req); static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len); static void scsi_target_free_buf(SCSIRequest *req); -static Property scsi_props[] = { - DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0), - DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1), - DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1), - DEFINE_PROP_END_OF_LIST(), -}; - -static void scsi_bus_class_init(ObjectClass *klass, void *data) -{ - BusClass *k = BUS_CLASS(klass); - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); - - k->get_dev_path = scsibus_get_dev_path; - k->get_fw_dev_path = scsibus_get_fw_dev_path; - hc->unplug = qdev_simple_device_unplug_cb; -} - -static const TypeInfo scsi_bus_info = { - .name = TYPE_SCSI_BUS, - .parent = TYPE_BUS, - .instance_size = sizeof(SCSIBus), - .class_init = scsi_bus_class_init, - .interfaces = (InterfaceInfo[]) { - { TYPE_HOTPLUG_HANDLER }, - { } - } -}; static int next_scsi_bus; static void scsi_device_realize(SCSIDevice *s, Error **errp) @@ -160,35 +133,68 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state) } } +static bool scsi_bus_is_address_free(SCSIBus *bus, + int channel, int target, int lun, + SCSIDevice **p_dev) +{ + SCSIDevice *d = scsi_device_find(bus, channel, target, lun); + if (d && d->lun == lun) { + if (p_dev) { + *p_dev = d; + } + return false; + } + if (p_dev) { + *p_dev = NULL; + } + return true; +} + +static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp) +{ + SCSIDevice *dev = SCSI_DEVICE(qdev); + SCSIBus *bus = SCSI_BUS(qbus); + + if (dev->channel > bus->info->max_channel) { + error_setg(errp, "bad scsi channel id: %d", dev->channel); + return false; + } + if (dev->id != -1 && dev->id > bus->info->max_target) { + error_setg(errp, "bad scsi device id: %d", dev->id); + return false; + } + if (dev->lun != -1 && dev->lun > bus->info->max_lun) { + error_setg(errp, "bad scsi device lun: %d", dev->lun); + return false; + } + + if (dev->id != -1 && dev->lun != -1) { + SCSIDevice *d; + if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) { + error_setg(errp, "lun already used by '%s'", d->qdev.id); + return false; + } + } + + return true; +} + static void scsi_qdev_realize(DeviceState *qdev, Error **errp) { SCSIDevice *dev = SCSI_DEVICE(qdev); SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus); - SCSIDevice *d; + bool is_free; Error *local_err = NULL; - if (dev->channel > bus->info->max_channel) { - error_setg(errp, "bad scsi channel id: %d", dev->channel); - return; - } - if (dev->id != -1 && dev->id > bus->info->max_target) { - error_setg(errp, "bad scsi device id: %d", dev->id); - return; - } - if (dev->lun != -1 && dev->lun > bus->info->max_lun) { - error_setg(errp, "bad scsi device lun: %d", dev->lun); - return; - } - if (dev->id == -1) { int id = -1; if (dev->lun == -1) { dev->lun = 0; } do { - d = scsi_device_find(bus, dev->channel, ++id, dev->lun); - } while (d && d->lun == dev->lun && id < bus->info->max_target); - if (d && d->lun == dev->lun) { + is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL); + } while (!is_free && id < bus->info->max_target); + if (!is_free) { error_setg(errp, "no free target"); return; } @@ -196,20 +202,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp) } else if (dev->lun == -1) { int lun = -1; do { - d = scsi_device_find(bus, dev->channel, dev->id, ++lun); - } while (d && d->lun == lun && lun < bus->info->max_lun); - if (d && d->lun == lun) { + is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL); + } while (!is_free && lun < bus->info->max_lun); + if (!is_free) { error_setg(errp, "no free lun"); return; } dev->lun = lun; - } else { - d = scsi_device_find(bus, dev->channel, dev->id, dev->lun); - assert(d); - if (d->lun == dev->lun && dev != d) { - error_setg(errp, "lun already used by '%s'", d->qdev.id); - return; - } } QTAILQ_INIT(&dev->requests); @@ -1723,6 +1722,13 @@ const VMStateDescription vmstate_scsi_device = { } }; +static Property scsi_props[] = { + DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0), + DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1), + DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1), + DEFINE_PROP_END_OF_LIST(), +}; + static void scsi_device_class_init(ObjectClass *klass, void *data) { DeviceClass *k = DEVICE_CLASS(klass); @@ -1753,6 +1759,28 @@ static const TypeInfo scsi_device_type_info = { .instance_init = scsi_dev_instance_init, }; +static void scsi_bus_class_init(ObjectClass *klass, void *data) +{ + BusClass *k = BUS_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); + + k->get_dev_path = scsibus_get_dev_path; + k->get_fw_dev_path = scsibus_get_fw_dev_path; + k->check_address = scsi_bus_check_address; + hc->unplug = qdev_simple_device_unplug_cb; +} + +static const TypeInfo scsi_bus_info = { + .name = TYPE_SCSI_BUS, + .parent = TYPE_BUS, + .instance_size = sizeof(SCSIBus), + .class_init = scsi_bus_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } +}; + static void scsi_register_types(void) { type_register_static(&scsi_bus_info); From a23151e8cc8cc08546252dc9c7671171d9c44615 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:39:00 +0300 Subject: [PATCH 33/38] device-core: use atomic_set on .realized property Some code might race with placement of new devices on a bus. We currently first place a (unrealized) device on the bus and then realize it. As a workaround, users that scan the child device list, can check the realized property to see if it is safe to access such a device. Use an atomic write here too to aid with this. A separate discussion is what to do with devices that are unrealized: It looks like for this case we only call the hotplug handler's unplug callback and its up to it to unrealize the device. An atomic operation doesn't cause harm for this code path though. Signed-off-by: Maxim Levitsky Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-6-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-10-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/core/qdev.c | 19 ++++++++++++++++++- include/hw/qdev-core.h | 2 ++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 59e5e710b7..fc4daa36fa 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -946,7 +946,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } } + qatomic_store_release(&dev->realized, value); + } else if (!value && dev->realized) { + + /* + * Change the value so that any concurrent users are aware + * that the device is going to be unrealized + * + * TODO: change .realized property to enum that states + * each phase of the device realization/unrealization + */ + + qatomic_set(&dev->realized, value); + /* + * Ensure that concurrent users see this update prior to + * any other changes done by unrealize. + */ + smp_wmb(); + QLIST_FOREACH(bus, &dev->child_bus, sibling) { qbus_unrealize(bus); } @@ -961,7 +979,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } assert(local_err == NULL); - dev->realized = value; return; child_realize_fail: diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 2c6307e3ed..868973319e 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -163,6 +163,8 @@ struct NamedClockList { /** * DeviceState: * @realized: Indicates whether the device has been fully constructed. + * When accessed outsize big qemu lock, must be accessed with + * atomic_load_acquire() * @reset: ResettableState for the device; handled by Resettable interface. * * This structure should not be accessed directly. We declare it here From 8ddf958e8d62ada6395460b91ec5964ef21fed12 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 6 Oct 2020 15:39:01 +0300 Subject: [PATCH 34/38] scsi/scsi-bus: scsi_device_find: don't return unrealized devices The device core first places a device on the bus and then realizes it. Make scsi_device_find avoid returing such devices to avoid races in drivers that use an iothread (currently virtio-scsi) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399 Suggested-by: Paolo Bonzini Signed-off-by: Maxim Levitsky Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-7-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-11-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 83 +++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 30 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 4ab9811cd8..7599113efe 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -24,6 +24,55 @@ static void scsi_target_free_buf(SCSIRequest *req); static int next_scsi_bus; +static SCSIDevice *do_scsi_device_find(SCSIBus *bus, + int channel, int id, int lun, + bool include_unrealized) +{ + BusChild *kid; + SCSIDevice *retval = NULL; + + QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) { + DeviceState *qdev = kid->child; + SCSIDevice *dev = SCSI_DEVICE(qdev); + + if (dev->channel == channel && dev->id == id) { + if (dev->lun == lun) { + retval = dev; + break; + } + + /* + * If we don't find exact match (channel/bus/lun), + * we will return the first device which matches channel/bus + */ + + if (!retval) { + retval = dev; + } + } + } + + /* + * This function might run on the IO thread and we might race against + * main thread hot-plugging the device. + * We assume that as soon as .realized is set to true we can let + * the user access the device. + */ + + if (retval && !include_unrealized && + !qatomic_load_acquire(&retval->qdev.realized)) { + retval = NULL; + } + + return retval; +} + +SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) +{ + RCU_READ_LOCK_GUARD(); + return do_scsi_device_find(bus, channel, id, lun, false); +} + static void scsi_device_realize(SCSIDevice *s, Error **errp) { SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s); @@ -137,7 +186,10 @@ static bool scsi_bus_is_address_free(SCSIBus *bus, int channel, int target, int lun, SCSIDevice **p_dev) { - SCSIDevice *d = scsi_device_find(bus, channel, target, lun); + SCSIDevice *d; + + RCU_READ_LOCK_GUARD(); + d = do_scsi_device_find(bus, channel, target, lun, true); if (d && d->lun == lun) { if (p_dev) { *p_dev = d; @@ -1570,35 +1622,6 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev) qdev_fw_name(dev), d->id, d->lun); } -SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) -{ - BusChild *kid; - SCSIDevice *target_dev = NULL; - - RCU_READ_LOCK_GUARD(); - QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) { - DeviceState *qdev = kid->child; - SCSIDevice *dev = SCSI_DEVICE(qdev); - - if (dev->channel == channel && dev->id == id) { - if (dev->lun == lun) { - return dev; - } - - /* - * If we don't find exact match (channel/bus/lun), - * we will return the first device which matches channel/bus - */ - - if (!target_dev) { - target_dev = dev; - } - } - } - - return target_dev; -} - /* SCSI request list. For simplicity, pv points to the whole device */ static int put_scsi_requests(QEMUFile *f, void *pv, size_t size, From 8ff34495601067d02edb54b4346cace84ec4e1df Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:39:02 +0300 Subject: [PATCH 35/38] scsi/scsi_bus: Add scsi_device_get Add scsi_device_get which finds the scsi device and takes a reference to it. Suggested-by: Stefan Hajnoczi Signed-off-by: Maxim Levitsky Message-Id: <20200913160259.32145-8-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-12-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 11 +++++++++++ include/hw/scsi/scsi.h | 1 + 2 files changed, 12 insertions(+) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 7599113efe..eda8cb7e70 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) return do_scsi_device_find(bus, channel, id, lun, false); } +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun) +{ + SCSIDevice *d; + RCU_READ_LOCK_GUARD(); + d = do_scsi_device_find(bus, channel, id, lun, false); + if (d) { + object_ref(d); + } + return d; +} + static void scsi_device_realize(SCSIDevice *s, Error **errp) { SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s); diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index 7a55cdbd74..09fa5c9d2a 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed); int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size, uint8_t *buf, uint8_t buf_size); SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun); +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun); /* scsi-generic.c. */ extern const SCSIReqOps scsi_generic_req_ops; From 07a47d4a1879370009baab44f1f387610d88a299 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:39:03 +0300 Subject: [PATCH 36/38] virtio-scsi: use scsi_device_get This will help us to avoid the scsi device disappearing after we took a reference to it. It doesn't by itself forbid case when we try to access an unrealized device Suggested-by: Stefan Hajnoczi Signed-off-by: Maxim Levitsky Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-9-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-13-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/virtio-scsi.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 971afbb217..3db9a8aae9 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -33,7 +33,7 @@ static inline int virtio_scsi_get_lun(uint8_t *lun) return ((lun[2] << 8) | lun[3]) & 0x3FFF; } -static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun) +static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun) { if (lun[0] != 1) { return NULL; @@ -41,7 +41,7 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun) if (lun[2] != 0 && !(lun[2] >= 0x40 && lun[2] < 0x80)) { return NULL; } - return scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun)); + return scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun)); } void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req) @@ -256,7 +256,7 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d) * case of async cancellation. */ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) { - SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun); + SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun); SCSIRequest *r, *next; BusChild *kid; int target; @@ -370,10 +370,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) rcu_read_lock(); QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) { - d = SCSI_DEVICE(kid->child); - if (d->channel == 0 && d->id == target) { - qdev_reset_all(&d->qdev); - } + SCSIDevice *d1 = SCSI_DEVICE(kid->child); + if (d1->channel == 0 && d1->id == target) { + qdev_reset_all(&d1->qdev); + } } rcu_read_unlock(); @@ -386,14 +386,17 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) break; } + object_unref(OBJECT(d)); return ret; incorrect_lun: req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN; + object_unref(OBJECT(d)); return ret; fail: req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET; + object_unref(OBJECT(d)); return ret; } @@ -564,7 +567,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) } } - d = virtio_scsi_device_find(s, req->req.cmd.lun); + d = virtio_scsi_device_get(s, req->req.cmd.lun); if (!d) { req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET; virtio_scsi_complete_cmd_req(req); @@ -580,10 +583,12 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) req->sreq->cmd.xfer > req->qsgl.size)) { req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN; virtio_scsi_complete_cmd_req(req); + object_unref(OBJECT(d)); return -ENOBUFS; } scsi_req_ref(req->sreq); blk_io_plug(d->conf.blk); + object_unref(OBJECT(d)); return 0; } From 8cfe8013baec2a6f66240ffd767fad2699d85144 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Tue, 6 Oct 2020 15:39:04 +0300 Subject: [PATCH 37/38] scsi/scsi_bus: fix races in REPORT LUNS Currently scsi_target_emulate_report_luns iterates over the child device list twice, and there is no guarantee that this list is the same in both iterations. The reason for iterating twice is that the first iteration calculates how much memory to allocate. However if we use a dynamic array we can avoid iterating twice, and therefore we avoid this race. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707 Signed-off-by: Maxim Levitsky Reviewed-by: Stefan Hajnoczi Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini Message-Id: <20201006123904.610658-14-mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-bus.c | 70 ++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index eda8cb7e70..b901e701f0 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -438,19 +438,23 @@ struct SCSITargetReq { static void store_lun(uint8_t *outbuf, int lun) { if (lun < 256) { + /* Simple logical unit addressing method*/ + outbuf[0] = 0; outbuf[1] = lun; - return; + } else { + /* Flat space addressing method */ + outbuf[0] = 0x40 | (lun >> 8); + outbuf[1] = (lun & 255); } - outbuf[1] = (lun & 255); - outbuf[0] = (lun >> 8) | 0x40; } static bool scsi_target_emulate_report_luns(SCSITargetReq *r) { BusChild *kid; - int i, len, n; int channel, id; - bool found_lun0; + uint8_t tmp[8] = {0}; + int len = 0; + GByteArray *buf; if (r->req.cmd.xfer < 16) { return false; @@ -458,46 +462,40 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) if (r->req.cmd.buf[2] > 2) { return false; } + + /* reserve space for 63 LUNs*/ + buf = g_byte_array_sized_new(512); + channel = r->req.dev->channel; id = r->req.dev->id; - found_lun0 = false; - n = 0; - RCU_READ_LOCK_GUARD(); + /* add size (will be updated later to correct value */ + g_byte_array_append(buf, tmp, 8); + len += 8; - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { - DeviceState *qdev = kid->child; - SCSIDevice *dev = SCSI_DEVICE(qdev); + /* add LUN0 */ + g_byte_array_append(buf, tmp, 8); + len += 8; - if (dev->channel == channel && dev->id == id) { - if (dev->lun == 0) { - found_lun0 = true; + WITH_RCU_READ_LOCK_GUARD() { + QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { + DeviceState *qdev = kid->child; + SCSIDevice *dev = SCSI_DEVICE(qdev); + + if (dev->channel == channel && dev->id == id && dev->lun != 0) { + store_lun(tmp, dev->lun); + g_byte_array_append(buf, tmp, 8); + len += 8; } - n += 8; - } - } - if (!found_lun0) { - n += 8; - } - - scsi_target_alloc_buf(&r->req, n + 8); - - len = MIN(n + 8, r->req.cmd.xfer & ~7); - memset(r->buf, 0, len); - stl_be_p(&r->buf[0], n); - i = found_lun0 ? 8 : 16; - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { - DeviceState *qdev = kid->child; - SCSIDevice *dev = SCSI_DEVICE(qdev); - - if (dev->channel == channel && dev->id == id) { - store_lun(&r->buf[i], dev->lun); - i += 8; } } - assert(i == n + 8); - r->len = len; + r->buf_len = len; + r->buf = g_byte_array_free(buf, FALSE); + r->len = MIN(len, r->req.cmd.xfer & ~7); + + /* store the LUN list length */ + stl_be_p(&r->buf[0], len - 8); return true; } From a0c9162c8250e121af438aee5ef93e64ec62dae1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 7 Oct 2020 11:01:51 -0400 Subject: [PATCH 38/38] meson: identify more sections of meson.build Add more headers that clarify code organization. Signed-off-by: Paolo Bonzini --- meson.build | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/meson.build b/meson.build index 0c676af194..652c37cceb 100644 --- a/meson.build +++ b/meson.build @@ -614,7 +614,9 @@ if not has_malloc_trim and get_option('malloc_trim').enabled() endif endif -# Create config-host.h +################# +# config-host.h # +################# config_host_data.set('CONFIG_COCOA', cocoa.found()) config_host_data.set('CONFIG_LIBUDEV', libudev.found()) @@ -660,6 +662,10 @@ foreach k, v: config_host endif endforeach +######################## +# Target configuration # +######################## + minikconf = find_program('scripts/minikconf.py') config_all = {} config_all_devices = {} @@ -866,7 +872,9 @@ config_all += { 'CONFIG_ALL': true, } -# Submodules +############## +# Submodules # +############## capstone = not_found capstone_opt = get_option('capstone') @@ -1105,9 +1113,11 @@ config_host_data.set('CONFIG_CAPSTONE', capstone.found()) config_host_data.set('CONFIG_FDT', fdt.found()) config_host_data.set('CONFIG_SLIRP', slirp.found()) -genh += configure_file(output: 'config-host.h', configuration: config_host_data) +##################### +# Generated sources # +##################### -# Generators +genh += configure_file(output: 'config-host.h', configuration: config_host_data) hxtool = find_program('scripts/hxtool') shaderinclude = find_program('scripts/shaderinclude.pl') @@ -1182,7 +1192,9 @@ sphinx_extn_depends = [ meson.source_root() / 'docs/sphinx/depfile.py', meson.source_root() / 'docs/sphinx/qmp_lexer.py', qapi_gen_depends ] -# Collect sourcesets. +################### +# Collect sources # +################### authz_ss = ss.source_set() blockdev_ss = ss.source_set() @@ -1320,8 +1332,6 @@ if enable_modules modulecommon = declare_dependency(link_whole: libmodulecommon, compile_args: '-DBUILD_DSO') endif -# Build targets from sourcesets - stub_ss = stub_ss.apply(config_all, strict: false) util_ss.add_all(trace_ss) @@ -1409,6 +1419,10 @@ specific_ss.add_all(when: 'CONFIG_LINUX_USER', if_true: linux_user_ss) subdir('tests/qtest/libqos') subdir('tests/qtest/fuzz') +######################## +# Library dependencies # +######################## + block_mods = [] softmmu_mods = [] foreach d, list : modules @@ -1443,10 +1457,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms', capture: true, command: [undefsym, nm, '@INPUT@']) -######################## -# Library dependencies # -######################## - qom_ss = qom_ss.apply(config_host, strict: false) libqom = static_library('qom', qom_ss.sources() + genh, dependencies: [qom_ss.dependencies()], @@ -1797,6 +1807,10 @@ if host_machine.system() == 'windows' alias_target('installer', nsis) endif +######################### +# Configuration summary # +######################### + summary_info = {} summary_info += {'Install prefix': config_host['prefix']} summary_info += {'BIOS directory': config_host['qemu_datadir']}