Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] tests-ppc64: test for -device spapr-pci-host-bridge

[Andreas Färber]

Am 14.02.2014 05:05, schrieb Alexey Kardashevskiy:
> On 02/11/2014 02:48 AM, Andreas Färber wrote:
>> Am 10.02.2014 04:52, schrieb Alexey Kardashevskiy:
>>> This adds a test if SPAPR PHB can be added via the command line.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>> ---
>>>  tests/Makefile         |  2 ++
>>>  tests/spapr-phb-test.c | 28 ++++++++++++++++++++++++++++
>>>  2 files changed, 30 insertions(+)
>>>  create mode 100644 tests/spapr-phb-test.c
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index 3a00ea7..303225e 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -117,6 +117,7 @@ check-qtest-s390x-y += tests/qom-test$(EXESUF)
>>>  check-qtest-unicore32-y += tests/qom-test$(EXESUF)
>>>  check-qtest-xtensa-y += tests/qom-test$(EXESUF)
>>>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>>>  
>>>  check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>>>          comments.json empty.json funny-char.json indented-expr.json \
>>> @@ -203,6 +204,7 @@ libqos-omap-obj-y = $(libqos-obj-y) 
>>> tests/libqos/i2c-omap.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
>>> +tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
>>>  tests/fdc-test$(EXESUF): tests/fdc-test.o
>>>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
>>>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>> diff --git a/tests/spapr-phb-test.c b/tests/spapr-phb-test.c
>>> new file mode 100644
>>> index 0000000..8cad354
>>> --- /dev/null
>>> +++ b/tests/spapr-phb-test.c
>>> @@ -0,0 +1,28 @@
>>> +/*
>>> + * QTest testcase for SPAPR PHB
>>> + *
>>
>> Missing some IBM or aik copyright?
>>
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +#include <glib.h>
>>> +
>>> +#include "libqtest.h"
>>> +
>>> +#define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
>>> +
>>> +static void test_phb_device(void)
>>> +{
>>> +    qtest_start("-device " TYPE_SPAPR_PCI_HOST_BRIDGE ",index=100");
>>> +
>>> +    qtest_end();
>>> +}
>>> +
>>> +int main(int argc, char **argv)
>>> +{
>>> +    g_test_init(&argc, &argv, NULL);
>>> +
>>> +    qtest_add_func("/qmp/phb_device", test_phb_device);
>>> +
>>> +    return g_test_run();
>>> +}
>>> +
>>
>> Trailing white line FWIW.
>>
>> This is looking good, thanks for your work. Apart from the device
>> there's nothing ppc-specific in here, so I might take it through
>> qom-next. One nit here is the test name, which I think should rather be
>> /spapr-phb/device (nothing QMP-specific in there) - I could tweak that
>> for you.
>>
>> Another question is, do you plan to test variations of the device?
>> Otherwise I would suggest to place the qtest_start() and qtest_end()
>> into main(), so that individual future tests can test certain
>> functionalities on the instantiated device rather than repeatedly
>> launching QEMU processes.
> 
> Is this better? It does the job.

Hmm... ;)

> I am not quite sure what qtest_add_func() is for. Is there any good example
> of calling such a function via json or whatever it is? Thanks.

There are lots of other tests/*-test.c files, including the series you
replied to. I've tweaked your original patch so that it outputs:

$ make check-qtest-ppc64 V=1
[...]
TEST: tests/spapr-phb-test... (pid=3080)
  /ppc64/spapr-phb/device:                                            OK
PASS: tests/spapr-phb-test

"/spapr-phb/device" is the name of the unit test supplied to
qtest_add_func(), which then prepends the architecture for uniqueness.
Without calling it, there is no indication that the test was successful.

Individual tests reading/writing registers etc. will need the
qemu-system-ppc64 process running, so g_test_run() needs to go before
qtest_end(). Or if you want to launch it multiple times (e.g. with
different parameters) place it into the test function like originally.

Your test did not pass on qemu.git or qom-next - I picked up the
corresponding fix from ppc-next to reallow -device spapr-pci-host-bridge.

Further the patch was missing a line for gcov to pick up the .o file:

Gcov report for ppc64-softmmu/hw/ppc/spapr_pci.c:
File '/home/andreas/QEMU/qemu/hw/ppc/spapr_pci.c'
Lines executed:42.98% of 342

File '../trace/generated-tracers.h'
Lines executed:0.00% of 12

File '/usr/include/libfdt.h'
Lines executed:100.00% of 5

File '/usr/include/libfdt_env.h'
Lines executed:100.00% of 2

File '/home/andreas/QEMU/qemu/include/hw/pci-host/spapr.h'
Lines executed:0.00% of 2

File '/home/andreas/QEMU/qemu/include/hw/ppc/spapr.h'
Lines executed:22.22% of 9

File '/home/andreas/QEMU/qemu/include/hw/pci/pci.h'
Lines executed:0.00% of 4

File '/home/andreas/QEMU/qemu/include/hw/irq.h'
Lines executed:0.00% of 4

File '/home/andreas/QEMU/qemu/include/qemu/bitops.h'
Lines executed:100.00% of 5

File '/home/andreas/QEMU/qemu/include/qemu/bswap.h'
Lines executed:100.00% of 6

File '/usr/include/bits/byteswap.h'
Lines executed:100.00% of 4


Regards,
Andreas

> diff --git a/tests/spapr-phb-test.c b/tests/spapr-phb-test.c
> new file mode 100644
> index 0000000..11f186c
> --- /dev/null
> +++ b/tests/spapr-phb-test.c
> @@ -0,0 +1,24 @@
> +/*
> + * QTest testcase for SPAPR PHB
> + *
> + * Authors:
> + *  Alexey Kardashevskiy <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include <glib.h>
> +
> +#include "libqtest.h"
> +
> +#define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_start("-device " TYPE_SPAPR_PCI_HOST_BRIDGE ",index=100");
> +    qtest_end();
> +
> +    return g_test_run();
> +}

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg