[Qemu-devel] [PATCH v3 0/7] Fix device introspection regressions

[Qemu-devel] [PATCH v3 0/7] Fix device introspection regressions

[Markus Armbruster]

QMP command device-list-properties regressed in 2.1: it can crash or
leave dangling pointers behind.

-device FOO,help regressed in 2.2: it no longer works for
non-pluggable devices.  I tried to fix that some time ago[*], but my
fix failed review.  This is my second, more comprehensive try.

PATCH 1,2 are preliminaries, PATCH 3 adds tests to demonstrate the
bugs, PATCH 4-6 fix them to a degree (see PATCH 5 for limitations),
and PATCH 7 cleans up.

[*] [PATCH] qdev: Make -device FOO,help help again when FOO is not
pluggable
https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03459.html
Message-Id: <1426527232-15044-1-git-send-email-armbru@redhat.com>

v3:
* PATCH 6: Mark "tilegx-cpu" [Eduardo] and new "spapr-rng", clean up
  whitespace.

v2:
* PATCH 1: New, made from old PATCH 7 and relevant Makefile parts of
  old PATCH 3, with a much improved commit message [Andreas]
* PATCH 3: Fix hmp() [Eric]
* PATCH 4: Tweak commit message and comments [Eric]
* PATCH 6: Mark only the CPUs that are actually broken [Eduardo]

Markus Armbruster (7):
  tests: Fix how qom-test is run
  libqtest: Clean up unused QTestState member sigact_old
  libqtest: New hmp() & friends
  device-introspect-test: New, covering device introspection
  qmp: Fix device-list-properties not to crash for abstract device
  qdev: Protect device-list-properties against broken devices
  Revert "qdev: Use qdev_get_device_class() for -device <type>,help"

 hw/arm/allwinner-a10.c         |   2 +
 hw/arm/digic.c                 |   2 +
 hw/arm/fsl-imx25.c             |   2 +
 hw/arm/fsl-imx31.c             |   2 +
 hw/arm/xlnx-zynqmp.c           |   2 +
 hw/pci-host/versatile.c        |  11 ++++
 hw/pcmcia/pxa2xx.c             |   9 ++++
 hw/ppc/spapr_rng.c             |   5 ++
 hw/s390x/event-facility.c      |   3 ++
 hw/s390x/sclp.c                |   3 ++
 include/hw/qdev-core.h         |  13 +++++
 qdev-monitor.c                 |   9 ++--
 qmp.c                          |  11 ++++
 target-alpha/cpu.c             |   7 +++
 target-arm/cpu.c               |   7 +++
 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/Makefile                 |  20 ++++---
 tests/device-introspect-test.c | 117 +++++++++++++++++++++++++++++++++++++++++
 tests/drive_del-test.c         |  22 +++-----
 tests/ide-test.c               |   8 +--
 tests/libqtest.c               |  38 ++++++++++++-
 tests/libqtest.h               |  33 ++++++++++++
 37 files changed, 400 insertions(+), 34 deletions(-)
 create mode 100644 tests/device-introspect-test.c

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/7] tests: Fix how qom-test is run

[Markus Armbruster]

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 <armbru@redhat.com>
---
 tests/Makefile | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 4063639..9380e14 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
