[Qemu-devel] [PATCH RFC 0/1] qtest: Generic PCI device test

[Qemu-devel] [PATCH RFC 0/1] qtest: Generic PCI device test

[Markus Armbruster]

The test uses QMP introspection to find PCI devices, then tries to
cold-plug each of them.  Could be extended to hot-plug and unplug.

The tests' QMP introspection part is patterned after Andreas's
qom-test, which uses QMP to find machine types.

The following devices pass the test:

    AC97                ich9-ahci           pcnet             
    ES1370              ich9-intel-hda      piix3-ide         
    VGA                 ich9-usb-ehci1      piix3-ide-xen     
    am53c974            ich9-usb-ehci2      piix3-usb-uhci    
    cirrus-vga          ich9-usb-uhci1      piix4-ide         
    dc390               ich9-usb-uhci2      piix4-usb-uhci    
    e1000               ich9-usb-uhci3      pvscsi            
    e1000-82540em       ich9-usb-uhci4      qxl-vga           
    e1000-82544gc       ich9-usb-uhci5      rtl8139           
    e1000-82545em       ich9-usb-uhci6      sdhci-pci         
    edu                 intel-hda           secondary-vga     
    i6300esb            ioh3420             tpci200           
    i82550              lsi53c810           usb-ehci          
    i82551              lsi53c895a          virtio-balloon-pci
    i82557a             megasas             virtio-blk-pci    
    i82557b             megasas-gen2        virtio-net-pci    
    i82557c             ne2k_pci            virtio-rng-pci    
    i82558a             nec-usb-xhci        virtio-scsi-pci   
    i82558b             nvme                virtio-serial-pci 
    i82559a             pci-bridge          vmware-svga       
    i82559b             pci-ohci            vmxnet3           
    i82559c             pci-serial          vt82c686b-usb-uhci
    i82559er            pci-serial-2x       x3130-upstream    
    i82562              pci-serial-4x       xen-pvdevice      
    i82801              pci-testdev         xio3130-downstream
    i82801b11-bridge

Many of them are not currently tested at all.

This test does everything a number of existing tests currently do:

    ac97-test.c e1000-test.c es1370-test.c eepro100-test.c
    ne2000-test.c nvme-test.c pcnet-test.c rtl8139-test.c
    tpci200-test.c virtio-balloon-test.c virtio-rng-test.c
    vmxnet3-test.c

They are all marked "TODO: Replace with functional tests".  Options

* Delete them now, undelete when we add functional tests

* Keep them, blacklist the devices in pci-devs-test.c

* Live with the duplicated testing

Andreas, I guess you got an opinion here.

There's overlap with a few others:

    i82801b11-test.c usb-hcd-ehci-test.c usb-hcd-ohci-test.c
    usb-hcd-xhci-test.c virtio-blk-test.c virtio-net-test.c
    virtio-scsi-test.c virtio-serial-test.c

Options:

* Blacklist the devices in pci-devs-test.c

* Live with the duplicated testing

Andreas?

I manually blacklisted devices unavailable with -device, because QMP
introspection can't tell (pity).

I further blacklisted devices that require a suitable host device to
pass through:

    kvm-pci-assign     vhost-scsi-pci       xen-pci-passthrough
    vfio-pci

I blacklisted virtio-9p-pci and ivshmem for now, because they require
funky backends.

I blacklisted qxl and xen-platform, because they fail the test.

The test runs only for the x86 targets, just like all of
$(check-qtest-pci-y).  It could run on other targets, as long as we
can find a machine type with a PCI bus.

Markus Armbruster (1):
  qtest: Add generic PCI device test

 tests/Makefile        |   2 +
 tests/pci-devs-test.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 301 insertions(+)
 create mode 100644 tests/pci-devs-test.c

-- 
1.9.3

[Qemu-devel] [PATCH RFC 1/1] qtest: Add generic PCI device test

[Markus Armbruster]

Covers only cold plug with -device so far.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile        |   2 +
 tests/pci-devs-test.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 301 insertions(+)
 create mode 100644 tests/pci-devs-test.c

diff --git a/tests/Makefile b/tests/Makefile
index c2e2e52..230238d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -100,6 +100,7 @@ gcov-files-virtio-y += i386-softmmu/hw/char/virtio-serial-bus.c
 check-qtest-virtio-y += $(check-qtest-virtioserial-y)
 gcov-files-virtio-y += $(gcov-files-virtioserial-y)
 
+check-qtest-pci-y += tests/pci-devs-test$(EXESUF)
 check-qtest-pci-y += tests/e1000-test$(EXESUF)
 gcov-files-pci-y += hw/net/e1000.c
 check-qtest-pci-y += tests/rtl8139-test$(EXESUF)
