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

[Qemu-devel] [PATCH 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>

Markus Armbruster (7):
  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"
  tests: Simplify how qom-test is run

 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/s390x/event-facility.c      |   3 ++
 hw/s390x/sclp.c                |   3 ++
 include/hw/qdev-core.h         |  13 +++++
 qdev-monitor.c                 |   9 ++--
 qmp.c                          |  11 ++++
 qom/cpu.c                      |   2 +
 target-i386/cpu.c              |   2 +
 target-ppc/kvm.c               |   4 ++
 tests/Makefile                 |  16 +++---
 tests/device-introspect-test.c | 117 +++++++++++++++++++++++++++++++++++++++++
 tests/drive_del-test.c         |  22 +++-----
 tests/ide-test.c               |   8 +--
 tests/libqtest.c               |  36 ++++++++++++-
 tests/libqtest.h               |  33 ++++++++++++
 21 files changed, 276 insertions(+), 33 deletions(-)
 create mode 100644 tests/device-introspect-test.c

-- 
2.4.3

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

[Markus Armbruster]

Unused since commit d766825.

Signed-off-by: Markus Armbruster <armbru@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 2/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>
---
 tests/drive_del-test.c | 22 ++++++----------------
 tests/ide-test.c       |  8 ++------
 tests/libqtest.c       | 35 +++++++++++++++++++++++++++++++++++
 tests/libqtest.h       | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 76 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 4a07e3a..ef0a473 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -488,9 +488,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);
@@ -502,9 +500,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..f98bde9 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,14 @@ 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;
+
+    va_start(ap, fmt);
+    return qtest_hmpv(global_qtest, fmt, ap);
+    va_end(ap);
+}
 
 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 3/7] device-introspect-test: New, covering device introspection

[Markus Armbruster]

The test doesn't check 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.  Blacklist them in
the test.  The next commits will fix.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile                 |  11 ++-
 tests/device-introspect-test.c | 149 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 157 insertions(+), 3 deletions(-)
 create mode 100644 tests/device-introspect-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 7c6025a..4559045 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -86,6 +86,9 @@ 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 += 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)
 gcov-files-ipack-y += hw/char/ipoctal232.c
@@ -375,6 +378,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
@@ -440,7 +444,8 @@ 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),))
-check-qtest-y=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y))
+check-qtest-y=$(check-qtest-generic-y)
+check-qtest-y+=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y))
 endif
 
 qtest-obj-y = tests/libqtest.o $(test-util-obj-y)
