From ac98fa849e834f48e5a64cf4b22218ba4047e142 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 8 Oct 2015 18:11:39 +0200 Subject: [PATCH 01/12] update-linux-headers: Rename SW_MAX to SW_MAX_ The next commit will compile hw/input/virtio-input.c and hw/input/virtio-input-hid.c even when CONFIG_LINUX is off. These files include both "include/standard-headers/linux/input.h" and then. Doesn't work, because both define SW_MAX. We don't actually use it. Patch input.h to define SW_MAX_ instead. Signed-off-by: Markus Armbruster Message-Id: <1444320700-26260-2-git-send-email-armbru@redhat.com> Reviewed-by: Gerd Hoffmann --- include/standard-headers/linux/input.h | 4 ++-- scripts/update-linux-headers.sh | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/standard-headers/linux/input.h b/include/standard-headers/linux/input.h index b003c67059..43f1850b6b 100644 --- a/include/standard-headers/linux/input.h +++ b/include/standard-headers/linux/input.h @@ -887,8 +887,8 @@ struct input_keymap_entry { #define SW_ROTATE_LOCK 0x0c /* set = rotate locked/disabled */ #define SW_LINEIN_INSERT 0x0d /* set = inserted */ #define SW_MUTE_DEVICE 0x0e /* set = device disabled */ -#define SW_MAX 0x0f -#define SW_CNT (SW_MAX+1) +#define SW_MAX_ 0x0f +#define SW_CNT (SW_MAX_+1) /* * Misc events diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh index 1107619121..457ef37b95 100755 --- a/scripts/update-linux-headers.sh +++ b/scripts/update-linux-headers.sh @@ -53,6 +53,7 @@ cp_portable() { -e 's/__attribute__((packed))/QEMU_PACKED/' \ -e 's/__inline__/inline/' \ -e '/sys\/ioctl.h/d' \ + -e 's/SW_MAX/SW_MAX_/' \ "$f" > "$to/$header"; } From c6047e9621f77a65993bcda8f58b676996e24bb5 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 8 Oct 2015 18:11:40 +0200 Subject: [PATCH 02/12] virtio-input: Fix device introspection on non-Linux hosts When CONFIG_LINUX is off, devices "virtio-keyboard-device", "virtio-mouse-device", "virtio-tablet-device" and "virtio-input-host-device" aren't compiled in, yet "virtio-keyboard-pci", "virtio-mouse-pci", "virtio-tablet-pci" and "virtio-input-host-pci" still are. Attempts to introspect them crash, e.g. $ qemu-system-x86_64 -device virtio-tablet-pci,help ** ERROR:/work/armbru/qemu/qom/object.c:333:object_initialize_with_type: assertion failed: (type != NULL) Broken in commit 710e2d9 and commit 006a5ed. Fix by compiling the "virtio-FOO-pci" exactly when compiling the "virtio-FOO-device": compile "virtio-keyboard-device", "virtio-mouse-device", "virtio-tablet-device" regardless of CONFIG_LINUX, and compile "virtio-input-host-pci" only for CONFIG_LINUX. Reported-by: Peter Maydell Signed-off-by: Markus Armbruster Reviewed-by: Gerd Hoffmann Message-Id: <1444320700-26260-3-git-send-email-armbru@redhat.com> --- hw/input/Makefile.objs | 2 +- hw/virtio/virtio-pci.c | 20 ++++++++++++-------- hw/virtio/virtio-pci.h | 4 ++++ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/hw/input/Makefile.objs b/hw/input/Makefile.objs index 624ba7ea40..7715d7230d 100644 --- a/hw/input/Makefile.objs +++ b/hw/input/Makefile.objs @@ -8,9 +8,9 @@ common-obj-$(CONFIG_STELLARIS_INPUT) += stellaris_input.o common-obj-$(CONFIG_TSC2005) += tsc2005.o common-obj-$(CONFIG_VMMOUSE) += vmmouse.o -ifeq ($(CONFIG_LINUX),y) common-obj-$(CONFIG_VIRTIO) += virtio-input.o common-obj-$(CONFIG_VIRTIO) += virtio-input-hid.o +ifeq ($(CONFIG_LINUX),y) common-obj-$(CONFIG_VIRTIO) += virtio-input-host.o endif diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 6703806f83..e5c406d1d2 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2134,14 +2134,6 @@ static void virtio_tablet_initfn(Object *obj) TYPE_VIRTIO_TABLET); } -static void virtio_host_initfn(Object *obj) -{ - VirtIOInputHostPCI *dev = VIRTIO_INPUT_HOST_PCI(obj); - - virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev), - TYPE_VIRTIO_INPUT_HOST); -} - static const TypeInfo virtio_input_pci_info = { .name = TYPE_VIRTIO_INPUT_PCI, .parent = TYPE_VIRTIO_PCI, @@ -2180,12 +2172,22 @@ static const TypeInfo virtio_tablet_pci_info = { .instance_init = virtio_tablet_initfn, }; +#ifdef CONFIG_LINUX +static void virtio_host_initfn(Object *obj) +{ + VirtIOInputHostPCI *dev = VIRTIO_INPUT_HOST_PCI(obj); + + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev), + TYPE_VIRTIO_INPUT_HOST); +} + static const TypeInfo virtio_host_pci_info = { .name = TYPE_VIRTIO_INPUT_HOST_PCI, .parent = TYPE_VIRTIO_INPUT_PCI, .instance_size = sizeof(VirtIOInputHostPCI), .instance_init = virtio_host_initfn, }; +#endif /* virtio-pci-bus */ @@ -2233,7 +2235,9 @@ static void virtio_pci_register_types(void) type_register_static(&virtio_keyboard_pci_info); type_register_static(&virtio_mouse_pci_info); type_register_static(&virtio_tablet_pci_info); +#ifdef CONFIG_LINUX type_register_static(&virtio_host_pci_info); +#endif type_register_static(&virtio_pci_bus_info); type_register_static(&virtio_pci_info); #ifdef CONFIG_VIRTFS diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index b6c442f522..801c23aef3 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -267,6 +267,8 @@ struct VirtIOInputHIDPCI { VirtIOInputHID vdev; }; +#ifdef CONFIG_LINUX + #define TYPE_VIRTIO_INPUT_HOST_PCI "virtio-input-host-pci" #define VIRTIO_INPUT_HOST_PCI(obj) \ OBJECT_CHECK(VirtIOInputHostPCI, (obj), TYPE_VIRTIO_INPUT_HOST_PCI) @@ -276,6 +278,8 @@ struct VirtIOInputHostPCI { VirtIOInputHost vdev; }; +#endif + /* * virtio-gpu-pci: This extends VirtioPCIProxy. */ From 2e2b8eb70fdb7dfbec39f3a19b20f9a73f2f813e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Oct 2015 10:59:50 +0200 Subject: [PATCH 03/12] memory: allow destroying a non-empty MemoryRegion This is legal; the MemoryRegion will simply unreference all the existing subregions and possibly bring them down with it as well. However, it requires a bit of care to avoid an infinite loop. Finalizing a memory region cannot trigger an address space update, but memory_region_del_subregion errs on the side of caution and might trigger a spurious update: avoid that by resetting mr->enabled first. Signed-off-by: Paolo Bonzini Signed-off-by: Markus Armbruster Message-Id: <1443689999-12182-2-git-send-email-armbru@redhat.com> --- memory.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/memory.c b/memory.c index 1b03d2251c..2eb1597518 100644 --- a/memory.c +++ b/memory.c @@ -1304,7 +1304,22 @@ static void memory_region_finalize(Object *obj) { MemoryRegion *mr = MEMORY_REGION(obj); - assert(QTAILQ_EMPTY(&mr->subregions)); + assert(!mr->container); + + /* We know the region is not visible in any address space (it + * does not have a container and cannot be a root either because + * it has no references, so we can blindly clear mr->enabled. + * memory_region_set_enabled instead could trigger a transaction + * and cause an infinite loop. + */ + mr->enabled = false; + memory_region_transaction_begin(); + while (!QTAILQ_EMPTY(&mr->subregions)) { + MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); + memory_region_del_subregion(mr, subregion); + } + memory_region_transaction_commit(); + mr->destructor(mr); memory_region_clear_coalescing(mr); g_free((char *)mr->name); From 81e0ab48dda611e9571dc2e166840205a4208567 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Oct 2015 10:59:51 +0200 Subject: [PATCH 04/12] hw: do not pass NULL to memory_region_init from instance_init This causes the region to outlive the object, because it attaches the region to /machine. This is not nice for the "realize" method, but much worse for "instance_init" because it can cause dangling pointers after a simple object_new/object_unref pair. Reported-by: Markus Armbruster Signed-off-by: Paolo Bonzini Reviewed-by: Peter Maydell Tested-by: Markus Armbruster Signed-off-by: Markus Armbruster Message-Id: <1443689999-12182-3-git-send-email-armbru@redhat.com> Reviewed-by: Thomas Huth --- hw/arm/pxa2xx.c | 2 +- hw/display/cg3.c | 4 ++-- hw/display/tcx.c | 2 +- hw/misc/arm_integrator_debug.c | 2 +- hw/misc/macio/cuda.c | 2 +- hw/misc/macio/macio.c | 6 +++--- hw/pcmcia/pxa2xx.c | 6 +++--- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c index 164260a9b6..79d22d91e5 100644 --- a/hw/arm/pxa2xx.c +++ b/hw/arm/pxa2xx.c @@ -1958,7 +1958,7 @@ static void pxa2xx_fir_instance_init(Object *obj) PXA2xxFIrState *s = PXA2XX_FIR(obj); SysBusDevice *sbd = SYS_BUS_DEVICE(obj); - memory_region_init_io(&s->iomem, NULL, &pxa2xx_fir_ops, s, + memory_region_init_io(&s->iomem, obj, &pxa2xx_fir_ops, s, "pxa2xx-fir", 0x1000); sysbus_init_mmio(sbd, &s->iomem); sysbus_init_irq(sbd, &s->irq); diff --git a/hw/display/cg3.c b/hw/display/cg3.c index d2a0d97320..e309fbe92e 100644 --- a/hw/display/cg3.c +++ b/hw/display/cg3.c @@ -280,12 +280,12 @@ static void cg3_initfn(Object *obj) SysBusDevice *sbd = SYS_BUS_DEVICE(obj); CG3State *s = CG3(obj); - memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE, + memory_region_init_ram(&s->rom, obj, "cg3.prom", FCODE_MAX_ROM_SIZE, &error_fatal); memory_region_set_readonly(&s->rom, true); sysbus_init_mmio(sbd, &s->rom); - memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg", + memory_region_init_io(&s->reg, obj, &cg3_reg_ops, s, "cg3.reg", CG3_REG_SIZE); sysbus_init_mmio(sbd, &s->reg); } diff --git a/hw/display/tcx.c b/hw/display/tcx.c index 463580094a..bf119bc89a 100644 --- a/hw/display/tcx.c +++ b/hw/display/tcx.c @@ -944,7 +944,7 @@ static void tcx_initfn(Object *obj) SysBusDevice *sbd = SYS_BUS_DEVICE(obj); TCXState *s = TCX(obj); - memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE, + memory_region_init_ram(&s->rom, OBJECT(s), "tcx.prom", FCODE_MAX_ROM_SIZE, &error_fatal); memory_region_set_readonly(&s->rom, true); sysbus_init_mmio(sbd, &s->rom); diff --git a/hw/misc/arm_integrator_debug.c b/hw/misc/arm_integrator_debug.c index 99b720fbb9..6d9dd74e38 100644 --- a/hw/misc/arm_integrator_debug.c +++ b/hw/misc/arm_integrator_debug.c @@ -79,7 +79,7 @@ static void intdbg_control_init(Object *obj) SysBusDevice *sd = SYS_BUS_DEVICE(obj); IntegratorDebugState *s = INTEGRATOR_DEBUG(obj); - memory_region_init_io(&s->iomem, NULL, &intdbg_control_ops, + memory_region_init_io(&s->iomem, obj, &intdbg_control_ops, NULL, "dbg-leds", 0x1000000); sysbus_init_mmio(sd, &s->iomem); } diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c index f3984e3a20..5d7043e99c 100644 --- a/hw/misc/macio/cuda.c +++ b/hw/misc/macio/cuda.c @@ -713,7 +713,7 @@ static void cuda_initfn(Object *obj) CUDAState *s = CUDA(obj); int i; - memory_region_init_io(&s->mem, NULL, &cuda_ops, s, "cuda", 0x2000); + memory_region_init_io(&s->mem, obj, &cuda_ops, s, "cuda", 0x2000); sysbus_init_mmio(d, &s->mem); sysbus_init_irq(d, &s->irq); diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c index e3c0242d41..2548d966c9 100644 --- a/hw/misc/macio/macio.c +++ b/hw/misc/macio/macio.c @@ -105,10 +105,10 @@ static void macio_escc_legacy_setup(MacIOState *macio_state) 0xF0, 0xE0, }; - memory_region_init(escc_legacy, NULL, "escc-legacy", 256); + memory_region_init(escc_legacy, OBJECT(macio_state), "escc-legacy", 256); for (i = 0; i < ARRAY_SIZE(maps); i += 2) { MemoryRegion *port = g_new(MemoryRegion, 1); - memory_region_init_alias(port, NULL, "escc-legacy-port", + memory_region_init_alias(port, OBJECT(macio_state), "escc-legacy-port", macio_state->escc_mem, maps[i+1], 0x2); memory_region_add_subregion(escc_legacy, maps[i], port); } @@ -330,7 +330,7 @@ static void macio_instance_init(Object *obj) MacIOState *s = MACIO(obj); MemoryRegion *dbdma_mem; - memory_region_init(&s->bar, NULL, "macio", 0x80000); + memory_region_init(&s->bar, obj, "macio", 0x80000); object_initialize(&s->cuda, sizeof(s->cuda), TYPE_CUDA); qdev_set_parent_bus(DEVICE(&s->cuda), sysbus_get_default()); diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c index a7e187743d..812716e1c8 100644 --- a/hw/pcmcia/pxa2xx.c +++ b/hw/pcmcia/pxa2xx.c @@ -163,7 +163,7 @@ static void pxa2xx_pcmcia_initfn(Object *obj) sysbus_init_mmio(sbd, &s->container_mem); /* Socket I/O Memory Space */ - memory_region_init_io(&s->iomem, NULL, &pxa2xx_pcmcia_io_ops, s, + memory_region_init_io(&s->iomem, obj, &pxa2xx_pcmcia_io_ops, s, "pxa2xx-pcmcia-io", 0x04000000); memory_region_add_subregion(&s->container_mem, 0x00000000, &s->iomem); @@ -171,13 +171,13 @@ static void pxa2xx_pcmcia_initfn(Object *obj) /* Then next 64 MB is reserved */ /* Socket Attribute Memory Space */ - memory_region_init_io(&s->attr_iomem, NULL, &pxa2xx_pcmcia_attr_ops, s, + memory_region_init_io(&s->attr_iomem, obj, &pxa2xx_pcmcia_attr_ops, s, "pxa2xx-pcmcia-attribute", 0x04000000); memory_region_add_subregion(&s->container_mem, 0x08000000, &s->attr_iomem); /* Socket Common Memory Space */ - memory_region_init_io(&s->common_iomem, NULL, &pxa2xx_pcmcia_common_ops, s, + memory_region_init_io(&s->common_iomem, obj, &pxa2xx_pcmcia_common_ops, s, "pxa2xx-pcmcia-common", 0x04000000); memory_region_add_subregion(&s->container_mem, 0x0c000000, &s->common_iomem); From c7104402353bf32ac1d3a276e3619a20e910506b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Oct 2015 10:59:52 +0200 Subject: [PATCH 05/12] macio: move DBDMA_init from instance_init to realize DBDMA_init is not idempotent, and calling it from instance_init breaks a simple object_new/object_unref pair. Work around this, pending qdev-ification of DBDMA, by moving the call to realize. Reported-by: Markus Armbruster Signed-off-by: Paolo Bonzini Reviewed-by: Thomas Huth Signed-off-by: Markus Armbruster Message-Id: <1443689999-12182-4-git-send-email-armbru@redhat.com> --- hw/misc/macio/macio.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c index 2548d966c9..c661f86c21 100644 --- a/hw/misc/macio/macio.c +++ b/hw/misc/macio/macio.c @@ -131,6 +131,10 @@ static void macio_common_realize(PCIDevice *d, Error **errp) MacIOState *s = MACIO(d); SysBusDevice *sysbus_dev; Error *err = NULL; + MemoryRegion *dbdma_mem; + + s->dbdma = DBDMA_init(&dbdma_mem); + memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem); object_property_set_bool(OBJECT(&s->cuda), true, "realized", &err); if (err) { @@ -328,16 +332,12 @@ static void macio_newworld_init(Object *obj) static void macio_instance_init(Object *obj) { MacIOState *s = MACIO(obj); - MemoryRegion *dbdma_mem; memory_region_init(&s->bar, obj, "macio", 0x80000); object_initialize(&s->cuda, sizeof(s->cuda), TYPE_CUDA); qdev_set_parent_bus(DEVICE(&s->cuda), sysbus_get_default()); object_property_add_child(obj, "cuda", OBJECT(&s->cuda), NULL); - - s->dbdma = DBDMA_init(&dbdma_mem); - memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem); } static const VMStateDescription vmstate_macio_oldworld = { From e253c287153c6f3ce4177686ac12c196f9bd8292 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:53 +0200 Subject: [PATCH 06/12] tests: Fix how qom-test is run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We want to run qom-test for every architecture, without having to manually add it to every architecture's list of tests. Commit 3687d53 accomplished this by adding it to every architecture's list automatically. However, some architectures inherit their tests from others, like this: check-qtest-x86_64-y = $(check-qtest-i386-y) check-qtest-microblazeel-y = $(check-qtest-microblaze-y) check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) For such architectures, we ended up running the (slow!) test twice. Commit 2b8419c attempted to avoid this by adding the test only when it's not already present. Works only as long as we consider adding the test to the architectures on the left hand side *after* the ones on the right hand side: x86_64 after i386, microblazeel after microblaze, xtensaeb after xtensa. Turns out we consider them in $(SYSEMU_TARGET_LIST) order. Defined as SYSEMU_TARGET_LIST := $(subst -softmmu.mak,,$(notdir \ $(wildcard $(SRC_PATH)/default-configs/*-softmmu.mak))) On my machine, this results in the oder xtensa, x86_64, microblazeel, microblaze, i386. Consequently, qom-test runs twice for microblazeel and x86_64. Replace this complex and flawed machinery with a much simpler one: add generic tests (currently just qom-test) to check-qtest-generic-y instead of check-qtest-$(target)-y for every target, then run $(check-qtest-generic-y) for every target. Signed-off-by: Markus Armbruster Reviewed-by: Andreas Färber Message-Id: <1443689999-12182-5-git-send-email-armbru@redhat.com> --- tests/Makefile | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/Makefile b/tests/Makefile index e6474ba31b..c32a43ed2c 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -86,6 +86,8 @@ check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh # All QTests for now are POSIX-only, but the dependencies are # really in libqtest, not in the testcases themselves. +check-qtest-generic-y = + gcov-files-ipack-y += hw/ipack/ipack.c check-qtest-ipack-y += tests/ipoctal232-test$(EXESUF) gcov-files-ipack-y += hw/char/ipoctal232.c @@ -218,10 +220,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c check-qtest-microblazeel-y = $(check-qtest-microblaze-y) check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) -# qom-test works for all sysemu architectures: -$(foreach target,$(SYSEMU_TARGET_LIST), \ - $(if $(findstring tests/qom-test$(EXESUF), $(check-qtest-$(target)-y)),, \ - $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF)))) +check-qtest-generic-y += tests/qom-test$(EXESUF) check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ comments.json empty.json enum-empty.json enum-missing-data.json \ @@ -448,8 +447,11 @@ CFLAGS += $(TEST_CFLAGS) TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS))) ifeq ($(CONFIG_POSIX),y) -QTEST_TARGETS=$(foreach TARGET,$(TARGETS), $(if $(check-qtest-$(TARGET)-y), $(TARGET),)) +QTEST_TARGETS = $(TARGETS) check-qtest-y=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y)) +check-qtest-y += $(check-qtest-generic-y) +else +QTEST_TARGETS = endif qtest-obj-y = tests/libqtest.o $(test-util-obj-y) @@ -487,7 +489,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y) $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \ QTEST_QEMU_IMG=qemu-img$(EXESUF) \ MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \ - gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@") + gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER $@") $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \ echo Gcov report for $$f:;\ $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \ From 82b15c7bdbda6207d1fee2ec824432e64af3ecb4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:54 +0200 Subject: [PATCH 07/12] libqtest: Clean up unused QTestState member sigact_old Unused since commit d766825. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <1443689999-12182-6-git-send-email-armbru@redhat.com> Reviewed-by: Thomas Huth --- tests/libqtest.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/libqtest.c b/tests/libqtest.c index e5188e0327..8dede56929 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -46,7 +46,6 @@ struct QTestState bool irq_level[MAX_IRQ]; GString *rx; pid_t qemu_pid; /* our child QEMU process */ - struct sigaction sigact_old; /* restored on exit */ }; static GList *qtest_instances; From 5fb48d9673b76fc53507a0e717a12968e57d846e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:55 +0200 Subject: [PATCH 08/12] libqtest: New hmp() & friends New convenience function hmp() to facilitate use of human-monitor-command in tests. Use it to simplify its existing uses. To blend into existing libqtest code, also add qtest_hmpv() and qtest_hmp(). That, and the egregiously verbose GTK-Doc comment format make this patch look bigger than it is. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Thomas Huth Message-Id: <1443689999-12182-7-git-send-email-armbru@redhat.com> --- tests/drive_del-test.c | 22 ++++++---------------- tests/ide-test.c | 8 ++------ tests/libqtest.c | 37 +++++++++++++++++++++++++++++++++++++ tests/libqtest.h | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 22 deletions(-) diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c index 8951f6f610..33909469f1 100644 --- a/tests/drive_del-test.c +++ b/tests/drive_del-test.c @@ -16,28 +16,18 @@ static void drive_add(void) { - QDict *response; + char *resp = hmp("drive_add 0 if=none,id=drive0"); - response = qmp("{'execute': 'human-monitor-command'," - " 'arguments': {" - " 'command-line': 'drive_add 0 if=none,id=drive0'" - "}}"); - g_assert(response); - g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n"); - QDECREF(response); + g_assert_cmpstr(resp, ==, "OK\r\n"); + g_free(resp); } static void drive_del(void) { - QDict *response; + char *resp = hmp("drive_del drive0"); - response = qmp("{'execute': 'human-monitor-command'," - " 'arguments': {" - " 'command-line': 'drive_del drive0'" - "}}"); - g_assert(response); - g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, ""); - QDECREF(response); + g_assert_cmpstr(resp, ==, ""); + g_free(resp); } static void device_del(void) diff --git a/tests/ide-test.c b/tests/ide-test.c index b6e9e1a232..d1014bbc46 100644 --- a/tests/ide-test.c +++ b/tests/ide-test.c @@ -510,9 +510,7 @@ static void test_flush(void) tmp_path); /* Delay the completion of the flush request until we explicitly do it */ - qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {" - " 'command-line':" - " 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }"); + g_free(hmp("qemu-io ide0-hd0 \"break flush_to_os A\"")); /* FLUSH CACHE command on device 0*/ outb(IDE_BASE + reg_device, 0); @@ -524,9 +522,7 @@ static void test_flush(void) assert_bit_clear(data, DF | ERR | DRQ); /* Complete the command */ - qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {" - " 'command-line':" - " 'qemu-io ide0-hd0 \"resume A\"'} }"); + g_free(hmp("qemu-io ide0-hd0 \"resume A\"")); /* Check registers */ data = inb(IDE_BASE + reg_device); diff --git a/tests/libqtest.c b/tests/libqtest.c index 8dede56929..2a396ba08d 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -483,6 +483,33 @@ void qtest_qmp_eventwait(QTestState *s, const char *event) } } +char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap) +{ + char *cmd; + QDict *resp; + char *ret; + + cmd = g_strdup_vprintf(fmt, ap); + resp = qtest_qmp(s, "{'execute': 'human-monitor-command'," + " 'arguments': {'command-line': %s}}", + cmd); + ret = g_strdup(qdict_get_try_str(resp, "return")); + g_assert(ret); + QDECREF(resp); + g_free(cmd); + return ret; +} + +char *qtest_hmp(QTestState *s, const char *fmt, ...) +{ + va_list ap; + char *ret; + + va_start(ap, fmt); + ret = qtest_hmpv(s, fmt, ap); + va_end(ap); + return ret; +} const char *qtest_get_arch(void) { @@ -774,6 +801,16 @@ void qmp_discard_response(const char *fmt, ...) qtest_qmpv_discard_response(global_qtest, fmt, ap); va_end(ap); } +char *hmp(const char *fmt, ...) +{ + va_list ap; + char *ret; + + va_start(ap, fmt); + ret = qtest_hmpv(global_qtest, fmt, ap); + va_end(ap); + return ret; +} bool qtest_big_endian(void) { diff --git a/tests/libqtest.h b/tests/libqtest.h index ec42031523..55bccbf0e6 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -119,6 +119,29 @@ QDict *qtest_qmp_receive(QTestState *s); */ void qtest_qmp_eventwait(QTestState *s, const char *event); +/** + * qtest_hmpv: + * @s: #QTestState instance to operate on. + * @fmt...: HMP command to send to QEMU + * + * Send HMP command to QEMU via QMP's human-monitor-command. + * + * Returns: the command's output. The caller should g_free() it. + */ +char *qtest_hmp(QTestState *s, const char *fmt, ...); + +/** + * qtest_hmpv: + * @s: #QTestState instance to operate on. + * @fmt: HMP command to send to QEMU + * @ap: HMP command arguments + * + * Send HMP command to QEMU via QMP's human-monitor-command. + * + * Returns: the command's output. The caller should g_free() it. + */ +char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap); + /** * qtest_get_irq: * @s: #QTestState instance to operate on. @@ -498,6 +521,16 @@ static inline void qmp_eventwait(const char *event) return qtest_qmp_eventwait(global_qtest, event); } +/** + * hmp: + * @fmt...: HMP command to send to QEMU + * + * Send HMP command to QEMU via QMP's human-monitor-command. + * + * Returns: the command's output. The caller should g_free() it. + */ +char *hmp(const char *fmt, ...); + /** * get_irq: * @num: Interrupt to observe. From 2d1abb850fd15fd6eb75a92290be5f93b2772ec5 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:56 +0200 Subject: [PATCH 09/12] device-introspect-test: New, covering device introspection The test doesn't check that the output makes any sense, only that QEMU survives. Useful since we've had an astounding number of crash bugs around there. In fact, we have a bunch of them right now: a few devices crash or hang, and some leave dangling pointers behind. The test skips testing the broken parts. The next commits will fix them up, and drop the skipping. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <1443689999-12182-8-git-send-email-armbru@redhat.com> --- tests/Makefile | 8 +- tests/device-introspect-test.c | 158 +++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 tests/device-introspect-test.c diff --git a/tests/Makefile b/tests/Makefile index c32a43ed2c..5a4732f75a 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -86,7 +86,8 @@ check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh # All QTests for now are POSIX-only, but the dependencies are # really in libqtest, not in the testcases themselves. -check-qtest-generic-y = +check-qtest-generic-y = tests/device-introspect-test$(EXESUF) +gcov-files-generic-y = qdev-monitor.c qmp.c gcov-files-ipack-y += hw/ipack/ipack.c check-qtest-ipack-y += tests/ipoctal232-test$(EXESUF) @@ -383,6 +384,7 @@ libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o libqos-usb-obj-y = $(libqos-pc-obj-y) tests/libqos/usb.o libqos-virtio-obj-y = $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o +tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o tests/rtc-test$(EXESUF): tests/rtc-test.o tests/m48t59-test$(EXESUF): tests/m48t59-test.o tests/endianness-test$(EXESUF): tests/endianness-test.o @@ -490,7 +492,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y) QTEST_QEMU_IMG=qemu-img$(EXESUF) \ MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \ gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER $@") - $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \ + $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \ echo Gcov report for $$f:;\ $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \ done,) @@ -501,7 +503,7 @@ $(patsubst %, check-%, $(check-unit-y)): check-%: % $(call quiet-command, \ MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \ gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER $*") - $(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y); do \ + $(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) $(gcov-files-generic-y); do \ echo Gcov report for $$f:;\ $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \ done,) diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c new file mode 100644 index 0000000000..f240b5c827 --- /dev/null +++ b/tests/device-introspect-test.c @@ -0,0 +1,158 @@ +/* + * Device introspection test cases + * + * Copyright (c) 2015 Red Hat Inc. + * + * Authors: + * Markus Armbruster , + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +/* + * Covers QMP device-list-properties and HMP device_add help. We + * currently don't check that their output makes sense, only that QEMU + * survives. Useful since we've had an astounding number of crash + * bugs around here. + */ + +#include +#include +#include "qemu-common.h" +#include "qapi/qmp/qstring.h" +#include "libqtest.h" + +const char common_args[] = "-nodefaults -machine none"; + +static QList *device_type_list(bool abstract) +{ + QDict *resp; + QList *ret; + + resp = qmp("{'execute': 'qom-list-types'," + " 'arguments': {'implements': 'device', 'abstract': %i}}", + abstract); + g_assert(qdict_haskey(resp, "return")); + ret = qdict_get_qlist(resp, "return"); + QINCREF(ret); + QDECREF(resp); + return ret; +} + +static void test_one_device(const char *type) +{ + QDict *resp; + char *help, *qom_tree; + + /* + * Skip this part for the abstract device test case, because + * device-list-properties crashes for such devices. + * FIXME fix it not to crash + */ + if (strcmp(type, "device")) { + resp = qmp("{'execute': 'device-list-properties'," + " 'arguments': {'typename': %s}}", + type); + QDECREF(resp); + } + + help = hmp("device_add \"%s,help\"", type); + g_free(help); + + /* + * Some devices leave dangling pointers in QOM behind. + * "info qom-tree" has a good chance at crashing then + */ + qom_tree = hmp("info qom-tree"); + g_free(qom_tree); +} + +static void test_device_intro_list(void) +{ + QList *types; + char *help; + + qtest_start(common_args); + + types = device_type_list(true); + QDECREF(types); + + help = hmp("device_add help"); + g_free(help); + + qtest_end(); +} + +static void test_device_intro_none(void) +{ + qtest_start(common_args); + test_one_device("nonexistent"); + qtest_end(); +} + +static void test_device_intro_abstract(void) +{ + qtest_start(common_args); + test_one_device("device"); + qtest_end(); +} + +static bool blacklisted(const char *type) +{ + static const char *blacklist[] = { + /* hang in object_unref(): */ + "realview_pci", "versatile_pci", + /* create a CPU, thus use after free (see below): */ + "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", + }; + size_t len = strlen(type); + int i; + + if (len >= 4 && !strcmp(type + len - 4, "-cpu")) { + /* use after free: cpu_exec_init() saves CPUState in cpus */ + return true; + } + + for (i = 0; i < ARRAY_SIZE(blacklist); i++) { + if (!strcmp(blacklist[i], type)) { + return true; + } + } + return false; +} + +static void test_device_intro_concrete(void) +{ + QList *types; + QListEntry *entry; + const char *type; + + qtest_start(common_args); + types = device_type_list(false); + + QLIST_FOREACH_ENTRY(types, entry) { + type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)), + "name"); + g_assert(type); + if (blacklisted(type)) { + continue; /* FIXME broken device, skip */ + } + test_one_device(type); + } + + QDECREF(types); + qtest_end(); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + qtest_add_func("device/introspect/list", test_device_intro_list); + qtest_add_func("device/introspect/none", test_device_intro_none); + qtest_add_func("device/introspect/abstract", test_device_intro_abstract); + qtest_add_func("device/introspect/concrete", test_device_intro_concrete); + + return g_test_run(); +} From edb1523d90415cb79f60f83b4028ef3820d15612 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:57 +0200 Subject: [PATCH 10/12] qmp: Fix device-list-properties not to crash for abstract device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Broken in commit f4eb32b "qmp: show QOM properties in device-list-properties", v2.1. Cc: qemu-stable@nongnu.org Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Andreas Färber Message-Id: <1443689999-12182-9-git-send-email-armbru@redhat.com> --- qmp.c | 6 ++++++ tests/device-introspect-test.c | 15 ++++----------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/qmp.c b/qmp.c index 057a7cb5e2..1413de439f 100644 --- a/qmp.c +++ b/qmp.c @@ -515,6 +515,12 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, return NULL; } + if (object_class_is_abstract(klass)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name", + "non-abstract device type"); + return NULL; + } + obj = object_new(typename); QTAILQ_FOREACH(prop, &obj->properties, node) { diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c index f240b5c827..8a27b4a702 100644 --- a/tests/device-introspect-test.c +++ b/tests/device-introspect-test.c @@ -45,17 +45,10 @@ static void test_one_device(const char *type) QDict *resp; char *help, *qom_tree; - /* - * Skip this part for the abstract device test case, because - * device-list-properties crashes for such devices. - * FIXME fix it not to crash - */ - if (strcmp(type, "device")) { - resp = qmp("{'execute': 'device-list-properties'," - " 'arguments': {'typename': %s}}", - type); - QDECREF(resp); - } + resp = qmp("{'execute': 'device-list-properties'," + " 'arguments': {'typename': %s}}", + type); + QDECREF(resp); help = hmp("device_add \"%s,help\"", type); g_free(help); From 4c315c27661502a0813b129e41c0bf640c34a8d6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:58 +0200 Subject: [PATCH 11/12] qdev: Protect device-list-properties against broken devices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several devices don't survive object_unref(object_new(T)): they crash or hang during cleanup, or they leave dangling pointers behind. This breaks at least device-list-properties, because qmp_device_list_properties() needs to create a device to find its properties. Broken in commit f4eb32b "qmp: show QOM properties in device-list-properties", v2.1. Example reproducer: $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}} { "execute": "qmp_capabilities" } {"return": {}} { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } } qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed. Aborted (core dumped) [Exit 134 (SIGABRT)] Unfortunately, I can't fix the problems in these devices right now. Instead, add DeviceClass member cannot_destroy_with_object_finalize_yet to mark them: * Hang during cleanup (didn't debug, so I can't say why): "realview_pci", "versatile_pci". * Dangling pointer in cpus: most CPUs, plus "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create such CPUs * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu", "host-powerpc64-cpu", "host-embedded-powerpc-cpu", "host-powerpc-cpu" (the powerpc ones can't currently reach the assertion, because the CPUs are only registered when KVM is enabled, but the assertion is arguably in the wrong place all the same) Make qmp_device_list_properties() fail cleanly when the device is so marked. This improves device-list-properties from "crashes, hangs or leaves dangling pointers behind" to "fails". Not a complete fix, just a better-than-nothing work-around. In the above reproducer, device-list-properties now fails with "Can't list properties of device 'pxa2xx-pcmcia'". This also protects -device FOO,help, which uses the same machinery since commit ef52358 "qdev-monitor: include QOM properties in -device FOO, help output", v2.2. Example reproducer: $ qemu-system-aarch64 -machine none -device pxa2xx-pcmcia,help Before: qemu-system-aarch64: .../memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed. After: Can't list properties of device 'pxa2xx-pcmcia' Cc: "Andreas Färber" Cc: "Edgar E. Iglesias" Cc: Alexander Graf Cc: Anthony Green Cc: Aurelien Jarno Cc: Bastian Koppelmann Cc: Blue Swirl Cc: Eduardo Habkost Cc: Guan Xuetao Cc: Jia Liu Cc: Leon Alrae Cc: Mark Cave-Ayland Cc: Max Filippov Cc: Michael Walle Cc: Paolo Bonzini Cc: Peter Maydell Cc: Richard Henderson Cc: qemu-ppc@nongnu.org Cc: qemu-stable@nongnu.org Signed-off-by: Markus Armbruster Reviewed-by: Eduardo Habkost Message-Id: <1443689999-12182-10-git-send-email-armbru@redhat.com> --- hw/arm/allwinner-a10.c | 6 ++++++ hw/arm/digic.c | 6 ++++++ hw/arm/fsl-imx25.c | 6 ++++++ hw/arm/fsl-imx31.c | 6 ++++++ hw/arm/xlnx-zynqmp.c | 6 ++++++ hw/pci-host/versatile.c | 11 +++++++++++ include/hw/qdev-core.h | 13 +++++++++++++ qmp.c | 5 +++++ target-alpha/cpu.c | 7 +++++++ target-arm/cpu.c | 11 +++++++++++ target-cris/cpu.c | 7 +++++++ target-i386/cpu.c | 8 ++++++++ target-lm32/cpu.c | 7 +++++++ target-m68k/cpu.c | 7 +++++++ target-microblaze/cpu.c | 6 ++++++ target-mips/cpu.c | 7 +++++++ target-moxie/cpu.c | 7 +++++++ target-openrisc/cpu.c | 7 +++++++ target-ppc/kvm.c | 4 ++++ target-s390x/cpu.c | 7 +++++++ target-sh4/cpu.c | 7 +++++++ target-sparc/cpu.c | 7 +++++++ target-tilegx/cpu.c | 7 +++++++ target-tricore/cpu.c | 6 ++++++ target-unicore32/cpu.c | 7 +++++++ target-xtensa/cpu.c | 7 +++++++ tests/device-introspect-test.c | 27 --------------------------- 27 files changed, 185 insertions(+), 27 deletions(-) diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c index ff249af335..43dc0a12de 100644 --- a/hw/arm/allwinner-a10.c +++ b/hw/arm/allwinner-a10.c @@ -103,6 +103,12 @@ static void aw_a10_class_init(ObjectClass *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = aw_a10_realize; + + /* + * Reason: creates an ARM CPU, thus use after free(), see + * arm_cpu_class_init() + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo aw_a10_type_info = { diff --git a/hw/arm/digic.c b/hw/arm/digic.c index ec8c330602..90f8190c48 100644 --- a/hw/arm/digic.c +++ b/hw/arm/digic.c @@ -97,6 +97,12 @@ static void digic_class_init(ObjectClass *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = digic_realize; + + /* + * Reason: creates an ARM CPU, thus use after free(), see + * arm_cpu_class_init() + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo digic_type_info = { diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c index 86fde42e34..e1cadac997 100644 --- a/hw/arm/fsl-imx25.c +++ b/hw/arm/fsl-imx25.c @@ -284,6 +284,12 @@ static void fsl_imx25_class_init(ObjectClass *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = fsl_imx25_realize; + + /* + * Reason: creates an ARM CPU, thus use after free(), see + * arm_cpu_class_init() + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo fsl_imx25_type_info = { diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c index 8e1ed4811b..53d4473250 100644 --- a/hw/arm/fsl-imx31.c +++ b/hw/arm/fsl-imx31.c @@ -258,6 +258,12 @@ static void fsl_imx31_class_init(ObjectClass *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = fsl_imx31_realize; + + /* + * Reason: creates an ARM CPU, thus use after free(), see + * arm_cpu_class_init() + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo fsl_imx31_type_info = { diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index a9097f9b72..b36ca3da74 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -271,6 +271,12 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data) dc->props = xlnx_zynqmp_props; dc->realize = xlnx_zynqmp_realize; + + /* + * Reason: creates an ARM CPU, thus use after free(), see + * arm_cpu_class_init() + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo xlnx_zynqmp_type_info = { diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c index 6d23553094..7172b90958 100644 --- a/hw/pci-host/versatile.c +++ b/hw/pci-host/versatile.c @@ -500,6 +500,8 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data) dc->reset = pci_vpb_reset; dc->vmsd = &pci_vpb_vmstate; dc->props = pci_vpb_properties; + /* Reason: object_unref() hangs */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo pci_vpb_info = { @@ -521,10 +523,19 @@ static void pci_realview_init(Object *obj) s->mem_win_size[2] = 0x08000000; } +static void pci_realview_class_init(ObjectClass *class, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(class); + + /* Reason: object_unref() hangs */ + dc->cannot_destroy_with_object_finalize_yet = true; +} + static const TypeInfo pci_realview_info = { .name = "realview_pci", .parent = TYPE_VERSATILE_PCI, .instance_init = pci_realview_init, + .class_init = pci_realview_class_init, }; static void versatile_pci_register_types(void) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 038b54d94b..8057aedaa6 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -114,6 +114,19 @@ typedef struct DeviceClass { * TODO remove once we're there */ bool cannot_instantiate_with_device_add_yet; + /* + * Does this device model survive object_unref(object_new(TNAME))? + * All device models should, and this flag shouldn't exist. Some + * devices crash in object_new(), some crash or hang in + * object_unref(). Makes introspecting properties with + * qmp_device_list_properties() dangerous. Bad, because it's used + * by -device FOO,help. This flag serves to protect that code. + * It should never be set without a comment explaining why it is + * set. + * TODO remove once we're there + */ + bool cannot_destroy_with_object_finalize_yet; + bool hotpluggable; /* callbacks */ diff --git a/qmp.c b/qmp.c index 1413de439f..d9ecedef93 100644 --- a/qmp.c +++ b/qmp.c @@ -521,6 +521,11 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename, return NULL; } + if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) { + error_setg(errp, "Can't list properties of device '%s'", typename); + return NULL; + } + obj = object_new(typename); QTAILQ_FOREACH(prop, &obj->properties, node) { diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c index 421d7e5364..ff1926a5d0 100644 --- a/target-alpha/cpu.c +++ b/target-alpha/cpu.c @@ -298,6 +298,13 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data) dc->vmsd = &vmstate_alpha_cpu; #endif cc->gdb_num_core_regs = 67; + + /* + * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo alpha_cpu_type_info = { diff --git a/target-arm/cpu.c b/target-arm/cpu.c index d7b4445413..30739fc0df 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -1427,6 +1427,17 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->debug_excp_handler = arm_debug_excp_handler; cc->disas_set_info = arm_disas_set_info; + + /* + * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + * + * Once this is fixed, the devices that create ARM CPUs should be + * updated not to set cannot_destroy_with_object_finalize_yet, + * unless they still screw up something else. + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void cpu_register(const ARMCPUInfo *info) diff --git a/target-cris/cpu.c b/target-cris/cpu.c index d461e074c1..8eaf5a5a31 100644 --- a/target-cris/cpu.c +++ b/target-cris/cpu.c @@ -309,6 +309,13 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_stop_before_watchpoint = true; cc->disas_set_info = cris_disas_set_info; + + /* + * Reason: cris_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo cris_cpu_type_info = { diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c793812cc2..05d7f26bf1 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1453,6 +1453,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data) */ dc->props = host_x86_cpu_properties; + /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void host_x86_cpu_initfn(Object *obj) @@ -3190,6 +3192,12 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) #endif cc->cpu_exec_enter = x86_cpu_exec_enter; cc->cpu_exec_exit = x86_cpu_exec_exit; + + /* + * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the + * object in cpus -> dangling pointer after final object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo x86_cpu_type_info = { diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c index c2b77c6986..d0ab2786ae 100644 --- a/target-lm32/cpu.c +++ b/target-lm32/cpu.c @@ -275,6 +275,13 @@ static void lm32_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_num_core_regs = 32 + 7; cc->gdb_stop_before_watchpoint = true; cc->debug_excp_handler = lm32_debug_excp_handler; + + /* + * Reason: lm32_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void lm32_register_cpu_type(const LM32CPUInfo *info) diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c index 4f246da748..97527ef32a 100644 --- a/target-m68k/cpu.c +++ b/target-m68k/cpu.c @@ -212,6 +212,13 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data) dc->vmsd = &vmstate_m68k_cpu; cc->gdb_num_core_regs = 18; cc->gdb_core_xml_file = "cf-core.xml"; + + /* + * Reason: m68k_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void register_cpu_type(const M68kCPUInfo *info) diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c index cbd84a22f7..52959e13b4 100644 --- a/target-microblaze/cpu.c +++ b/target-microblaze/cpu.c @@ -264,6 +264,12 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_num_core_regs = 32 + 5; cc->disas_set_info = mb_disas_set_info; + + /* + * Reason: mb_cpu_initfn() calls cpu_exec_init(), which saves the + * object in cpus -> dangling pointer after final object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo mb_cpu_type_info = { diff --git a/target-mips/cpu.c b/target-mips/cpu.c index 4027d0f417..7fe1f0407f 100644 --- a/target-mips/cpu.c +++ b/target-mips/cpu.c @@ -153,6 +153,13 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->gdb_num_core_regs = 73; cc->gdb_stop_before_watchpoint = true; + + /* + * Reason: mips_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo mips_cpu_type_info = { diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c index 6b035aaab3..3af37799b7 100644 --- a/target-moxie/cpu.c +++ b/target-moxie/cpu.c @@ -114,6 +114,13 @@ static void moxie_cpu_class_init(ObjectClass *oc, void *data) cc->get_phys_page_debug = moxie_cpu_get_phys_page_debug; cc->vmsd = &vmstate_moxie_cpu; #endif + + /* + * Reason: moxie_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void moxielite_initfn(Object *obj) diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c index d97f3c03c2..cc5e2d1c5d 100644 --- a/target-openrisc/cpu.c +++ b/target-openrisc/cpu.c @@ -177,6 +177,13 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data) dc->vmsd = &vmstate_openrisc_cpu; #endif cc->gdb_num_core_regs = 32 + 3; + + /* + * Reason: openrisc_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void cpu_register(const OpenRISCCPUInfo *info) diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index f8ea783a6d..72762991dc 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -2192,6 +2192,7 @@ static void kvmppc_host_cpu_initfn(Object *obj) static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) { + DeviceClass *dc = DEVICE_CLASS(oc); PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); uint32_t vmx = kvmppc_get_vmx(); uint32_t dfp = kvmppc_get_dfp(); @@ -2218,6 +2219,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) if (icache_size != -1) { pcc->l1_icache_size = icache_size; } + + /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */ + dc->cannot_destroy_with_object_finalize_yet = true; } bool kvmppc_has_cap_epr(void) diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c index c3e21b445c..ccfaa8a919 100644 --- a/target-s390x/cpu.c +++ b/target-s390x/cpu.c @@ -353,6 +353,13 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) #endif cc->gdb_num_core_regs = S390_NUM_CORE_REGS; cc->gdb_core_xml_file = "s390x-core64.xml"; + + /* + * Reason: s390_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo s390_cpu_type_info = { diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c index 5c65ab4df5..64e4467c04 100644 --- a/target-sh4/cpu.c +++ b/target-sh4/cpu.c @@ -290,6 +290,13 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data) #endif dc->vmsd = &vmstate_sh_cpu; cc->gdb_num_core_regs = 59; + + /* + * Reason: superh_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo superh_cpu_type_info = { diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c index 9528e3afbb..82bb72ab79 100644 --- a/target-sparc/cpu.c +++ b/target-sparc/cpu.c @@ -854,6 +854,13 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) #else cc->gdb_num_core_regs = 72; #endif + + /* + * Reason: sparc_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo sparc_cpu_type_info = { diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c index 3c5481d443..c24970436d 100644 --- a/target-tilegx/cpu.c +++ b/target-tilegx/cpu.c @@ -159,6 +159,13 @@ static void tilegx_cpu_class_init(ObjectClass *oc, void *data) cc->set_pc = tilegx_cpu_set_pc; cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault; cc->gdb_num_core_regs = 0; + + /* + * Reason: tilegx_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo tilegx_cpu_type_info = { diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c index 2029ef651a..ed8b030ef5 100644 --- a/target-tricore/cpu.c +++ b/target-tricore/cpu.c @@ -170,6 +170,12 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data) cc->set_pc = tricore_cpu_set_pc; cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb; + /* + * Reason: tricore_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void cpu_register(const TriCoreCPUInfo *info) diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c index fc451a1a35..e5252ebaf8 100644 --- a/target-unicore32/cpu.c +++ b/target-unicore32/cpu.c @@ -155,6 +155,13 @@ static void uc32_cpu_class_init(ObjectClass *oc, void *data) cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug; #endif dc->vmsd = &vmstate_uc32_cpu; + + /* + * Reason: uc32_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static void uc32_register_cpu_type(const UniCore32CPUInfo *info) diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c index da8129db50..4e49bee9b5 100644 --- a/target-xtensa/cpu.c +++ b/target-xtensa/cpu.c @@ -155,6 +155,13 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data) #endif cc->debug_excp_handler = xtensa_breakpoint_handler; dc->vmsd = &vmstate_xtensa_cpu; + + /* + * Reason: xtensa_cpu_initfn() calls cpu_exec_init(), which saves + * the object in cpus -> dangling pointer after final + * object_unref(). + */ + dc->cannot_destroy_with_object_finalize_yet = true; } static const TypeInfo xtensa_cpu_type_info = { diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c index 8a27b4a702..11d5fea3e2 100644 --- a/tests/device-introspect-test.c +++ b/tests/device-introspect-test.c @@ -91,30 +91,6 @@ static void test_device_intro_abstract(void) qtest_end(); } -static bool blacklisted(const char *type) -{ - static const char *blacklist[] = { - /* hang in object_unref(): */ - "realview_pci", "versatile_pci", - /* create a CPU, thus use after free (see below): */ - "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", - }; - size_t len = strlen(type); - int i; - - if (len >= 4 && !strcmp(type + len - 4, "-cpu")) { - /* use after free: cpu_exec_init() saves CPUState in cpus */ - return true; - } - - for (i = 0; i < ARRAY_SIZE(blacklist); i++) { - if (!strcmp(blacklist[i], type)) { - return true; - } - } - return false; -} - static void test_device_intro_concrete(void) { QList *types; @@ -128,9 +104,6 @@ static void test_device_intro_concrete(void) type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)), "name"); g_assert(type); - if (blacklisted(type)) { - continue; /* FIXME broken device, skip */ - } test_one_device(type); } From 33fe96833015cf15f4c0aa5bf8d34f60526e0732 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Oct 2015 10:59:59 +0200 Subject: [PATCH 12/12] Revert "qdev: Use qdev_get_device_class() for -device ,help" This reverts commit 31bed5509dfcbdfc293154ce81086a4dbd7a80b6. The reverted commit changed qdev_device_help() to reject abstract devices and devices that have cannot_instantiate_with_device_add_yet set, to fix crash bugs like -device x86_64-cpu,help. Rejecting abstract devices makes sense: they're purely internal, and the implementation of the help feature can't cope with them. Rejecting non-pluggable devices makes less sense: even though you can't use them with -device, the help may still be useful elsewhere, for instance with -global. This is a regression: -device FOO,help used to help even for FOO that aren't pluggable. The previous two commits fixed the crash bug at a lower layer, so reverting this one is now safe. Fixes the -device FOO,help regression, except for the broken devices marked cannot_even_create_with_object_new_yet. For those, the error message is improved. Example of a device where the regression is fixed: $ qemu-system-x86_64 -device PIIX4_PM,help PIIX4_PM.command_serr_enable=bool (on/off) PIIX4_PM.multifunction=bool (on/off) PIIX4_PM.rombar=uint32 PIIX4_PM.romfile=str PIIX4_PM.addr=int32 (Slot and optional function number, example: 06.0 or 06) PIIX4_PM.memory-hotplug-support=bool PIIX4_PM.acpi-pci-hotplug-with-bridge-support=bool PIIX4_PM.s4_val=uint8 PIIX4_PM.disable_s4=uint8 PIIX4_PM.disable_s3=uint8 PIIX4_PM.smb_io_base=uint32 Example of a device where it isn't fixed: $ qemu-system-x86_64 -device host-x86_64-cpu,help Can't list properties of device 'host-x86_64-cpu' Both failed with "Parameter 'driver' expects pluggable device type" before. Cc: qemu-stable@nongnu.org Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Eduardo Habkost Message-Id: <1443689999-12182-11-git-send-email-armbru@redhat.com> --- qdev-monitor.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/qdev-monitor.c b/qdev-monitor.c index eb7aef2c81..1cadefbb13 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -237,9 +237,12 @@ int qdev_device_help(QemuOpts *opts) return 0; } - qdev_get_device_class(&driver, &local_err); - if (local_err) { - goto error; + if (!object_class_by_name(driver)) { + const char *typename = find_typename_by_alias(driver); + + if (typename) { + driver = typename; + } } prop_list = qmp_device_list_properties(driver, &local_err);