From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49632) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLNb7-0003GB-Cg for qemu-devel@nongnu.org; Wed, 05 Mar 2014 20:59:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLNb0-0002M5-Jp for qemu-devel@nongnu.org; Wed, 05 Mar 2014 20:59:45 -0500 Received: from mail-pa0-f43.google.com ([209.85.220.43]:47273) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLNb0-0002Lx-Bu for qemu-devel@nongnu.org; Wed, 05 Mar 2014 20:59:38 -0500 Received: by mail-pa0-f43.google.com with SMTP id bj1so1928150pad.16 for ; Wed, 05 Mar 2014 17:59:36 -0800 (PST) Message-ID: <5317D683.3060905@ozlabs.ru> Date: Thu, 06 Mar 2014 12:59:31 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1391947053-26753-1-git-send-email-afaerber@suse.de> <1392004376-17016-1-git-send-email-aik@ozlabs.ru> <1392004376-17016-3-git-send-email-aik@ozlabs.ru> <52F8F4BE.8000108@suse.de> <52FD95F0.6010101@ozlabs.ru> <53178C32.3080103@suse.de> In-Reply-To: <53178C32.3080103@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/2] tests-ppc64: test for -device spapr-pci-host-bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org, Alexander Graf On 03/06/2014 07:42 AM, Andreas Färber wrote: > 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 >>>> --- >>>> 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 >>>> + >>>> +#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. Sure it did not pass, this is why I made the test ;) But I wonder why this fix not gone to upstream immediately - it was breakage. > 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 What did you do to get this output? None from "make check-help" seems to do this. btw it would nice if people updated "make check-help" when they add new tests as without documentation this is all we have to know what tests are there. For example, it was not very obvious how check-tests/qemu-iotests-quick.sh is called and how it works. Or there is some howto and I am just disturbingly blind? Thanks. Also thanks for comments and tweaks but I still do not understand if I still have to do anything regarding these 2 patches. > > 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 >> + * >> + * 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 >> + >> +#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(); >> +} > -- Alexey