@@ -478,8 +483,8 @@ $(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 $@")
-	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
+		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) $(check-qtest-$*-y),"GTESTER $@")
+	$(if $(CONFIG_GCOV),@for f in $(gcov-files-generic-y) $(gcov-files-$*-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..bcaaf2a
--- /dev/null
+++ b/tests/device-introspect-test.c
@@ -0,0 +1,149 @@
+/*
+ * 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 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;
+
+    /* FIXME device-list-properties crashes for abstract device, skip  */
+    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

[Qemu-devel] [PATCH 4/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>
---
 qmp.c                          |  6 ++++++
 tests/device-introspect-test.c | 11 ++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/qmp.c b/qmp.c
index 9623c80..6f370d5 100644
--- a/qmp.c
+++ b/qmp.c
@@ -520,6 +520,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 bcaaf2a..3e40877 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -45,13 +45,10 @@ static void test_one_device(const char *type)
     QDict *resp;
     char *help;
 
-    /* FIXME device-list-properties crashes for abstract device, skip  */
-    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 5/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 or hang during cleanup (didn't debug them, so I can't say
  why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
  "s390-sclp-event-facility", "sclp"

* Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
  "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs

* Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
  "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
  "host-powerpc-cpu"

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-* -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/s390x/event-facility.c      |  3 +++
 hw/s390x/sclp.c                |  3 +++
 include/hw/qdev-core.h         | 13 +++++++++++++
 qmp.c                          |  5 +++++
 qom/cpu.c                      |  2 ++
 target-i386/cpu.c              |  2 ++
 target-ppc/kvm.c               |  4 ++++
 tests/device-introspect-test.c | 29 -----------------------------
 15 files changed, 62 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 2185542..d558b10 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/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 fd277e1..b2b46c9 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -562,6 +562,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 6f370d5..257f09f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -526,6 +526,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/qom/cpu.c b/qom/cpu.c
index fb80d13..5ff9ea7 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -361,6 +361,8 @@ static void cpu_class_init(ObjectClass *klass, void *data)
      * IRQs, adding reset handlers, halting non-first CPUs, ...
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    /* Reason: use after free: cpu_exec_init() saves CPUState in cpus */
+    dc->cannot_even_create_with_object_new_yet = true;
 }
 
 static const TypeInfo cpu_type_info = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7c52714..32e7b84 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)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 110436d..9943bba 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2188,6 +2188,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();
@@ -2214,6 +2215,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/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 3e40877..ca82f0c 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

[Qemu-devel] [PATCH 6/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>
---
 qdev-monitor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index f9e2d62..0e14747 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

[Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run

[Markus Armbruster]

Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
every target.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 4559045..28c5f93 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -219,10 +219,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 \
-- 
2.4.3

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

[Christian Borntraeger]

Am 18.09.2015 um 14:00 schrieb 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 or hang during cleanup (didn't debug them, so I can't say
>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>   "s390-sclp-event-facility", "sclp"

Ack for the sclp things. Theses devices are created by the machine and
sclp creates the event-facility, so not having a way to query properties
for these devices is better than a hang.

David, can you have a look on why these devices fail as outlined?



> 
> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> 
> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>   "host-powerpc-cpu"
> 
> 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-* -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/s390x/event-facility.c      |  3 +++
>  hw/s390x/sclp.c                |  3 +++
>  include/hw/qdev-core.h         | 13 +++++++++++++
>  qmp.c                          |  5 +++++
>  qom/cpu.c                      |  2 ++
>  target-i386/cpu.c              |  2 ++
>  target-ppc/kvm.c               |  4 ++++
>  tests/device-introspect-test.c | 29 -----------------------------
>  15 files changed, 62 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 2185542..d558b10 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/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 fd277e1..b2b46c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -562,6 +562,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 6f370d5..257f09f 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -526,6 +526,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/qom/cpu.c b/qom/cpu.c
> index fb80d13..5ff9ea7 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -361,6 +361,8 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
>       */
>      dc->cannot_instantiate_with_device_add_yet = true;
> +    /* Reason: use after free: cpu_exec_init() saves CPUState in cpus */
> +    dc->cannot_even_create_with_object_new_yet = true;
>  }
> 
>  static const TypeInfo cpu_type_info = {
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7c52714..32e7b84 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)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..9943bba 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2188,6 +2188,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();
> @@ -2214,6 +2215,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/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 3e40877..ca82f0c 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);
>      }
> 

Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run

[Andreas Färber]

Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
> every target.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 4559045..28c5f93 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -219,10 +219,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)

Does this -generic- have the same filtering code to avoid running the
tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.

Regards,
Andreas

>  
>  check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>  	comments.json empty.json enum-empty.json enum-missing-data.json \
> 


-- 
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 7/7] tests: Simplify how qom-test is run

[Markus Armbruster]

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

> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>> every target.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/Makefile | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>> 
>> diff --git a/tests/Makefile b/tests/Makefile
>> index 4559045..28c5f93 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -219,10 +219,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)
>
> Does this -generic- have the same filtering code to avoid running the
> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.

I'm dense today.  Can you explain the filtering code to me?

[...]

Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run

[Andreas Färber]

Am 18.09.2015 um 16:24 schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
>> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>>> every target.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/Makefile | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index 4559045..28c5f93 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -219,10 +219,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)
>>
>> Does this -generic- have the same filtering code to avoid running the
>> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
> 
> I'm dense today.  Can you explain the filtering code to me?

For practical purpose,s x86_64 adds all tests from i386, that included
qom-test then. If we now add it for x86_64 too, it got executed twice,
which the above $(if ...) fixes by not adding it for x86_64 if it's
already in. Just checking whether -generic- has equivalent filtering or
other code somewhere else?

BR,
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 1/7] libqtest: Clean up unused QTestState member sigact_old

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> Unused since commit d766825.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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;
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> 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>
> ---
>  tests/drive_del-test.c | 22 ++++++----------------
>  tests/ide-test.c       |  8 ++------
>  tests/libqtest.c       | 35 +++++++++++++++++++++++++++++++++++
>  tests/libqtest.h       | 33 +++++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+), 22 deletions(-)
> 


> @@ -774,6 +801,14 @@ 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;
> +
> +    va_start(ap, fmt);
> +    return qtest_hmpv(global_qtest, fmt, ap);
> +    va_end(ap);

Umm, that isn't quite what you meant :)

With the dead code after return fixed,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> The test doesn't check the output makes any sense, only that QEMU

Reads slightly better as:
s/check/check that/

> 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.  Blacklist them in
> the test.  The next commits will fix.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile                 |  11 ++-
>  tests/device-introspect-test.c | 149 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 157 insertions(+), 3 deletions(-)
>  create mode 100644 tests/device-introspect-test.c
> 

> @@ -478,8 +483,8 @@ $(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 $@")
> -	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
> +		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) $(check-qtest-$*-y),"GTESTER $@")

Long line; worth adding a \ line-wrap?

> +++ b/tests/device-introspect-test.c
> @@ -0,0 +1,149 @@

> +/*
> + * Covers QMP device-list-properties and HMP device_add help.  We
> + * currently don't check their output makes sense, only that QEMU

again, might be better as:
s/check/check that/

> +static void test_one_device(const char *type)
> +{
> +    QDict *resp;
> +    char *help;
> +
> +    /* FIXME device-list-properties crashes for abstract device, skip  */
> +    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);

The comment mentions a skip, and the commit message header mentioned a
blacklist, but I'm not seeing a skip here. Am I missing something?

> +
> +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",
> +    };

Okay, here's the blacklist the commit message mentioned, but maybe that
means the earlier comment is stale?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> 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>
> ---
>  qmp.c                          |  6 ++++++
>  tests/device-introspect-test.c | 11 ++++-------
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 

> +++ b/tests/device-introspect-test.c
> @@ -45,13 +45,10 @@ static void test_one_device(const char *type)
>      QDict *resp;
>      char *help;
>  
> -    /* FIXME device-list-properties crashes for abstract device, skip  */
> -    if (strcmp(type, "device")) {

Ah, this answers part of my confusion on 3/7.  I was looking for so
condition on 'abstract', but the skip was based on a condition on
'device' (which happens to be abstract).  Don't know if a different
wording on that comment would help; maybe:

FIXME device-list-properties crashes for device, because it is abstract;
skip

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]

On 09/18/2015 06:00 AM, 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.
> 

> 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:

(intentionally) Ugly because it is a workaround, but then again, giving
the attribute an ugly name will help call attention to the specific
device owners to fix the mess.  I can live with that.

> 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-* -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'

Not ideal, but much better than a crash, so it gets my vote for
inclusion as an incremental improvement.


> +++ 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;

And a sufficiently verbose explanation for why the code is so ugly.

> @@ -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 */
> -        }

The devices are still broken, but the testsuite no longer flags them as
broken because it no longer crashes or hangs, and you intentionally
wrote the test to treat any output (including a graceful error message)
as successful use of the probe.  The ugly attribute is now our only
documentation of the problems, but that is still something sufficient to
hopefully get it fixed.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> 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>
> ---
>  qdev-monitor.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eduardo Habkost]

On Fri, Sep 18, 2015 at 02:00:38PM +0200, 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 or hang during cleanup (didn't debug them, so I can't say
>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>   "s390-sclp-event-facility", "sclp"
> 
> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs

That's isn't true for all CPU classes, only the ones that (incorrectly)
call cpu_exec_init() on instance_init instead of realize. I believe at
least TYPE_POWERPC_CPU is safe already.

-- 
Eduardo

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

[Thomas Huth]

On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>   "s390-sclp-event-facility", "sclp"
> 
> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> 
> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>   "host-powerpc-cpu"

I just had a look at the powerpc code - you're likely talking about
the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
target-ppc/kvm.c ? That should be fine, I think, because
kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
enabled.

 Thomas

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

[Eduardo Habkost]

On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
> >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >   "s390-sclp-event-facility", "sclp"
> > 
> > * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> >   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> > 
> > * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> >   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> >   "host-powerpc-cpu"
> 
> I just had a look at the powerpc code - you're likely talking about
> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> target-ppc/kvm.c ? That should be fine, I think, because
> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> enabled.

It's true that currently the assert() will never trigger, but we will
have to eventually move class registration to type_init if we want to
make a generic query-cpu-definitions implementation work properly
without depending on global machine accel configuration.

It won't hurt to set cannot_even_create_with_object_new_yet properly to
reflect that the class isn't ready yet.

-- 
Eduardo

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/18/2015 06:00 AM, Markus Armbruster wrote:
>> 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>
>> ---
>>  tests/drive_del-test.c | 22 ++++++----------------
>>  tests/ide-test.c       |  8 ++------
>>  tests/libqtest.c       | 35 +++++++++++++++++++++++++++++++++++
>>  tests/libqtest.h       | 33 +++++++++++++++++++++++++++++++++
>>  4 files changed, 76 insertions(+), 22 deletions(-)
>> 
>
>
>> @@ -774,6 +801,14 @@ 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;
>> +
>> +    va_start(ap, fmt);
>> +    return qtest_hmpv(global_qtest, fmt, ap);
>> +    va_end(ap);
>
> Umm, that isn't quite what you meant :)

D'oh!

> With the dead code after return fixed,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/18/2015 06:00 AM, Markus Armbruster wrote:
>> The test doesn't check the output makes any sense, only that QEMU
>
> Reads slightly better as:
> s/check/check that/

Okay.

>> 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.  Blacklist them in
>> the test.  The next commits will fix.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/Makefile                 |  11 ++-
>>  tests/device-introspect-test.c | 149 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 157 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/device-introspect-test.c
>> 
>
>> @@ -478,8 +483,8 @@ $(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 $@")
>> -	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>> +		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) $(check-qtest-$*-y),"GTESTER $@")
>
> Long line; worth adding a \ line-wrap?

I'll look into it.

>> +++ b/tests/device-introspect-test.c
>> @@ -0,0 +1,149 @@
>
>> +/*
>> + * Covers QMP device-list-properties and HMP device_add help.  We
>> + * currently don't check their output makes sense, only that QEMU
>
> again, might be better as:
> s/check/check that/

Okay.

>> +static void test_one_device(const char *type)
>> +{
>> +    QDict *resp;
>> +    char *help;
>> +
>> +    /* FIXME device-list-properties crashes for abstract device, skip  */
>> +    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);
>
> The comment mentions a skip, and the commit message header mentioned a
> blacklist, but I'm not seeing a skip here. Am I missing something?

The conditional skips part of the test when type is "device", because
that part crashes.

>> +
>> +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",
>> +    };
>
> Okay, here's the blacklist the commit message mentioned, but maybe that
> means the earlier comment is stale?

The commit message is slightly inaccurate: unlike the other devices,
"device" isn't blacklisted completely.  Good enough?

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/18/2015 06:00 AM, 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.
>> 
>
>> 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:
>
> (intentionally) Ugly because it is a workaround, but then again, giving
> the attribute an ugly name will help call attention to the specific
> device owners to fix the mess.  I can live with that.

Named in Anthony's memory (see commit efec3dd) ;-P

>> 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-* -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'
>
> Not ideal, but much better than a crash, so it gets my vote for
> inclusion as an incremental improvement.
>
>
>> +++ 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;
>
> And a sufficiently verbose explanation for why the code is so ugly.
>
>> @@ -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 */
>> -        }
>
> The devices are still broken, but the testsuite no longer flags them as
> broken because it no longer crashes or hangs, and you intentionally
> wrote the test to treat any output (including a graceful error message)
> as successful use of the probe.  The ugly attribute is now our only
> documentation of the problems, but that is still something sufficient to
> hopefully get it fixed.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Sep 18, 2015 at 02:00:38PM +0200, 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 or hang during cleanup (didn't debug them, so I can't say
>>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>   "s390-sclp-event-facility", "sclp"
>> 
>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>
> That's isn't true for all CPU classes, only the ones that (incorrectly)
> call cpu_exec_init() on instance_init instead of realize. I believe at
> least TYPE_POWERPC_CPU is safe already.

Okay, I'll try to mark only the ones that actually screw up.

Thanks!

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>> On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
>> >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>> >   "s390-sclp-event-facility", "sclp"
>> > 
>> > * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>> >   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>> > 
>> > * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>> >   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>> >   "host-powerpc-cpu"
>> 
>> I just had a look at the powerpc code - you're likely talking about
>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>> target-ppc/kvm.c ? That should be fine, I think, because
>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>> enabled.

Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
and off, where C is the appropriate host CPU.

> It's true that currently the assert() will never trigger, but we will
> have to eventually move class registration to type_init if we want to
> make a generic query-cpu-definitions implementation work properly
> without depending on global machine accel configuration.

Good point.  These CPUs need a TODO marker.

> It won't hurt to set cannot_even_create_with_object_new_yet properly to
> reflect that the class isn't ready yet.

Marking cannot_even_create_with_object_new_yet() is the obvious TODO
marker.  It has an unnecessary side effect: we disable introspection for
these CPUs needlessly.  Tolerable?

Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run

[Markus Armbruster]

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

> Am 18.09.2015 um 16:24 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>>> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>>>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>>>> every target.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  tests/Makefile | 5 +----
>>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index 4559045..28c5f93 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -219,10 +219,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)
>>>
>>> Does this -generic- have the same filtering code to avoid running the
>>> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
>> 
>> I'm dense today.  Can you explain the filtering code to me?
>
> For practical purpose,s x86_64 adds all tests from i386, that included
> qom-test then. If we now add it for x86_64 too, it got executed twice,
> which the above $(if ...) fixes by not adding it for x86_64 if it's
> already in. Just checking whether -generic- has equivalent filtering or
> other code somewhere else?

