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

[Anthony Liguori <aliguori@us.ibm.com>]

Markus Armbruster <armbru@redhat.com> writes:

> 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.
>
> 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?

With a very big comment stating what's happening.

Regards,

Anthony Liguori

>
> [...]