@@ -319,6 +320,7 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
+tests/pci-devs-test$(EXESUF): tests/pci-devs-test.o
 tests/e1000-test$(EXESUF): tests/e1000-test.o
 tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o
 tests/pcnet-test$(EXESUF): tests/pcnet-test.o
diff --git a/tests/pci-devs-test.c b/tests/pci-devs-test.c
new file mode 100644
index 0000000..27cbeee
--- /dev/null
+++ b/tests/pci-devs-test.c
@@ -0,0 +1,299 @@
+/*
+ * Generic PCI device 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 only cold plug with -device so far.  Better than nothing.
+ * Improvements welcome.
+ */
+
+#include "qemu-common.h"
+#include "libqtest.h"
+#include "qapi/qmp/types.h"
+
+typedef struct Prop {
+    const char *name;
+    const char *value;
+} Prop;
+
+typedef struct {
+    Prop prop[16];
+    int nprop;
+    const char *xprops;
+    int nchr;
+    int nblk;
+} TestCase;
+
+static const char *blacklist[] = {
+    /* cannot_instantiate_with_device_add_yet, no way to introspect it */
+    "Bonito",
+    "ICH9 SMB",
+    "ICH9-LPC",
+    "PIIX3",
+    "PIIX3-xen",
+    "PIIX4",
+    "PIIX4_PM",
+    "VT82C686B",
+    "dec-21154",
+    "e500-host-bridge",
+    "grackle",
+    "gt64120_pci",
+    "i440FX",
+    "mch",
+    "pbm-pci",
+    "ppc4xx-host-bridge",
+    "raven",
+    "s390-pcihost",
+    "sh_pci_host",
+    "spapr-pci-host-bridge",
+    "u3-agp",
+    "uni-north-agp",
+    "uni-north-internal-pci",
+    "uni-north-pci",
+    "versatile_pci_host",
+    /* require a suitable host device to pass through */
+    "kvm-pci-assign",
+    "vfio-pci",
+    "vhost-scsi-pci",
+    "xen-pci-passthrough",
+    /* require a backend this test can't setup (yet) */
+    "virtio-9p-pci",            /* requires -fsdev ... */
+    "ivshmem",                  /* requires special chardev */
+    /* test fails */
+    "qxl",                      /* qemu crashes */
+    "xen-platform"              /* qemu crashes */
+};
+
+static struct {
+    const char *name, *propstr;
+} oddball[] = {
+    { "nvme", "serial=foo" },
+    { "xen-pvdevice", "device-id=666" },
+    { "pci-bridge", "chassis_nr=13" },
+};
+
+static bool is_blacklisted(const char *typename)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
+        if (!strcmp(blacklist[i], typename)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static void add_xprops(TestCase *tcase, const char *typename)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(oddball); i++) {
+        if (!strcmp(oddball[i].name, typename)) {
+            tcase->xprops = oddball[i].propstr;
+            break;
+        }
+    }
+}
+
+static void add_prop(TestCase *tcase, const char *name, const char *value)
+{
+    g_assert(tcase->nprop < ARRAY_SIZE(tcase->prop));
+    tcase->prop[tcase->nprop].name = g_strdup(name);
+    tcase->prop[tcase->nprop].value = g_strdup(value);
+    tcase->nprop++;
+}
+
+static char *fmt_props(const Prop prop[], int nprop, const char *xprops)
+{
+    char **strv = g_new(char *, nprop + 2);
+    int i;
+    char *props;
+
+    for (i = 0; i < nprop; i++) {
+        strv[i] = g_strdup_printf("%s=%s", prop[i].name, prop[i].value);
+    }
+    strv[nprop] = (char *)xprops;
+    strv[nprop + 1] = NULL;
+ 
+    props = g_strjoinv(",", strv);
+
+    for (i = 0; i < nprop; i++) {
+        g_free(strv[i]);
+    }
+
+    return props;
+}
+
+static char *fmt_blkid(char *buf, size_t sz, int index)
+{
+    size_t len = snprintf(buf, sz, "blk%d", index);
+    g_assert(len < sz);
+    return buf;
+}
+
+static char *fmt_blockdevs(int nblk)
+{
+    char **strv = g_new(char *, nblk + 1);
+    int i;
+    char buf[64];
+    char *blks;
+
+    for (i = 0; i < nblk; i++) {
+        strv[i] = g_strdup_printf("-drive if=none,id=%s,format=raw,file=/dev/null",
+                                  fmt_blkid(buf, sizeof(buf), i));
+    }
+    strv[nblk] = NULL;
+ 
+    blks = g_strjoinv(" ", strv);
+
+    for (i = 0; i < nblk; i++) {
+        g_free(strv[i]);
+    }
+
+    return blks;
+}
+
+static char *fmt_chrid(char *buf, size_t sz, int index)
+{
+    size_t len = snprintf(buf, sz, "chr%d", index);
+    g_assert(len < sz);
+    return buf;
+}
+
+static char *fmt_chardevs(int nchr)
+{
+    char **strv = g_new(char *, nchr + 1);
+    int i;
+    char buf[64];
+    char *chrs;
+
+    for (i = 0; i < nchr; i++) {
+        strv[i] = g_strdup_printf("-chardev null,id=%s",
+                                  fmt_chrid(buf, sizeof(buf), i));
+    }
+    strv[nchr] = NULL;
+ 
+    chrs = g_strjoinv(" ", strv);
+
+    for (i = 0; i < nchr; i++) {
+        g_free(strv[i]);
+    }
+
+    return chrs;
+}
+
+static void test_device(gconstpointer data)
+{
+    const TestCase *tcase = data;
+    char *blks = fmt_blockdevs(tcase->nblk);
+    char *chrs = fmt_chardevs(tcase->nchr);
+    char *props = fmt_props(tcase->prop, tcase->nprop, tcase->xprops);
+    char *args;
+    QDict *response;
+
+    args = g_strconcat(blks, chrs, " -device '", props, "'", NULL);
+    g_free(props);
+    qtest_start(args);
+    response = qmp("{ 'execute': 'quit' }");
+    g_assert(qdict_haskey(response, "return"));
+    qtest_end();
+    g_free(args);
+}
+
+static void add_backends(TestCase *tcase, const char *typename)
+{
+    QDict *response, *dpinfo;
+    QList *list;
+    const QListEntry *p;
+    QObject *qobj;
+    QString *qstr;
+    const char *name;
+    char buf[64];
+
+    response = qmp("{ 'execute': 'device-list-properties', "
+                   "'arguments': { 'typename': %s }  }", typename);
+    g_assert(response);
+    list = qdict_get_qlist(response, "return");
+    g_assert(list);
+    
+    for (p = qlist_first(list); p; p = qlist_next(p)) {
+        dpinfo = qobject_to_qdict(qlist_entry_obj(p));
+        g_assert(dpinfo);
+        qobj = qdict_get(dpinfo, "name");
+        g_assert(qobj);
+        qstr = qobject_to_qstring(qobj);
+        g_assert(qstr);
+        name = qstring_get_str(qstr);
+
+        if (strstart(name, "chardev", NULL)) {
+            add_prop(tcase, name,
+                     fmt_chrid(buf, sizeof(buf), tcase->nchr++));
+        } else if (strstart(name, "drive", NULL)) {
+            add_prop(tcase, name,
+                     fmt_blkid(buf, sizeof(buf), tcase->nblk++));
+        }
+    }
+
+    QDECREF(response);
+}
+
+static void add_device_test_cases(void)
+{
+    QDict *response, *otinfo;
+    QList *list;
+    const QListEntry *p;
+    QObject *qobj;
+    QString *qstr;
+    const char *name, *path;
+    TestCase *tcase;
+
+    qtest_start("-machine none");
+    response = qmp("{ 'execute': 'qom-list-types', "
+                   "'arguments': { 'implements': 'pci-device' } }");
+    g_assert(response);
+    list = qdict_get_qlist(response, "return");
+    g_assert(list);
+
+    for (p = qlist_first(list); p; p = qlist_next(p)) {
+        otinfo = qobject_to_qdict(qlist_entry_obj(p));
+        g_assert(otinfo);
+        qobj = qdict_get(otinfo, "name");
+        g_assert(qobj);
+        qstr = qobject_to_qstring(qobj);
+        g_assert(qstr);
+        name = qstring_get_str(qstr);
+
+        if (is_blacklisted(name)) {
+            continue;
+        }
+
+        tcase = g_new0(TestCase, 1);
+        add_prop(tcase, "driver", name);
+        add_backends(tcase, name);
+        add_xprops(tcase, name);
+
+        path = g_strdup_printf("/pci-devs/%s", name);
+        g_test_add_data_func(path, tcase, test_device);
+    }
+
+    QDECREF(response);
+    qtest_end();
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    add_device_test_cases();
+
+    return g_test_run();
+}
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 0/1] qtest: Generic PCI device test