I'll double-check.  Thanks!

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

[David Hildenbrand]

> Am 18.09.2015 um 14:00 schrieb 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 or hang during cleanup (didn't debug them, so I can't say
> >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >   "s390-sclp-event-facility", "sclp"
> 
> Ack for the sclp things. Theses devices are created by the machine and
> sclp creates the event-facility, so not having a way to query properties
> for these devices is better than a hang.
> 
> David, can you have a look on why these devices fail as outlined?
> 

The problem was that the quiesce and cpu hotplug sclp event devices had no
parent (onoly a parent bus). So when the bus (inside the event facility) was
removed, it looped forever trying remove/unparent it's "bus childs" (which had
no parent).

sclp (parent=machine)
    -> even-facility (parent=sclp)
                    -> bus (parent=event-facility)
                          -> quiesce (parent=null)
                          -> cpu hotplug (parent=null)

Maybe that helps others struggling with the same symptoms.


Just a quick comment on the introspection:

I don't think it is a good idea to expose properties that way. Temporarily
creating devices for the sake of querying property names sounds very wrong to
me, because certain devices require certain knowledge about how and when they
can be created.

Feels a little like hacking an interface to a java program, which allows to
create any object of a special kind dynamically (constructor arguments?),
reading some member variable (names) via reflections and then throwing that
object away. Which sounds very wrong to me.

Wonder if it wouldn't make more sense to query only the static properties of a
device. I mean if the dynamic properties are dynamic by definition (and can
change during runtime). So looking up the static properties via the typename
feels more KIS-style to me. Of course, the relevant properties would have to be
defined statically then, which shouldn't be a problem.

Dynamic properties that are actually created could depend on almost
everything in the system - why fake something that we don't know.

David

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 08:09:48AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Fri, Sep 18, 2015 at 02:00:38PM +0200, 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 or hang during cleanup (didn't debug them, so I can't say
> >>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >>   "s390-sclp-event-facility", "sclp"
> >> 
> >> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> >>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> >
> > That's isn't true for all CPU classes, only the ones that (incorrectly)
> > call cpu_exec_init() on instance_init instead of realize. I believe at
> > least TYPE_POWERPC_CPU is safe already.
> 
> Okay, I'll try to mark only the ones that actually screw up.

Most of them screw up, today. If you prefer to simply set it to true on
TYPE_CPU and then explicitly override it to false only on the few
subclasses that are already fixed, I think it would be OK.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 08:14:56AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> >> On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
> >> >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >> >   "s390-sclp-event-facility", "sclp"
> >> > 
> >> > * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> >> >   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> >> > 
> >> > * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> >> >   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> >> >   "host-powerpc-cpu"
> >> 
> >> I just had a look at the powerpc code - you're likely talking about
> >> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> >> target-ppc/kvm.c ? That should be fine, I think, because
> >> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> >> enabled.
> 
> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> and off, where C is the appropriate host CPU.
> 
> > It's true that currently the assert() will never trigger, but we will
> > have to eventually move class registration to type_init if we want to
> > make a generic query-cpu-definitions implementation work properly
> > without depending on global machine accel configuration.
> 
> Good point.  These CPUs need a TODO marker.
> 
> > It won't hurt to set cannot_even_create_with_object_new_yet properly to
> > reflect that the class isn't ready yet.
> 
> Marking cannot_even_create_with_object_new_yet() is the obvious TODO
> marker.  It has an unnecessary side effect: we disable introspection for
> these CPUs needlessly.  Tolerable?

To me, it is. We already disable introspection of these CPUs needlessly
in all cases except when KVM is explicitly enabled in the command-line.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
> > Am 18.09.2015 um 14:00 schrieb 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 or hang during cleanup (didn't debug them, so I can't say
> > >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> > >   "s390-sclp-event-facility", "sclp"
> > 
> > Ack for the sclp things. Theses devices are created by the machine and
> > sclp creates the event-facility, so not having a way to query properties
> > for these devices is better than a hang.
> > 
> > David, can you have a look on why these devices fail as outlined?
> > 
> 
> The problem was that the quiesce and cpu hotplug sclp event devices had no
> parent (onoly a parent bus). So when the bus (inside the event facility) was
> removed, it looped forever trying remove/unparent it's "bus childs" (which had
> no parent).
> 
> sclp (parent=machine)
>     -> even-facility (parent=sclp)
>                     -> bus (parent=event-facility)
>                           -> quiesce (parent=null)
>                           -> cpu hotplug (parent=null)
> 
> Maybe that helps others struggling with the same symptoms.
> 
> 
> Just a quick comment on the introspection:
> 
> I don't think it is a good idea to expose properties that way. Temporarily
> creating devices for the sake of querying property names sounds very wrong to
> me, because certain devices require certain knowledge about how and when they
> can be created.

No knowledge should be required for object_new(). Classes' instance_init
functions should have no side-effects outside the object itself.

> 
> Feels a little like hacking an interface to a java program, which allows to
> create any object of a special kind dynamically (constructor arguments?),
> reading some member variable (names) via reflections and then throwing that
> object away. Which sounds very wrong to me.

I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
and prototype-based approaches to inheritance and property
introspection.

> 
> Wonder if it wouldn't make more sense to query only the static properties of a
> device. I mean if the dynamic properties are dynamic by definition (and can
> change during runtime). So looking up the static properties via the typename
> feels more KIS-style to me. Of course, the relevant properties would have to be
> defined statically then, which shouldn't be a problem.

It depends on your definition of "shouldn't be a problem". :)

The static property system is more limited than the QOM dynamic property
system, today. We already depend on introspection of dynamic properties
registered by instance_init functions, so we would need to move
everything to a better static property system if we want to stop doing
object_new() during class introspection without regressions.

> 
> Dynamic properties that are actually created could depend on almost
> everything in the system - why fake something that we don't know.

Properties registered on instance_init shouldn't depend on anything else
on the system. Properties registered later in the object lifetime (e.g.
on realize) can.

-- 
Eduardo

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

[Thomas Huth]

On 21/09/15 08:14, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>> On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
>>>>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>>>   "s390-sclp-event-facility", "sclp"
>>>>
>>>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>>>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>>>>
>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>>>>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>>>>   "host-powerpc-cpu"
>>>
>>> I just had a look at the powerpc code - you're likely talking about
>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>> target-ppc/kvm.c ? That should be fine, I think, because
>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>> enabled.
> 
> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> and off, where C is the appropriate host CPU.

In both cases (KVM on and off), I simply get:

# qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
'host-powerpc64-cpu' is not a valid device model name
# qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
'host-powerpc64-cpu' is not a valid device model name

No crash/abort here.

 Thomas

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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 21/09/15 08:14, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>>> On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
>>>>>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>>>>   "s390-sclp-event-facility", "sclp"
>>>>>
>>>>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>>>>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>>>>>
>>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>>>>>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>>>>>   "host-powerpc-cpu"
>>>>
>>>> I just had a look at the powerpc code - you're likely talking about
>>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>>> target-ppc/kvm.c ? That should be fine, I think, because
>>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>>> enabled.
>> 
>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
>> and off, where C is the appropriate host CPU.
>
> In both cases (KVM on and off), I simply get:
>
> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
> 'host-powerpc64-cpu' is not a valid device model name
> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
> 'host-powerpc64-cpu' is not a valid device model name
>
> No crash/abort here.

No introspection, either.  Can you try the same with QMP command
device-list-properties?

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

[Thomas Huth]

