Markus Armbruster <arm...@redhat.com> writes:
> Anthony Liguori <aligu...@us.ibm.com> writes:
>
>> Markus Armbruster <arm...@redhat.com> writes:
>>
>>> Signed-off-by: Markus Armbruster <arm...@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 <arm...@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
>
> [...]
