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

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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 <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"
>> +
>> +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?

[...]