On 21/09/15 18:39, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 21/09/15 08:14, Markus Armbruster wrote:
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>>>> On 18/09/15 14:00, 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 or hang during cleanup (didn't debug them, so I can't say
>>>>>>   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>>>>>   "s390-sclp-event-facility", "sclp"
>>>>>>
>>>>>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>>>>>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>>>>>>
>>>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>>>>>>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>>>>>>   "host-powerpc-cpu"
>>>>>
>>>>> I just had a look at the powerpc code - you're likely talking about
>>>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>>>> target-ppc/kvm.c ? That should be fine, I think, because
>>>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>>>> enabled.
>>>
>>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
>>> and off, where C is the appropriate host CPU.
>>
>> In both cases (KVM on and off), I simply get:
>>
>> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
>> 'host-powerpc64-cpu' is not a valid device model name
>> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
>> 'host-powerpc64-cpu' is not a valid device model name
>>
>> No crash/abort here.
> 
> No introspection, either.  Can you try the same with QMP command
> device-list-properties?

# ppc64-softmmu/qemu-system-ppc64 -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
{"error": {"class": "DeviceNotFound", "desc": "Device 'host-powerpc64-cpu' not found"}}
{ "execute": "quit" }
{"return": {}}
{"timestamp": {"seconds": 1442856053, "microseconds": 732079}, "event": "SHUTDOWN"}

# ppc64-softmmu/qemu-system-ppc64 -machine accel=kvm -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
{"return": [{"name": "compat", "description": "compatibility mode, power6/power7/power8", "type": "str"}]}
{ "execute": "quit" }
{"return": {}}
{"timestamp": {"seconds": 1442856113, "microseconds": 913588}, "event": "SHUTDOWN"}

 Thomas

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 07:22:51PM +0200, Thomas Huth wrote:
> On 21/09/15 18:39, Markus Armbruster wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> >> On 21/09/15 08:14, Markus Armbruster wrote:
> >>> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> >>>>> On 18/09/15 14:00, Markus Armbruster wrote:
[...]
> >>>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> >>>>>>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> >>>>>>   "host-powerpc-cpu"
> >>>>>
> >>>>> I just had a look at the powerpc code - you're likely talking about
> >>>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> >>>>> target-ppc/kvm.c ? That should be fine, I think, because
> >>>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> >>>>> enabled.
> >>>
> >>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> >>> and off, where C is the appropriate host CPU.
> >>
> >> In both cases (KVM on and off), I simply get:
> >>
> >> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
> >> 'host-powerpc64-cpu' is not a valid device model name
> >> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
> >> 'host-powerpc64-cpu' is not a valid device model name
> >>
> >> No crash/abort here.
> > 
> > No introspection, either.  Can you try the same with QMP command
> > device-list-properties?
> 
> # ppc64-softmmu/qemu-system-ppc64 -nodefaults -S -display none -qmp stdio
[...]
> { "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
> {"error": {"class": "DeviceNotFound", "desc": "Device 'host-powerpc64-cpu' not found"}}
> 
> # ppc64-softmmu/qemu-system-ppc64 -machine accel=kvm -nodefaults -S -display none -qmp stdio
[...]
> { "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
> {"return": [{"name": "compat", "description": "compatibility mode, power6/power7/power8", "type": "str"}]}

So, introspection is already broken, but inconsistently broken (because
it depends on machine arguments). Setting
cannot_even_create_with_object_new_yet will make it more consistently
broken.

-- 
Eduardo

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

[David Hildenbrand]

> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.

While this should theoretically be true, I can guarantee to you that this is
not the case for all devices :) (especially as there are too many unwritten
laws related to that whole qmp/qom thing flying around).

E.g. we can save ourselves from the creation of other devices (init + realize)
by setting a device to !hotpluggable in the device class. This code simply
bypasses such checks and assumes that QEMUs internal structure can deal with
that. We saw that this doesn't work this far.

Other interfaces (object_add) seem to rely on TYPE_USER_CREATABLE, so we can't
trigger "everything" from outside QEMU.

One can argue that we simply have to fix the devices - nevertheless I think this
is the wrong approach to the problem.

> 
> > 
> > Feels a little like hacking an interface to a java program, which allows to
> > create any object of a special kind dynamically (constructor arguments?),
> > reading some member variable (names) via reflections and then throwing that
> > object away. Which sounds very wrong to me.
> 
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.
> 
> > 
> > Wonder if it wouldn't make more sense to query only the static properties of a
> > device. I mean if the dynamic properties are dynamic by definition (and can
> > change during runtime). So looking up the static properties via the typename
> > feels more KIS-style to me. Of course, the relevant properties would have to be
> > defined statically then, which shouldn't be a problem.
> 
> It depends on your definition of "shouldn't be a problem". :)

Well, it might require some internal rework of that property thingy, hehe ;)

> 
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.
> 
> > 
> > Dynamic properties that are actually created could depend on almost
> > everything in the system - why fake something that we don't know.
> 
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.
> 

