Re: [Qemu-devel] [PATCH v3 08/16] boot-order-test: New; covering just PC for now

["Andreas Färber" <afaerber@suse.de>]

Am 18.06.2013 15:33, schrieb Markus Armbruster:
> Anthony Liguori <aliguori@us.ibm.com> writes:
> 
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/Makefile          |  2 ++
>>>  tests/boot-order-test.c | 68
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 70 insertions(+)
>>>  create mode 100644 tests/boot-order-test.c
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index c107489..394e029 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -54,6 +54,7 @@ gcov-files-i386-y = hw/fdc.c
>>>  check-qtest-i386-y += tests/ide-test$(EXESUF)
>>>  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>>>  gcov-files-i386-y += hw/hd-geometry.c
>>> +check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>>>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>>>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>>>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
>>> @@ -130,6 +131,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
>>>  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
>>> +tests/boot-order-test$(EXESUF): tests/boot-order-test.o
>>>  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)
>>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>>> new file mode 100644
>>> index 0000000..2215710
>>> --- /dev/null
>>> +++ b/tests/boot-order-test.c
>>> @@ -0,0 +1,68 @@
>>> +/*
>>> + * Boot order test cases.
>>> + *
>>> + * Copyright (c) 2013 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.
>>> + */
>>> +
>>> +#include <glib.h>
>>> +#include "libqtest.h"
>>> +
>>> +static void test_pc_cmos_byte(int reg, int expected)
>>> +{
>>> +    int actual;
>>> +
>>> +    outb(0x70, reg);
>>> +    actual = inb(0x71);
>>> +    g_assert_cmphex(actual, ==, expected);
>>> +}
>>> +
>>> +static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
>>> +{
>>> +    test_pc_cmos_byte(0x38, boot1);
>>> +    test_pc_cmos_byte(0x3d, boot2);
>>> +}
>>> +
>>> +static void test_pc_with_args(const char *test_args,
>>> +                              uint8_t boot1, uint8_t boot2,
>>> +                              uint8_t reboot1, uint8_t reboot2)
>>> +{
>>> +    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
>>> +
>>> +    qtest_start(args);
>>> +    test_pc_cmos(boot1, boot2);
>>> +    qmp("{ 'execute': 'system_reset' }");
>         test_pc_cmos(reboot1, reboot2);
>>
>> I think this races.  I'd suggest doing a tight loop of this test and
>> running it a few thousand times to see if you can catch it.
>>
>> qmp_system_reset() calls qemu_system_reset_requested() which stops all
>> CPUs but let's control fall back to the main loop which actually does
>> the device reset.
>>
>> I think there's a tiny window where this command could return while the
>> reset routines have not been actually called yet.
>>
>> Technically speaking, I think it's necessary to wait for a reset event
>> to know that the device model has been reset.
> 
> Hmm.
> 
> First attempt to "win" this race: tight loop around test_a_boot_order(),
> i.e. the complete test.  Failed because libqtest leaks two file
> descriptors and some memory per iteration.  With that fixed (patch
> coming), I still couldn't make the test fail in >75,000 runs on two
> otherwise pretty much unloaded cores.
> 
> Second attempt: tight loop around just
> 
>     qmp("{ 'execute': 'system_reset' }");
>     actual = read_boot_order();
>     g_assert_cmphex(actual, ==, expected_reboot);
> 
> Still no luck with x86, but "success" with ppc.
> 
> Waiting for the event RESET is safe.  But doing that right involves
> quite some infrastructure work.  All we have now is qtest_qmpv(), which
> sends the command, then reads QMP output character by character until it
> got a complete object.  Normally, that's the QMP command response.  But
> it could be an event.  Racy all by itself, even without my "help" :)
> 
> Oh, and it doesn't know about strings, it just counts curlies.  If the
> output has unmatched curlies in strings...  I wonder how this code ever
> made it past review ;-P
> 
> The proper solution is real QMP support in libqtest.  Unfortunately,
> that's not something I can do right now.

Didn't Michael or Jason have patches improving the qtest QMP support for
migration some months ago? At least they allowed to access the QMP
response in parsed form IIRC.

Andreas

> 
> fdc-test.c uses qmp("") to ignore an expected event.  If I put a similar
> hack into boot-order-test.c, ppc survives >500,000 iterations.  Good
> enough to get this test in?
> 
> [...]
> 


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