[Andreas Färber]

Hi Markus,

Again thanks for digging into this.

Am 29.01.2015 um 15:58 schrieb Markus Armbruster:
> This test does everything a number of existing tests currently do:
> 
>     ac97-test.c e1000-test.c es1370-test.c eepro100-test.c
>     ne2000-test.c nvme-test.c pcnet-test.c rtl8139-test.c
>     tpci200-test.c virtio-balloon-test.c virtio-rng-test.c
>     vmxnet3-test.c
> 
> They are all marked "TODO: Replace with functional tests".  Options
> 
> * Delete them now, undelete when we add functional tests
> 
> * Keep them, blacklist the devices in pci-devs-test.c
> 
> * Live with the duplicated testing
> 
> Andreas, I guess you got an opinion here.

My preference would be to not remove device files. "Functional tests"
refers to doing real device-specific MMIO or PIO, and the intent of
contributing such stubs, beyond the basic -device init/realize testing,
was lowering the hurdle so that maintainers can require bugfixers to
contribute a matching test case where applicable.

> There's overlap with a few others:
> 
>     i82801b11-test.c usb-hcd-ehci-test.c usb-hcd-ohci-test.c
>     usb-hcd-xhci-test.c virtio-blk-test.c virtio-net-test.c
>     virtio-scsi-test.c virtio-serial-test.c
> 
> Options:
> 
> * Blacklist the devices in pci-devs-test.c
> 
> * Live with the duplicated testing
> 
> Andreas?