Okay, so if the properties in instance_init should depend on nothing else in
the system, they are always the same for one QEMU version + device.

So maybe it would make sense to define all these "fixed properties" on a device
class level (e.g. via dc->props) and fill them with life afterwards (if we can't
completely specify them at that point). Of course this would need some
rework/careful thinking (e.g. concept of uninitialized properties).

Looking at the errors we get with that approach tells me that it will easily
break again in the future (we need tests for all devices - do we already have
it?) and makes me wonder if a definition on a device class level wouldn't be the
right thing to do - a clean interface description that is valid for each device
instance.

David

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
>> > Am 18.09.2015 um 14:00 schrieb 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 or hang during cleanup (didn't debug them, so I can't say
>> > >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>> > >   "s390-sclp-event-facility", "sclp"
>> > 
>> > Ack for the sclp things. Theses devices are created by the machine and
>> > sclp creates the event-facility, so not having a way to query properties
>> > for these devices is better than a hang.
>> > 
>> > David, can you have a look on why these devices fail as outlined?
>> > 
>> 
>> The problem was that the quiesce and cpu hotplug sclp event devices had no
>> parent (onoly a parent bus). So when the bus (inside the event facility) was
>> removed, it looped forever trying remove/unparent it's "bus childs" (which had
>> no parent).
>> 
>> sclp (parent=machine)
>>     -> even-facility (parent=sclp)
>>                     -> bus (parent=event-facility)
>>                           -> quiesce (parent=null)
>>                           -> cpu hotplug (parent=null)
>> 
>> Maybe that helps others struggling with the same symptoms.
>> 
>> 
>> Just a quick comment on the introspection:
>> 
>> I don't think it is a good idea to expose properties that way. Temporarily
>> creating devices for the sake of querying property names sounds very wrong to
>> me, because certain devices require certain knowledge about how and when they
>> can be created.

I sympathize.  However, QOM is what it is.  Its design assumption
include:

  Properties are dynamically added.  To introspect them, you need to
  create the object.

and:

> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.

I'd tighten this to "object_unref(object_new(...)) works and has no
visible side effect".

See also review of [PATCH RFC 1/7] qom: allow properties to be
registered against classes
Message-ID: <55E72154.7070404@suse.de>
http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00517.html

>> Feels a little like hacking an interface to a java program, which allows to
>> create any object of a special kind dynamically (constructor arguments?),
>> reading some member variable (names) via reflections and then throwing that
>> object away. Which sounds very wrong to me.
>
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.

I'm no friend of this aspect of QOM.  But QOM experts assure us it is
necessary (see thread quoted above).

>> 
>> Wonder if it wouldn't make more sense to query only the static properties of a
>> device. I mean if the dynamic properties are dynamic by definition (and can
>> change during runtime). So looking up the static properties via the typename
>> feels more KIS-style to me. Of course, the relevant properties would
>> have to be
>> defined statically then, which shouldn't be a problem.
>
> It depends on your definition of "shouldn't be a problem". :)
>
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.

If Dan's patch "qom: allow properties to be registered against classes"
goes in, we can certainly consider the idea to limit
device-list-properties to properties registered against classes.

Even then, the assumption "object_unref(object_new(...)) works and has
no visible side effect" remains until proven unnecessary, because
fundamental design assumptions should not be discarded lightly.

Until then, side effects in instance_init() methods are simply bugs.
This patch protects device-list-properties from known ones.

>> Dynamic properties that are actually created could depend on almost
>> everything in the system - why fake something that we don't know.
>
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.

Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run

[Markus Armbruster]

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

> Am 18.09.2015 um 16:24 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>>> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>>>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>>>> every target.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  tests/Makefile | 5 +----
>>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index 4559045..28c5f93 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -219,10 +219,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)
>>>
>>> Does this -generic- have the same filtering code to avoid running the
>>> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
>> 
>> I'm dense today.  Can you explain the filtering code to me?
>
> For practical purpose,s x86_64 adds all tests from i386, that included
> qom-test then. If we now add it for x86_64 too, it got executed twice,
> which the above $(if ...) fixes by not adding it for x86_64 if it's
> already in. Just checking whether -generic- has equivalent filtering or
> other code somewhere else?

The code in master works only sometimes.  Here's the explanation copied
from my revised patch's commit message:

    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.
    
After my patch v2 (to be sent soon), it runs exactly once per target.