@@ -216,10 +218,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 \
@@ -446,8 +445,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)
@@ -485,7 +487,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`; \
-- 
2.4.3

[Qemu-devel] [PATCH v3 2/7] libqtest: Clean up unused QTestState member sigact_old

[Markus Armbruster]

Unused since commit d766825.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index e5188e0..8dede56 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;
-- 
2.4.3

[Qemu-devel] [PATCH v3 3/7] libqtest: New hmp() & friends

[Markus Armbruster]

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 <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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 8951f6f..3390946 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 5594738..6173dfa 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 8dede56..2a396ba 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 ec42031..b270f7b 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -120,6 +120,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.
+ */
+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.
+ */
+char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
+
+/**
  * qtest_get_irq:
  * @s: #QTestState instance to operate on.
  * @num: Interrupt to observe.
@@ -499,6 +522,16 @@ static inline void qmp_eventwait(const char *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.
+ */
+char *hmp(const char *fmt, ...);
+
+/**
  * get_irq:
  * @num: Interrupt to observe.
  *
-- 
2.4.3

[Qemu-devel] [PATCH v3 4/7] device-introspect-test: New, covering device introspection

[Markus Armbruster]

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: several devices crash or
hang, and all CPUs leave a dangling pointer behind.  The test skips
testing the broken parts.  The next commits will fix them, and drop
the skipping.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/Makefile                 |   8 ++-
 tests/device-introspect-test.c | 153 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+), 3 deletions(-)
 create mode 100644 tests/device-introspect-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 9380e14..2bf7ba1 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)
@@ -381,6 +382,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
@@ -488,7 +490,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,)
@@ -499,7 +501,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 0000000..44da30e
--- /dev/null
+++ b/tests/device-introspect-test.c
@@ -0,0 +1,153 @@
+/*
+ * Device introspection test cases
+ *
+ * Copyright (c) 2015 Red Hat Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>,
+ *
+ * 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 <glib.h>
+#include <stdarg.h>
+#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;
+
+    /*
+     * 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);
+}
+
+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[] = {
+        /* crash in object_unref(): */
+        "pxa2xx-pcmcia",
+        /* hang in object_unref(): */
+        "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp",
+        /* 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();
+}
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 4/7] device-introspect-test: New, covering device introspection

[Thomas Huth]

On 24/09/15 20:57, Markus Armbruster wrote:
> 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: several devices crash or
> hang, and all CPUs leave a dangling pointer behind.  The test skips
> testing the broken parts.  The next commits will fix them, and drop
> the skipping.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/Makefile                 |   8 ++-
>  tests/device-introspect-test.c | 153 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 158 insertions(+), 3 deletions(-)
>  create mode 100644 tests/device-introspect-test.c

 Hi Markus,

just a quick note: When I run the tester directly, it aborts:

$ tests/boot-order-test
**
ERROR:/home/thuth/devel/qemu/tests/libqtest.c:517:qtest_get_arch:
assertion failed: (qemu != NULL)
Aborted (core dumped)

... that's a little bit ugly, maybe you could print the help text instead?

 Thomas

Re: [Qemu-devel] [PATCH v3 4/7] device-introspect-test: New, covering device introspection

[Andreas Färber]

Am 25.09.2015 um 12:17 schrieb Thomas Huth:
> On 24/09/15 20:57, Markus Armbruster wrote:
>> 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: several devices crash or
>> hang, and all CPUs leave a dangling pointer behind.  The test skips
>> testing the broken parts.  The next commits will fix them, and drop
>> the skipping.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  tests/Makefile                 |   8 ++-
>>  tests/device-introspect-test.c | 153 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 158 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/device-introspect-test.c
> 
>  Hi Markus,
> 
> just a quick note: When I run the tester directly, it aborts:
> 
> $ tests/boot-order-test
> **
> ERROR:/home/thuth/devel/qemu/tests/libqtest.c:517:qtest_get_arch:
> assertion failed: (qemu != NULL)
> Aborted (core dumped)
> 
> ... that's a little bit ugly, maybe you could print the help text instead?

That's got nothing to do with his test, all QTests require environment
variables set in the Makefile. Feel free to send a patch for qtest.c.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 4/7] device-introspect-test: New, covering device introspection

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 25.09.2015 um 12:17 schrieb Thomas Huth:
>> On 24/09/15 20:57, Markus Armbruster wrote:
>>> 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: several devices crash or
>>> hang, and all CPUs leave a dangling pointer behind.  The test skips
>>> testing the broken parts.  The next commits will fix them, and drop
>>> the skipping.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  tests/Makefile                 |   8 ++-
>>>  tests/device-introspect-test.c | 153
>>> +++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 158 insertions(+), 3 deletions(-)
>>>  create mode 100644 tests/device-introspect-test.c
>> 
>>  Hi Markus,
>> 
>> just a quick note: When I run the tester directly, it aborts:
>> 
>> $ tests/boot-order-test
>> **
>> ERROR:/home/thuth/devel/qemu/tests/libqtest.c:517:qtest_get_arch:
>> assertion failed: (qemu != NULL)
>> Aborted (core dumped)
>> 
>> ... that's a little bit ugly, maybe you could print the help text instead?
>
> That's got nothing to do with his test, all QTests require environment
> variables set in the Makefile. Feel free to send a patch for qtest.c.

Here's an example run I fished out of my bash history:

$ QTEST_QEMU_BINARY="ppc64-softmmu/qemu-system-ppc64" QTEST_QEMU_IMG=qemu-img MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k --verbose -m=quick tests/device-help-test

Or with valgrind spliced in:

$ QTEST_QEMU_BINARY="valgrind --vgdb-error=1 --log-file=vg.log ppc64-softmmu/qemu-system-ppc64" QTEST_QEMU_IMG=qemu-img MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k --verbose -m=quick tests/device-help-test

[Qemu-devel] [PATCH v3 5/7] qmp: Fix device-list-properties not to crash for abstract device

[Markus Armbruster]

Broken in commit f4eb32b "qmp: show QOM properties in
device-list-properties", v2.1.

Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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 057a7cb..1413de4 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 44da30e..6c7366f 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;
 
-    /*
-     * 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);
-- 
2.4.3

[Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

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_even_create_with_object_new_yet
to mark them:

* Crash during init (didn't debug, so I can't say why): "spapr-rng"

* Crash or hang during cleanup (didn't debug, so I can't say why):
  "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
  "s390-sclp-event-facility", "sclp"

* Dangling pointers: 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 or hangs"
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" <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Antony Pavlov <antonynpavlov@gmail.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Li Guang <lig.fnst@cn.fujitsu.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: qemu-ppc@nongnu.org
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/allwinner-a10.c         |  2 ++
 hw/arm/digic.c                 |  2 ++
 hw/arm/fsl-imx25.c             |  2 ++
 hw/arm/fsl-imx31.c             |  2 ++
 hw/arm/xlnx-zynqmp.c           |  2 ++
 hw/pci-host/versatile.c        | 11 +++++++++++
 hw/pcmcia/pxa2xx.c             |  9 +++++++++
 hw/ppc/spapr_rng.c             |  5 +++++
 hw/s390x/event-facility.c      |  3 +++
 hw/s390x/sclp.c                |  3 +++
 include/hw/qdev-core.h         | 13 +++++++++++++
 qmp.c                          |  5 +++++
 target-alpha/cpu.c             |  7 +++++++
 target-arm/cpu.c               |  7 +++++++
 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 | 29 -----------------------------
 31 files changed, 181 insertions(+), 29 deletions(-)

diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index ff249af..7692090 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -103,6 +103,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = aw_a10_realize;
+    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo aw_a10_type_info = {
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index ec8c330..3decef4 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -97,6 +97,8 @@ static void digic_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = digic_realize;
+    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo digic_type_info = {
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 86fde42..13c06b2 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -284,6 +284,8 @@ static void fsl_imx25_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = fsl_imx25_realize;
+    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo fsl_imx25_type_info = {
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 8e1ed48..7cb8fd4 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -258,6 +258,8 @@ static void fsl_imx31_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = fsl_imx31_realize;
+    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo fsl_imx31_type_info = {
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index a9097f9..1b209dc 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -271,6 +271,8 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
 
     dc->props = xlnx_zynqmp_props;
     dc->realize = xlnx_zynqmp_realize;
+    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo xlnx_zynqmp_type_info = {
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 6d23553..f28a115 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_even_create_with_object_new_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_even_create_with_object_new_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/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index a7e1877..c050c41 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -249,11 +249,20 @@ void pxa2xx_pcmcia_set_irq_cb(void *opaque, qemu_irq irq, qemu_irq cd_irq)
     s->cd_irq = cd_irq;
 }
 
+static void pxa2xx_pcmcia_class_init(ObjectClass *class, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(class);
+
+    /* Reason: object_unref() crashes */
+    dc->cannot_even_create_with_object_new_yet = true;
+}
+
 static const TypeInfo pxa2xx_pcmcia_type_info = {
     .name = TYPE_PXA2XX_PCMCIA,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(PXA2xxPCMCIAState),
     .instance_init = pxa2xx_pcmcia_initfn,
+    .class_init = pxa2xx_pcmcia_class_init,
 };
 
 static void pxa2xx_pcmcia_register_types(void)
diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
index ed43d5e..e1b115d 100644
--- a/hw/ppc/spapr_rng.c
+++ b/hw/ppc/spapr_rng.c
@@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
     dc->realize = spapr_rng_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->props = spapr_rng_properties;
+
+    /*
+     * Reason: crashes device-introspect-test for unknown reason.
+     */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo spapr_rng_info = {
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index ef2a051..8fa361d 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -381,6 +381,9 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     k->command_handler = command_handler;
     k->event_pending = event_pending;
+
+    /* Reason: object_unref() hangs */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo sclp_event_facility_info = {
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index a061b49..fdc5a98 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -563,6 +563,9 @@ static void sclp_class_init(ObjectClass *oc, void *data)
     sc->read_cpu_info = sclp_read_cpu_info;
     sc->execute = sclp_execute;
     sc->service_interrupt = service_interrupt;
+
+    /* Reason: object_unref() hangs */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static TypeInfo sclp_info = {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 038b54d..bc30cca 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_even_create_with_object_new_yet;
+
     bool hotpluggable;
 
     /* callbacks */
diff --git a/qmp.c b/qmp.c
index 1413de4..f8db2d7 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_even_create_with_object_new_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 421d7e5..54854f0 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo alpha_cpu_type_info = {
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d7b4445..f595bb5 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1427,6 +1427,13 @@ 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().
+     */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static void cpu_register(const ARMCPUInfo *info)
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index d461e07..2336e09 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo cris_cpu_type_info = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7c52714..09807c5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1449,6 +1449,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_even_create_with_object_new_yet = true;
 }
 
 static void host_x86_cpu_initfn(Object *obj)
@@ -3175,6 +3177,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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo x86_cpu_type_info = {
diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
index c2b77c6..b32825c 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_even_create_with_object_new_yet = true;
 }
 
 static void lm32_register_cpu_type(const LM32CPUInfo *info)
diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
index 4f246da..7f2036b 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_even_create_with_object_new_yet = true;
 }
 
 static void register_cpu_type(const M68kCPUInfo *info)
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index 9ac509a..6960c7e 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo mb_cpu_type_info = {
diff --git a/target-mips/cpu.c b/target-mips/cpu.c
index 4027d0f..56c745e 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo mips_cpu_type_info = {
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 6b035aa..a328fd1 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_even_create_with_object_new_yet = true;
 }
 
 static void moxielite_initfn(Object *obj)
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index d97f3c0..7c8bab8 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_even_create_with_object_new_yet = true;
 }
 
 static void cpu_register(const OpenRISCCPUInfo *info)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index e641680..9fb78f0 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2193,6 +2193,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();
@@ -2219,6 +2220,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_even_create_with_object_new_yet = true;
 }
 
 bool kvmppc_has_cap_epr(void)
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c3e21b4..1ecd203 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo s390_cpu_type_info = {
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index 5c65ab4..b26b685 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo superh_cpu_type_info = {
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 9528e3a..bda312f 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo sparc_cpu_type_info = {
diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
index 78b73e4..bee9f93 100644
--- a/target-tilegx/cpu.c
+++ b/target-tilegx/cpu.c
@@ -154,6 +154,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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo tilegx_cpu_type_info = {
diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
index 2029ef6..6f906cf 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_even_create_with_object_new_yet = true;
 }
 
 static void cpu_register(const TriCoreCPUInfo *info)
diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
index fc451a1..89b1fec 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_even_create_with_object_new_yet = true;
 }
 
 static void uc32_register_cpu_type(const UniCore32CPUInfo *info)
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index da8129d..2ea43cb 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_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo xtensa_cpu_type_info = {
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 6c7366f..3c1b361 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -84,32 +84,6 @@ static void test_device_intro_abstract(void)
     qtest_end();
 }
 
-static bool blacklisted(const char *type)
-{
-    static const char *blacklist[] = {
-        /* crash in object_unref(): */
-        "pxa2xx-pcmcia",
-        /* hang in object_unref(): */
-        "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp",
-        /* 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;
@@ -123,9 +97,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);
     }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Eduardo Habkost]

On Thu, Sep 24, 2015 at 08:57:21PM +0200, Markus Armbruster wrote:
[...]
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index ff249af..7692090 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -103,6 +103,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      dc->realize = aw_a10_realize;
> +    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
> +    dc->cannot_even_create_with_object_new_yet = true;

The comments at aw_a10_class_init(), digic_class_init(),
fsl_imx25_class_init(), fsl_imx31_class_init(), and
xlnx_zynqmp_class_init() are now outdated, as cpu_class_init() doesn't
set cannot_even_create_with_object_new_yet anymore.

We could do this:
* Update the comments to "Reason: creates an ARM CPU, thus use after
  free(), see arm_cpu_class_init()"
* Add a note at arm_cpu_class_init() saying that we can probably
  unset cannot_even_create_with_object_new_yet in those functions
  once we fix TYPE_ARM_CPU

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Sep 24, 2015 at 08:57:21PM +0200, Markus Armbruster wrote:
> [...]
>> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
>> index ff249af..7692090 100644
>> --- a/hw/arm/allwinner-a10.c
>> +++ b/hw/arm/allwinner-a10.c
>> @@ -103,6 +103,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>  
>>      dc->realize = aw_a10_realize;
>> +    /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
>> +    dc->cannot_even_create_with_object_new_yet = true;
>
> The comments at aw_a10_class_init(), digic_class_init(),
> fsl_imx25_class_init(), fsl_imx31_class_init(), and
> xlnx_zynqmp_class_init() are now outdated, as cpu_class_init() doesn't
> set cannot_even_create_with_object_new_yet anymore.
>
> We could do this:
> * Update the comments to "Reason: creates an ARM CPU, thus use after
>   free(), see arm_cpu_class_init()"

Yes.

> * Add a note at arm_cpu_class_init() saying that we can probably
>   unset cannot_even_create_with_object_new_yet in those functions
>   once we fix TYPE_ARM_CPU

Okay.

Thanks!

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Thomas Huth]

On 24/09/15 20:57, Markus Armbruster wrote:
> 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_even_create_with_object_new_yet
> to mark them:
> 
> * Crash during init (didn't debug, so I can't say why): "spapr-rng"
> 
> * Crash or hang during cleanup (didn't debug, so I can't say why):
>   "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>   "s390-sclp-event-facility", "sclp"
> 
> * Dangling pointers: 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 or hangs"
> 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" <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Li Guang <lig.fnst@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
...
>  static void pxa2xx_pcmcia_register_types(void)
> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
> index ed43d5e..e1b115d 100644
> --- a/hw/ppc/spapr_rng.c
> +++ b/hw/ppc/spapr_rng.c
> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>      dc->realize = spapr_rng_realize;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      dc->props = spapr_rng_properties;
> +
> +    /*
> +     * Reason: crashes device-introspect-test for unknown reason.
> +     */
> +    dc->cannot_even_create_with_object_new_yet = true;
>  }

Please don't do that! That breaks the help output from
"-device spapr-rng,?" which should help the user to see how to use this
device!

I tried to debug why this device breaks the test, but the test
environment is giving me a hard time ... how do you best hook a gdb into
that framework, so you can trace such problems?
Anyway, with some trial and error, I found out that it seems like the

  object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)

in spapr_rng_instance_init() is causing the problems. Could it be that
object_resolve_path_type is not working with the test environment?

 Thomas

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 24/09/15 20:57, Markus Armbruster wrote:
>> 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_even_create_with_object_new_yet
>> to mark them:
>> 
>> * Crash during init (didn't debug, so I can't say why): "spapr-rng"
>> 
>> * Crash or hang during cleanup (didn't debug, so I can't say why):
>>   "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>   "s390-sclp-event-facility", "sclp"
>> 
>> * Dangling pointers: 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 or hangs"
>> 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" <afaerber@suse.de>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>> Cc: Antony Pavlov <antonynpavlov@gmail.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Li Guang <lig.fnst@cn.fujitsu.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: qemu-ppc@nongnu.org
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
> ...
>>  static void pxa2xx_pcmcia_register_types(void)
>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>> index ed43d5e..e1b115d 100644
>> --- a/hw/ppc/spapr_rng.c
>> +++ b/hw/ppc/spapr_rng.c
>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>>      dc->realize = spapr_rng_realize;
>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>      dc->props = spapr_rng_properties;
>> +
>> +    /*
>> +     * Reason: crashes device-introspect-test for unknown reason.
>> +     */
>> +    dc->cannot_even_create_with_object_new_yet = true;
>>  }
>
> Please don't do that! That breaks the help output from
> "-device spapr-rng,?" which should help the user to see how to use this
> device!

Well, device-introspection-test makes qemu crash, with the backtrace
pointing squarely to this device.  Stands to reason that device
introspection could crash in normal usage, too.  Until the crash is
debugged, we better disable introspection of this device.

I quite agree that disabling introspection hurts users.  Just not as
much as crashes :)

> I tried to debug why this device breaks the test, but the test
> environment is giving me a hard time ... how do you best hook a gdb into
> that framework, so you can trace such problems?
> Anyway, with some trial and error, I found out that it seems like the
>
>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>
> in spapr_rng_instance_init() is causing the problems. Could it be that
> object_resolve_path_type is not working with the test environment?

I tried to figure out why this device breaks under this test, but
couldn't, so I posted with the "for unknown reason" comment.

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Thomas Huth]

On 25/09/15 16:17, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 24/09/15 20:57, Markus Armbruster wrote:
>>> 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_even_create_with_object_new_yet
>>> to mark them:
...
>>>  static void pxa2xx_pcmcia_register_types(void)
>>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>>> index ed43d5e..e1b115d 100644
>>> --- a/hw/ppc/spapr_rng.c
>>> +++ b/hw/ppc/spapr_rng.c
>>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>>>      dc->realize = spapr_rng_realize;
>>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>      dc->props = spapr_rng_properties;
>>> +
>>> +    /*
>>> +     * Reason: crashes device-introspect-test for unknown reason.
>>> +     */
>>> +    dc->cannot_even_create_with_object_new_yet = true;
>>>  }
>>
>> Please don't do that! That breaks the help output from
>> "-device spapr-rng,?" which should help the user to see how to use this
>> device!
> 
> Well, device-introspection-test makes qemu crash, with the backtrace
> pointing squarely to this device.  Stands to reason that device
> introspection could crash in normal usage, too.  Until the crash is
> debugged, we better disable introspection of this device.
> 
> I quite agree that disabling introspection hurts users.  Just not as
> much as crashes :)
> 
>> I tried to debug why this device breaks the test, but the test
>> environment is giving me a hard time ... how do you best hook a gdb into
>> that framework, so you can trace such problems?
>> Anyway, with some trial and error, I found out that it seems like the
>>
>>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>>
>> in spapr_rng_instance_init() is causing the problems. Could it be that
>> object_resolve_path_type is not working with the test environment?
> 
> I tried to figure out why this device breaks under this test, but
> couldn't, so I posted with the "for unknown reason" comment.

I've debugged this now for a while (thanks for the tip with
MALLOC_PERTURB, by the way!) and it seems to me that the problem is in
the macio object than in spapr-rng - the latter is just the victim of
some memory corruption caused by the first one: The
object_resolve_path_type() crashes while trying to go through the macio
object.

So could you please add the "dc->cannot_even_create_with_object_new_yet
= true;" to macio_class_init() instead? ... that seems to fix the crash
for me, too, and is likely the better place.

Or maybe we could get this also fixed? The problem could be the
memory_region_init(&s->bar, NULL, "macio", 0x80000) in
macio_instance_init() ... is this ok here? Or does this rather have to
go to the realize() function instead?

 Thomas

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 25/09/15 16:17, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 24/09/15 20:57, Markus Armbruster wrote:
>>>> 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_even_create_with_object_new_yet
>>>> to mark them:
> ...
>>>>  static void pxa2xx_pcmcia_register_types(void)
>>>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>>>> index ed43d5e..e1b115d 100644
>>>> --- a/hw/ppc/spapr_rng.c
>>>> +++ b/hw/ppc/spapr_rng.c
>>>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>>>>      dc->realize = spapr_rng_realize;
>>>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>>      dc->props = spapr_rng_properties;
>>>> +
>>>> +    /*
>>>> +     * Reason: crashes device-introspect-test for unknown reason.
>>>> +     */
>>>> +    dc->cannot_even_create_with_object_new_yet = true;
>>>>  }
>>>
>>> Please don't do that! That breaks the help output from
>>> "-device spapr-rng,?" which should help the user to see how to use this
>>> device!
>> 
>> Well, device-introspection-test makes qemu crash, with the backtrace
>> pointing squarely to this device.  Stands to reason that device
>> introspection could crash in normal usage, too.  Until the crash is
>> debugged, we better disable introspection of this device.
>> 
>> I quite agree that disabling introspection hurts users.  Just not as
>> much as crashes :)
>> 
>>> I tried to debug why this device breaks the test, but the test
>>> environment is giving me a hard time ... how do you best hook a gdb into
>>> that framework, so you can trace such problems?
>>> Anyway, with some trial and error, I found out that it seems like the
>>>
>>>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>>>
>>> in spapr_rng_instance_init() is causing the problems. Could it be that
>>> object_resolve_path_type is not working with the test environment?
>> 
>> I tried to figure out why this device breaks under this test, but
>> couldn't, so I posted with the "for unknown reason" comment.
>
> I've debugged this now for a while (thanks for the tip with
> MALLOC_PERTURB, by the way!) and it seems to me that the problem is in
> the macio object than in spapr-rng - the latter is just the victim of
> some memory corruption caused by the first one: The
> object_resolve_path_type() crashes while trying to go through the macio
> object.
>
> So could you please add the "dc->cannot_even_create_with_object_new_yet
> = true;" to macio_class_init() instead? ... that seems to fix the crash
> for me, too, and is likely the better place.

Hmm.

For most of the devices my patch marks, we have a pretty good idea on
what's wrong with them.  spapr-rng is among the exceptions.  You believe
it's actually "the macio object".  Which one?  "macio" is abstract...

You report introspecting "spapr-rng" crashes "while trying to go through
the macio object".  I wonder how omitting introspection of macio objects
(that's what marking them does to this test) could affect the object
we're going through when we crash.

> Or maybe we could get this also fixed? The problem could be the
> memory_region_init(&s->bar, NULL, "macio", 0x80000) in
> macio_instance_init() ... is this ok here? Or does this rather have to
> go to the realize() function instead?

Hmm, does creating and destroying a macio object leave the memory region
behind?

Paolo, is calling memory_region_init() in an instance_init() method
okay?

If yes, where should they be destroyed, and how?

If no, we should search for the erroneous pattern and mark the
offenders.

Some more evidence for macio's culpability: valgrind lets me happily
introspect spapr-rng as often as I want, but once I introspected
macio-newworld, further introspection of spapr-rng throws "Invalid read"
errors.

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Andreas Färber]

Am 28.09.2015 um 10:11 schrieb Markus Armbruster:
> Hmm, does creating and destroying a macio object leave the memory region
> behind?
> 
> Paolo, is calling memory_region_init() in an instance_init() method
> okay?
> 
> If yes, where should they be destroyed, and how?

The counterpart to .instance_init is .instance_finalize aka uninit.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Paolo Bonzini]

On 28/09/2015 10:15, Andreas Färber wrote:
> Am 28.09.2015 um 10:11 schrieb Markus Armbruster:
>> Hmm, does creating and destroying a macio object leave the memory region
>> behind?
>>
>> Paolo, is calling memory_region_init() in an instance_init() method
>> okay?
>>
>> If yes, where should they be destroyed, and how?
> 
> The counterpart to .instance_init is .instance_finalize aka uninit.

memory_region_init adds the region as a child of the object, so it need
not be destroyed anywhere.

Lo and behold, it's even documented in docs/memory.txt: "Destruction of
a memory region happens automatically when the owner object dies".

Paolo

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Paolo Bonzini]

On 28/09/2015 10:11, Markus Armbruster wrote:
> 
> For most of the devices my patch marks, we have a pretty good idea on
> what's wrong with them.  spapr-rng is among the exceptions.  You believe
> it's actually "the macio object".  Which one?  "macio" is abstract...
> 
> You report introspecting "spapr-rng" crashes "while trying to go through
> the macio object".  I wonder how omitting introspection of macio objects
> (that's what marking them does to this test) could affect the object
> we're going through when we crash.
> 
>> > Or maybe we could get this also fixed? The problem could be the
>> > memory_region_init(&s->bar, NULL, "macio", 0x80000) in
>> > macio_instance_init() ... is this ok here? Or does this rather have to
>> > go to the realize() function instead?
> Hmm, does creating and destroying a macio object leave the memory region
> behind?
> 
> Paolo, is calling memory_region_init() in an instance_init() method
> okay?

Yes, but it has to have a non-NULL owner.

The reason why this particular call has a NULL owner is that the
(non-qdevified) DBDMA_init object inside it is also passing a NULL
owner.  DBDMA_init object is also doing a few more non-idempotent things
such as a malloc, a vmstate_register and a qemu_register_reset.

The full solution would be to qdev-ify DBDMA.  A simpler but also valid
solution would be to move DBDMA_init to macio_common_realize, in
addition to setting the owner of s->bar.

Paolo

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/09/2015 10:11, Markus Armbruster wrote:
>> 
>> For most of the devices my patch marks, we have a pretty good idea on
>> what's wrong with them.  spapr-rng is among the exceptions.  You believe
>> it's actually "the macio object".  Which one?  "macio" is abstract...
>> 
>> You report introspecting "spapr-rng" crashes "while trying to go through
>> the macio object".  I wonder how omitting introspection of macio objects
>> (that's what marking them does to this test) could affect the object
>> we're going through when we crash.
>> 
>>> > Or maybe we could get this also fixed? The problem could be the
>>> > memory_region_init(&s->bar, NULL, "macio", 0x80000) in
>>> > macio_instance_init() ... is this ok here? Or does this rather have to
>>> > go to the realize() function instead?
>> Hmm, does creating and destroying a macio object leave the memory region
>> behind?
>> 
>> Paolo, is calling memory_region_init() in an instance_init() method
>> okay?
>
> Yes, but it has to have a non-NULL owner.
>
> The reason why this particular call has a NULL owner is that the
> (non-qdevified) DBDMA_init object inside it is also passing a NULL
> owner.  DBDMA_init object is also doing a few more non-idempotent things
> such as a malloc, a vmstate_register and a qemu_register_reset.
>
> The full solution would be to qdev-ify DBDMA.  A simpler but also valid
> solution would be to move DBDMA_init to macio_common_realize, in
> addition to setting the owner of s->bar.

All right, we're now sure macio is the broken part that needs to be
marked in this series.

I searched for memory_region_init() calls passing a null owner:

* exec.c memory_map_init()
  hw/i386/pc_piix.c pc_init1()
  hw/i386/pc_q35.c pc_q35_init()
  hw/mips/mips_jazz.c mips_jazz_init()
  hw/mips/mips_r4k.c mips_r4k_init()
  hw/ppc/ppc405_boards.c ref405ep_init()

  Not device model code, doesn't affect my series.

* hw/core/platform-bus.c platform_bus_realize()
  hw/display/vga-pci.c pci_std_vga_realize()

  Realize method, doesn't affect my series.

* hw/misc/omap_gpmc.c omap_gpmc_cs_map()

  Is this sane?  I guess the object_unparent() in omap_gpmc_cs_unmap()
  could make it sane.

* hw/ppc/ppc4xx_devs.c sdram_set_bcr()

  Is this sane?  I guess the object_unparent() there could make it sane.

* hw/misc/macio/macio.c macio_instance_init()

  I understand this isn't currently sane, and I'll mark type "macio"
  (and thus its sub-types) in my v4.

* hw/misc/macio/macio.c macio_escc_legacy_setup() 

  I don't care due to the previous item.

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Paolo Bonzini]

On 28/09/2015 16:17, Markus Armbruster wrote:
>> The reason why this particular call has a NULL owner is that the
>> (non-qdevified) DBDMA_init object inside it is also passing a NULL
>> owner.  DBDMA_init object is also doing a few more non-idempotent things
>> such as a malloc, a vmstate_register and a qemu_register_reset.

I missed a step here: "... thus macio really
cannot_be_created_even_by_object_new_yet".

> * hw/misc/omap_gpmc.c omap_gpmc_cs_map()
> 
>   Is this sane?  I guess the object_unparent() in omap_gpmc_cs_unmap()
>   could make it sane.
> 
> * hw/ppc/ppc4xx_devs.c sdram_set_bcr()
> 
>   Is this sane?  I guess the object_unparent() there could make it sane.

Yes and yes.

There are many more similar places that you missed, which call
memory_region_init_alias(), memory_region_init_io() and
memory_region_init_ram().  Those are the same.

Paolo

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Thomas Huth]

On 28/09/15 10:11, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 25/09/15 16:17, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> On 24/09/15 20:57, Markus Armbruster wrote:
>>>>> 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_even_create_with_object_new_yet
>>>>> to mark them:
>> ...
>>>>>  static void pxa2xx_pcmcia_register_types(void)
>>>>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>>>>> index ed43d5e..e1b115d 100644
>>>>> --- a/hw/ppc/spapr_rng.c
>>>>> +++ b/hw/ppc/spapr_rng.c
>>>>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>>>>>      dc->realize = spapr_rng_realize;
>>>>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>>>      dc->props = spapr_rng_properties;
>>>>> +
>>>>> +    /*
>>>>> +     * Reason: crashes device-introspect-test for unknown reason.
>>>>> +     */
>>>>> +    dc->cannot_even_create_with_object_new_yet = true;
>>>>>  }
>>>>
>>>> Please don't do that! That breaks the help output from
>>>> "-device spapr-rng,?" which should help the user to see how to use this
>>>> device!
>>>
>>> Well, device-introspection-test makes qemu crash, with the backtrace
>>> pointing squarely to this device.  Stands to reason that device
>>> introspection could crash in normal usage, too.  Until the crash is
>>> debugged, we better disable introspection of this device.
>>>
>>> I quite agree that disabling introspection hurts users.  Just not as
>>> much as crashes :)
>>>
>>>> I tried to debug why this device breaks the test, but the test
>>>> environment is giving me a hard time ... how do you best hook a gdb into
>>>> that framework, so you can trace such problems?
>>>> Anyway, with some trial and error, I found out that it seems like the
>>>>
>>>>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>>>>
>>>> in spapr_rng_instance_init() is causing the problems. Could it be that
>>>> object_resolve_path_type is not working with the test environment?
>>>
>>> I tried to figure out why this device breaks under this test, but
>>> couldn't, so I posted with the "for unknown reason" comment.
>>
>> I've debugged this now for a while (thanks for the tip with
>> MALLOC_PERTURB, by the way!) and it seems to me that the problem is in
>> the macio object than in spapr-rng - the latter is just the victim of
>> some memory corruption caused by the first one: The
>> object_resolve_path_type() crashes while trying to go through the macio
>> object.
>>
>> So could you please add the "dc->cannot_even_create_with_object_new_yet
>> = true;" to macio_class_init() instead? ... that seems to fix the crash
>> for me, too, and is likely the better place.
> 
> Hmm.
> 
> For most of the devices my patch marks, we have a pretty good idea on
> what's wrong with them.  spapr-rng is among the exceptions.  You believe
> it's actually "the macio object".  Which one?  "macio" is abstract...
> 
> You report introspecting "spapr-rng" crashes "while trying to go through
> the macio object".  I wonder how omitting introspection of macio objects
> (that's what marking them does to this test) could affect the object
> we're going through when we crash.

I have to correct myself: It's not going through the macio object, the
problem is actually the "macio[0]" property that is created during
memory_region_init() with object_property_add_child() ... the property
points to a free()d object when the crash happens.

>> Or maybe we could get this also fixed? The problem could be the
>> memory_region_init(&s->bar, NULL, "macio", 0x80000) in
>> macio_instance_init() ... is this ok here? Or does this rather have to
>> go to the realize() function instead?
> 
> Hmm, does creating and destroying a macio object leave the memory region
> behind?
> 
> Paolo, is calling memory_region_init() in an instance_init() method
> okay?

As Paolo mentioned, we likely need to pass an "owner" to
memory_region_init() or the macio memory region will get attached to
"/unattached" instead - and then leave a dangling link property behind
when the original macio object got destroyed.

By the way, there are some more spots like this in the code, e.g. in
pxa2xx_fir_instance_init() in hw/arm/pxa2xx.c ...

 Thomas

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Peter Maydell]

On 28 September 2015 at 10:17, Thomas Huth <thuth@redhat.com> wrote:
> By the way, there are some more spots like this in the code, e.g. in
> pxa2xx_fir_instance_init() in hw/arm/pxa2xx.c ...

That's an oversight from when it was converted to qdev, I think,
and could be fixed.

-- PMM

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 28/09/15 10:11, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 25/09/15 16:17, Markus Armbruster wrote:
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>>> On 24/09/15 20:57, Markus Armbruster wrote:
>>>>>> 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_even_create_with_object_new_yet
>>>>>> to mark them:
>>> ...
>>>>>>  static void pxa2xx_pcmcia_register_types(void)
>>>>>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>>>>>> index ed43d5e..e1b115d 100644
>>>>>> --- a/hw/ppc/spapr_rng.c
>>>>>> +++ b/hw/ppc/spapr_rng.c
>>>>>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>>>>>>      dc->realize = spapr_rng_realize;
>>>>>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>>>>      dc->props = spapr_rng_properties;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Reason: crashes device-introspect-test for unknown reason.
>>>>>> +     */
>>>>>> +    dc->cannot_even_create_with_object_new_yet = true;
>>>>>>  }
>>>>>
>>>>> Please don't do that! That breaks the help output from
>>>>> "-device spapr-rng,?" which should help the user to see how to use this
>>>>> device!
>>>>
>>>> Well, device-introspection-test makes qemu crash, with the backtrace
>>>> pointing squarely to this device.  Stands to reason that device
>>>> introspection could crash in normal usage, too.  Until the crash is
>>>> debugged, we better disable introspection of this device.
>>>>
>>>> I quite agree that disabling introspection hurts users.  Just not as
>>>> much as crashes :)
>>>>
>>>>> I tried to debug why this device breaks the test, but the test
>>>>> environment is giving me a hard time ... how do you best hook a gdb into
>>>>> that framework, so you can trace such problems?
>>>>> Anyway, with some trial and error, I found out that it seems like the
>>>>>
>>>>>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>>>>>
>>>>> in spapr_rng_instance_init() is causing the problems. Could it be that
>>>>> object_resolve_path_type is not working with the test environment?
>>>>
>>>> I tried to figure out why this device breaks under this test, but
>>>> couldn't, so I posted with the "for unknown reason" comment.
>>>
>>> I've debugged this now for a while (thanks for the tip with
>>> MALLOC_PERTURB, by the way!) and it seems to me that the problem is in
>>> the macio object than in spapr-rng - the latter is just the victim of
>>> some memory corruption caused by the first one: The
>>> object_resolve_path_type() crashes while trying to go through the macio
>>> object.
>>>
>>> So could you please add the "dc->cannot_even_create_with_object_new_yet
>>> = true;" to macio_class_init() instead? ... that seems to fix the crash
>>> for me, too, and is likely the better place.
>> 
>> Hmm.
>> 
>> For most of the devices my patch marks, we have a pretty good idea on
>> what's wrong with them.  spapr-rng is among the exceptions.  You believe
>> it's actually "the macio object".  Which one?  "macio" is abstract...
>> 
>> You report introspecting "spapr-rng" crashes "while trying to go through
>> the macio object".  I wonder how omitting introspection of macio objects
>> (that's what marking them does to this test) could affect the object
>> we're going through when we crash.
>
> I have to correct myself: It's not going through the macio object, the
> problem is actually the "macio[0]" property that is created during
> memory_region_init() with object_property_add_child() ... the property
> points to a free()d object when the crash happens.
>
>>> Or maybe we could get this also fixed? The problem could be the
>>> memory_region_init(&s->bar, NULL, "macio", 0x80000) in
>>> macio_instance_init() ... is this ok here? Or does this rather have to
>>> go to the realize() function instead?
>> 
>> Hmm, does creating and destroying a macio object leave the memory region
>> behind?
>> 
>> Paolo, is calling memory_region_init() in an instance_init() method
>> okay?
>
> As Paolo mentioned, we likely need to pass an "owner" to
> memory_region_init() or the macio memory region will get attached to
> "/unattached" instead - and then leave a dangling link property behind
> when the original macio object got destroyed.
>
> By the way, there are some more spots like this in the code, e.g. in
> pxa2xx_fir_instance_init() in hw/arm/pxa2xx.c ...

That's a memory_region_init_io(), so I should search for that pattern,
too.  Any memory_region_init*() in fact, I guess.  >300 hits :(

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Peter Maydell]

On 28 September 2015 at 15:35, Markus Armbruster <armbru@redhat.com> wrote:
> That's a memory_region_init_io(), so I should search for that pattern,
> too.  Any memory_region_init*() in fact, I guess.  >300 hits :(

You can ignore all the ones which are in board models or
non-qdevified devices, though.

-- PMM

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Thomas Huth <thuth@redhat.com> writes:
>
>> On 28/09/15 10:11, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>> 
>>>> On 25/09/15 16:17, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>
>>>>>> On 24/09/15 20:57, Markus Armbruster wrote:
>>>>>>> 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_even_create_with_object_new_yet
>>>>>>> to mark them:
>>>> ...
>>>>>>>  static void pxa2xx_pcmcia_register_types(void)
>>>>>>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>>>>>>> index ed43d5e..e1b115d 100644
>>>>>>> --- a/hw/ppc/spapr_rng.c
>>>>>>> +++ b/hw/ppc/spapr_rng.c
>>>>>>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>>>>>>>      dc->realize = spapr_rng_realize;
>>>>>>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>>>>>      dc->props = spapr_rng_properties;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Reason: crashes device-introspect-test for unknown reason.
>>>>>>> +     */
>>>>>>> +    dc->cannot_even_create_with_object_new_yet = true;
>>>>>>>  }
>>>>>>
>>>>>> Please don't do that! That breaks the help output from
>>>>>> "-device spapr-rng,?" which should help the user to see how to use this
>>>>>> device!
>>>>>
>>>>> Well, device-introspection-test makes qemu crash, with the backtrace
>>>>> pointing squarely to this device.  Stands to reason that device
>>>>> introspection could crash in normal usage, too.  Until the crash is
>>>>> debugged, we better disable introspection of this device.
>>>>>
>>>>> I quite agree that disabling introspection hurts users.  Just not as
>>>>> much as crashes :)
>>>>>
>>>>>> I tried to debug why this device breaks the test, but the test
>>>>>> environment is giving me a hard time ... how do you best hook a gdb into
>>>>>> that framework, so you can trace such problems?
>>>>>> Anyway, with some trial and error, I found out that it seems like the
>>>>>>
>>>>>>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>>>>>>
>>>>>> in spapr_rng_instance_init() is causing the problems. Could it be that
>>>>>> object_resolve_path_type is not working with the test environment?
>>>>>
>>>>> I tried to figure out why this device breaks under this test, but
>>>>> couldn't, so I posted with the "for unknown reason" comment.
>>>>
>>>> I've debugged this now for a while (thanks for the tip with
>>>> MALLOC_PERTURB, by the way!) and it seems to me that the problem is in
>>>> the macio object than in spapr-rng - the latter is just the victim of
>>>> some memory corruption caused by the first one: The
>>>> object_resolve_path_type() crashes while trying to go through the macio
>>>> object.
>>>>
>>>> So could you please add the "dc->cannot_even_create_with_object_new_yet
>>>> = true;" to macio_class_init() instead? ... that seems to fix the crash
>>>> for me, too, and is likely the better place.
>>> 
>>> Hmm.
>>> 
>>> For most of the devices my patch marks, we have a pretty good idea on
>>> what's wrong with them.  spapr-rng is among the exceptions.  You believe
>>> it's actually "the macio object".  Which one?  "macio" is abstract...
>>> 
>>> You report introspecting "spapr-rng" crashes "while trying to go through
>>> the macio object".  I wonder how omitting introspection of macio objects
>>> (that's what marking them does to this test) could affect the object
>>> we're going through when we crash.
>>
>> I have to correct myself: It's not going through the macio object, the
>> problem is actually the "macio[0]" property that is created during
>> memory_region_init() with object_property_add_child() ... the property
>> points to a free()d object when the crash happens.
>>
>>>> Or maybe we could get this also fixed? The problem could be the
>>>> memory_region_init(&s->bar, NULL, "macio", 0x80000) in
>>>> macio_instance_init() ... is this ok here? Or does this rather have to
>>>> go to the realize() function instead?
>>> 
>>> Hmm, does creating and destroying a macio object leave the memory region
>>> behind?
>>> 
>>> Paolo, is calling memory_region_init() in an instance_init() method
>>> okay?
>>
>> As Paolo mentioned, we likely need to pass an "owner" to
>> memory_region_init() or the macio memory region will get attached to
>> "/unattached" instead - and then leave a dangling link property behind
>> when the original macio object got destroyed.
>>
>> By the way, there are some more spots like this in the code, e.g. in
>> pxa2xx_fir_instance_init() in hw/arm/pxa2xx.c ...
>
> That's a memory_region_init_io(), so I should search for that pattern,
> too.  Any memory_region_init*() in fact, I guess.  >300 hits :(

I tracked down problematic devices in two ways:

1. I made device-introspection-test run "info qom-tree", which has a
   lovely propensity to crash when a crappy device left dangling pointer
   behind.  This led me to "cgthree", "cuda", "integrator_debug",
   "macio-oldworld", "macio-newworld", "pxa2xx-fir", "SUNW,tcx".  They
   all create memory regions without owner in their instance_init()
   method.

   "pxa2xx-pcmcia" does, too.  It's already marked in v3, because it
   actually crashes.  Perhaps it has additional problems.

2. I instrumented memory_region_init() and object_init_with_type() to
   crash when the former is called with null owner from within
   ->instance_init().  I verified this catches cases like the above.  It
   doesn't catch any new ones.  This makes me reasonably confident I got
   them all.

I'll send out v4 shortly.

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Peter Maydell]

On 28 September 2015 at 20:36, Markus Armbruster <armbru@redhat.com> wrote:
> 1. I made device-introspection-test run "info qom-tree", which has a
>    lovely propensity to crash when a crappy device left dangling pointer
>    behind.  This led me to "cgthree", "cuda", "integrator_debug",
>    "macio-oldworld", "macio-newworld", "pxa2xx-fir", "SUNW,tcx".  They
>    all create memory regions without owner in their instance_init()
>    method.

I guess these are all just "oops, we forgot to pass the Object* in
instead of NULL" bugs rather than more difficult fixes.

-- PMM

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 28 September 2015 at 20:36, Markus Armbruster <armbru@redhat.com> wrote:
>> 1. I made device-introspection-test run "info qom-tree", which has a
>>    lovely propensity to crash when a crappy device left dangling pointer
>>    behind.  This led me to "cgthree", "cuda", "integrator_debug",
>>    "macio-oldworld", "macio-newworld", "pxa2xx-fir", "SUNW,tcx".  They
>>    all create memory regions without owner in their instance_init()
>>    method.
>
> I guess these are all just "oops, we forgot to pass the Object* in
> instead of NULL" bugs rather than more difficult fixes.

I'm leaving the actual fixing to people with a better understanding of
these devices, and ability to actually test them.

Re: [Qemu-devel] [PATCH v3 6/7] qdev: Protect device-list-properties against broken devices

[Paolo Bonzini]

On 2/09/2015 10:05, Markus Armbruster wrote:
> > > 1. I made device-introspection-test run "info qom-tree", which has a
> > >    lovely propensity to crash when a crappy device left dangling pointer
> > >    behind.  This led me to "cgthree", "cuda", "integrator_debug",
> > >    "macio-oldworld", "macio-newworld", "pxa2xx-fir", "SUNW,tcx".  They
> > >    all create memory regions without owner in their instance_init()
> > >    method.
> >
> > I guess these are all just "oops, we forgot to pass the Object* in
> > instead of NULL" bugs rather than more difficult fixes.
>
> I'm leaving the actual fixing to people with a better understanding of
> these devices, and ability to actually test them.

Gave it a shot...

Paolo

[Qemu-devel] [PATCH v3 7/7] Revert "qdev: Use qdev_get_device_class() for -device <type>, help"

[Markus Armbruster]

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 <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qdev-monitor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index eb7aef2..1cadefb 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);
-- 
2.4.3