Not having reviewed the respective test code yet, I would suggest to
keep generic tests in pci-devs-test.c and to rather drop generic tests
from device-specific files (de-duplication).

While I haven't benchmarked it, I assume that the bigger contributor to
qtest runtime is the overhead of spawning qemu-system-* processes rather
than running multiple tests per process. So living with duplication may
be a convenience option.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH RFC 0/1] qtest: Generic PCI device test

[John Snow]

On 01/29/2015 09:58 AM, Markus Armbruster wrote:
> The test uses QMP introspection to find PCI devices, then tries to
> cold-plug each of them.  Could be extended to hot-plug and unplug.
>
> The tests' QMP introspection part is patterned after Andreas's
> qom-test, which uses QMP to find machine types.
>
> The following devices pass the test:
>
>      AC97                ich9-ahci           pcnet
>      ES1370              ich9-intel-hda      piix3-ide
>      VGA                 ich9-usb-ehci1      piix3-ide-xen
>      am53c974            ich9-usb-ehci2      piix3-usb-uhci
>      cirrus-vga          ich9-usb-uhci1      piix4-ide
>      dc390               ich9-usb-uhci2      piix4-usb-uhci
>      e1000               ich9-usb-uhci3      pvscsi
>      e1000-82540em       ich9-usb-uhci4      qxl-vga
>      e1000-82544gc       ich9-usb-uhci5      rtl8139
>      e1000-82545em       ich9-usb-uhci6      sdhci-pci
>      edu                 intel-hda           secondary-vga
>      i6300esb            ioh3420             tpci200
>      i82550              lsi53c810           usb-ehci
>      i82551              lsi53c895a          virtio-balloon-pci
>      i82557a             megasas             virtio-blk-pci
>      i82557b             megasas-gen2        virtio-net-pci
>      i82557c             ne2k_pci            virtio-rng-pci
>      i82558a             nec-usb-xhci        virtio-scsi-pci
>      i82558b             nvme                virtio-serial-pci
>      i82559a             pci-bridge          vmware-svga
>      i82559b             pci-ohci            vmxnet3
>      i82559c             pci-serial          vt82c686b-usb-uhci
>      i82559er            pci-serial-2x       x3130-upstream
>      i82562              pci-serial-4x       xen-pvdevice
>      i82801              pci-testdev         xio3130-downstream
>      i82801b11-bridge
>
> Many of them are not currently tested at all.
>
> This test does everything a number of existing tests currently do:
>
>      ac97-test.c e1000-test.c es1370-test.c eepro100-test.c
>      ne2000-test.c nvme-test.c pcnet-test.c rtl8139-test.c
>      tpci200-test.c virtio-balloon-test.c virtio-rng-test.c
>      vmxnet3-test.c
>
> They are all marked "TODO: Replace with functional tests".  Options
>
> * Delete them now, undelete when we add functional tests
>
> * Keep them, blacklist the devices in pci-devs-test.c
>
> * Live with the duplicated testing
>

I'd be happy with stubbing out some of those tests, so we at least have 
some record and starting place for beginning tests again in the future.

So... Delete the test interface, keep the tests?

Or: Remove these devices from the generic test, but factor the generic 
test code such that these device tests can just invoke the generic 
routines from their own tests.

That way there's no duplication, everybody gets to use the shared code, 
and we keep the record that we'd like to enhance testing of these devices.

> Andreas, I guess you got an opinion here.
>
> There's overlap with a few others:
>
>      i82801b11-test.c usb-hcd-ehci-test.c usb-hcd-ohci-test.c
>      usb-hcd-xhci-test.c virtio-blk-test.c virtio-net-test.c
>      virtio-scsi-test.c virtio-serial-test.c
>
> Options:
>
> * Blacklist the devices in pci-devs-test.c
>
> * Live with the duplicated testing
>

If you factor out the core generic PCI testing as above, you can just 
cull the common parts out of these tests and let them do their own 
functional tests on top, then blacklist them from the generic dispatcher.

> Andreas?
>
> I manually blacklisted devices unavailable with -device, because QMP
> introspection can't tell (pity).
>
> I further blacklisted devices that require a suitable host device to
> pass through:
>
>      kvm-pci-assign     vhost-scsi-pci       xen-pci-passthrough
>      vfio-pci
>
> I blacklisted virtio-9p-pci and ivshmem for now, because they require
> funky backends.
>
> I blacklisted qxl and xen-platform, because they fail the test.
>
> The test runs only for the x86 targets, just like all of
> $(check-qtest-pci-y).  It could run on other targets, as long as we
> can find a machine type with a PCI bus.
>
> Markus Armbruster (1):
>    qtest: Add generic PCI device test
>
>   tests/Makefile        |   2 +
>   tests/pci-devs-test.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 301 insertions(+)
>   create mode 100644 tests/pci-devs-test.c
>

Re: [Qemu-devel] [PATCH RFC 0/1] qtest: Generic PCI device test

[Markus Armbruster]

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

> Hi Markus,
>
> Again thanks for digging into this.
>
> Am 29.01.2015 um 15:58 schrieb Markus Armbruster:
>> This test does everything a number of existing tests currently do:
>> 
>>     ac97-test.c e1000-test.c es1370-test.c eepro100-test.c
>>     ne2000-test.c nvme-test.c pcnet-test.c rtl8139-test.c
>>     tpci200-test.c virtio-balloon-test.c virtio-rng-test.c
>>     vmxnet3-test.c
>> 
>> They are all marked "TODO: Replace with functional tests".  Options
>> 
>> * Delete them now, undelete when we add functional tests
>> 
>> * Keep them, blacklist the devices in pci-devs-test.c
>> 
>> * Live with the duplicated testing
>> 
>> Andreas, I guess you got an opinion here.
>
> My preference would be to not remove device files. "Functional tests"
> refers to doing real device-specific MMIO or PIO, and the intent of
> contributing such stubs, beyond the basic -device init/realize testing,
> was lowering the hurdle so that maintainers can require bugfixers to
> contribute a matching test case where applicable.

Yes, asking people who aren't familiar with device tests to create one
from scratch is asking a lot.  Asking them to copy a skeleton and flesh
it out is much more practical.  Almost as good as fleshing out one of
your stubs, I think.

A non-skeleton test for a really simple PCI device could serve as
skeleton.  wdt_i6300esb.c is a possible candidate.

>> There's overlap with a few others:
>> 
>>     i82801b11-test.c usb-hcd-ehci-test.c usb-hcd-ohci-test.c
>>     usb-hcd-xhci-test.c virtio-blk-test.c virtio-net-test.c
>>     virtio-scsi-test.c virtio-serial-test.c
>> 
>> Options:
>> 
>> * Blacklist the devices in pci-devs-test.c
>> 
>> * Live with the duplicated testing
>> 
>> Andreas?
>
> Not having reviewed the respective test code yet, I would suggest to
> keep generic tests in pci-devs-test.c and to rather drop generic tests
> from device-specific files (de-duplication).

Makes sense to me.  However, some existing device-specific files won't
have any tests left then.  What do you want me to do with them?

> While I haven't benchmarked it, I assume that the bigger contributor to
> qtest runtime is the overhead of spawning qemu-system-* processes rather
> than running multiple tests per process. So living with duplication may
> be a convenience option.

My new test is a pig in that regard: it runs qemu-system-FOO once per
non-blacklisted PCI device.

I could pack tests into fewer runs by using more than just a single PCI
slot.  i440FX can do 30 without a pci-bridge (would probably a bad idea
for this test) and multifunction (certainly a bad idea).  However, when
a test fails, the culprit isn't as obvious then.  Do you want me to try
that?