[PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Philippe Mathieu-Daudé]

The boot-serial test uses SeaBIOS on HPPA, and expects to read the
"SeaBIOS wants SYSTEM HALT" string, see [*]:

 122 void __VISIBLE __noreturn hlt(void)
 123 {
 124     if (pdc_debug)
 125         printf("HALT initiated from %p\n",  __builtin_return_address(0));
 126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
 127     asm volatile("\t.word 0xfffdead0": : :"memory");
 128     while (1);
 129 }

A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
it as an infinite loop.

If SeaBIOS does not use the expected serial port but another device,
we might poll the console indefinitely while the machine is halted.

Allow the HPPA machine to 'shutdown'. When it does, we'll get
a qtest error:

  $ make check-qtest-hppa
    TEST    check-qtest-hppa: tests/qtest/boot-serial-test
  ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sjxoM6Q'
  ERROR - Bail out! FATAL-ERROR: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sjxoM6Q'
  make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1

[*] https://github.com/qemu/seabios-hppa/blob/1630ac7d65/src/parisc/parisc.c#L138

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
I first used:

    shutdown = !strcmp(test->arch, "hppa");

Then remembered we'll have other HPPA machines soon.

Instead of:

    shutdown = !strcmp(test->machine, "hppa");

I find the check on the SeaBIOS specific string more accurate
(since someone might add some assembly lines as the other tests).
---
 tests/qtest/boot-serial-test.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 05c7f44457..4e92f292f5 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -188,6 +188,17 @@ static void test_machine(const void *data)
     const uint8_t *code = NULL;
     QTestState *qts;
     int ser_fd;
+    bool shutdown;
+
+    /*
+     * This test uses SeaBIOS on HPPA, and expects to read the
+     * "SeaBIOS wants SYSTEM HALT" string. A 'SYSTEM HALT' really
+     * halts the CPU. If SeaBIOS does not use the expected serial
+     * port but another device, we might poll the console
+     * indefinitely while the machine is halted.
+     * Keep using this option for all the other tests.
+     */
+    shutdown = !strcmp(test->expect, "SeaBIOS wants SYSTEM HALT");
 
     ser_fd = mkstemp(serialtmp);
     g_assert(ser_fd != -1);
@@ -215,10 +226,11 @@ static void test_machine(const void *data)
      * Make sure that this test uses tcg if available: It is used as a
      * fast-enough smoketest for that.
      */
-    qts = qtest_initf("%s %s -M %s -no-shutdown "
+    qts = qtest_initf("%s %s -M %s %s "
                       "-chardev file,id=serial0,path=%s "
                       "-serial chardev:serial0 -accel tcg -accel kvm %s",
                       codeparam, code ? codetmp : "", test->machine,
+                      shutdown ? "" : "-no-shutdown",
                       serialtmp, test->extra);
     if (code) {
         unlink(codetmp);
-- 
2.21.1

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Richard Henderson]

On 1/22/20 2:36 PM, Philippe Mathieu-Daudé wrote:
> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
> "SeaBIOS wants SYSTEM HALT" string, see [*]:
> 
>  122 void __VISIBLE __noreturn hlt(void)
>  123 {
>  124     if (pdc_debug)
>  125         printf("HALT initiated from %p\n",  __builtin_return_address(0));
>  126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>  127     asm volatile("\t.word 0xfffdead0": : :"memory");
>  128     while (1);
>  129 }
> 
> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
> it as an infinite loop.

Well, SeaBIOS implements it as the magic QEMU halt instruction,

# These are artificial instructions used by QEMU firmware.
# They are allocated from the unassigned instruction space.
halt            1111 1111 1111 1101 1110 1010 1101 0000
reset           1111 1111 1111 1101 1110 1010 1101 0001

followed by an infinite loop, probably to avoid a compiler warning and Just In
Case.  We really should halt here, unless shutdown is disabled.

> -    qts = qtest_initf("%s %s -M %s -no-shutdown "
> +    qts = qtest_initf("%s %s -M %s %s "
>                        "-chardev file,id=serial0,path=%s "
>                        "-serial chardev:serial0 -accel tcg -accel kvm %s",
>                        codeparam, code ? codetmp : "", test->machine,
> +                      shutdown ? "" : "-no-shutdown",
>                        serialtmp, test->extra);

And thus avoiding the -no-shutdown should in fact shutdown.

Are you saying this doesn't happen, or what?  I think I got lost with the rest
of the message...


r~

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Thomas Huth]

On 23/01/2020 01.36, Philippe Mathieu-Daudé wrote:
> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
> "SeaBIOS wants SYSTEM HALT" string, see [*]:
> 
>  122 void __VISIBLE __noreturn hlt(void)
>  123 {
>  124     if (pdc_debug)
>  125         printf("HALT initiated from %p\n",  __builtin_return_address(0));
>  126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>  127     asm volatile("\t.word 0xfffdead0": : :"memory");
>  128     while (1);
>  129 }
> 
> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
> it as an infinite loop.
> 
> If SeaBIOS does not use the expected serial port but another device,
> we might poll the console indefinitely while the machine is halted.
> 
> Allow the HPPA machine to 'shutdown'. When it does, we'll get
> a qtest error:
> 
>   $ make check-qtest-hppa
>     TEST    check-qtest-hppa: tests/qtest/boot-serial-test
>   ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>   ERROR - Bail out! FATAL-ERROR: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>   make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1

The tests are run with -no-shutdown. Why does qemu exit in that case?
Sounds like a bug in another place, and not in the boot-serial-test.

 Thomas

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Philippe Mathieu-Daudé]

On 1/23/20 5:39 AM, Thomas Huth wrote:
> On 23/01/2020 01.36, Philippe Mathieu-Daudé wrote:
>> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
>> "SeaBIOS wants SYSTEM HALT" string, see [*]:
>>
>>   122 void __VISIBLE __noreturn hlt(void)
>>   123 {
>>   124     if (pdc_debug)
>>   125         printf("HALT initiated from %p\n",  __builtin_return_address(0));
>>   126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>>   127     asm volatile("\t.word 0xfffdead0": : :"memory");
>>   128     while (1);
>>   129 }
>>
>> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
>> it as an infinite loop.
>>
>> If SeaBIOS does not use the expected serial port but another device,
>> we might poll the console indefinitely while the machine is halted.
>>
>> Allow the HPPA machine to 'shutdown'. When it does, we'll get
>> a qtest error:
>>
>>    $ make check-qtest-hppa
>>      TEST    check-qtest-hppa: tests/qtest/boot-serial-test
>>    ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>>    ERROR - Bail out! FATAL-ERROR: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>>    make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1
> 
> The tests are run with -no-shutdown. Why does qemu exit in that case?

Because the HPPA firmware called HALT.

> Sounds like a bug in another place, and not in the boot-serial-test.

Yes, the bug is elsewhere, but with the bug the boot-serial-test hangs 
forever.... No output on the console, qtest waiting indefinitely.

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Philippe Mathieu-Daudé]

On 1/23/20 4:27 AM, Richard Henderson wrote:
> On 1/22/20 2:36 PM, Philippe Mathieu-Daudé wrote:
>> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
>> "SeaBIOS wants SYSTEM HALT" string, see [*]:
>>
>>   122 void __VISIBLE __noreturn hlt(void)
>>   123 {
>>   124     if (pdc_debug)
>>   125         printf("HALT initiated from %p\n",  __builtin_return_address(0));
>>   126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>>   127     asm volatile("\t.word 0xfffdead0": : :"memory");
>>   128     while (1);
>>   129 }
>>
>> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
>> it as an infinite loop.
> 
> Well, SeaBIOS implements it as the magic QEMU halt instruction,
> 
> # These are artificial instructions used by QEMU firmware.
> # They are allocated from the unassigned instruction space.
> halt            1111 1111 1111 1101 1110 1010 1101 0000
> reset           1111 1111 1111 1101 1110 1010 1101 0001
> 
> followed by an infinite loop, probably to avoid a compiler warning and Just In
> Case.  We really should halt here, unless shutdown is disabled.
> 
>> -    qts = qtest_initf("%s %s -M %s -no-shutdown "
>> +    qts = qtest_initf("%s %s -M %s %s "
>>                         "-chardev file,id=serial0,path=%s "
>>                         "-serial chardev:serial0 -accel tcg -accel kvm %s",
>>                         codeparam, code ? codetmp : "", test->machine,
>> +                      shutdown ? "" : "-no-shutdown",
>>                         serialtmp, test->extra);
> 
> And thus avoiding the -no-shutdown should in fact shutdown.
> 
> Are you saying this doesn't happen, or what?  I think I got lost with the rest
> of the message...

"If the command-line option "-no-shutdown" has been specified,
  qemu will not exit, and a STOP event will eventually follow
  the SHUTDOWN event"

What I see is the HPPA firmware calling HALT, then QEMU doesn't exit.

The QTest keeps reading the console for "SeaBIOS wants SYSTEM HALT.".
When this string is displayed on the graphic output instead of the 
serial, the QTest keep reading indefinitely, and the test never 
succeeds/fails, it is just stuck.

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Philippe Mathieu-Daudé]

On 1/23/20 7:29 PM, Philippe Mathieu-Daudé wrote:
> On 1/23/20 5:39 AM, Thomas Huth wrote:
>> On 23/01/2020 01.36, Philippe Mathieu-Daudé wrote:
>>> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
>>> "SeaBIOS wants SYSTEM HALT" string, see [*]:
>>>
>>>   122 void __VISIBLE __noreturn hlt(void)
>>>   123 {
>>>   124     if (pdc_debug)
>>>   125         printf("HALT initiated from %p\n",  
>>> __builtin_return_address(0));
>>>   126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>>>   127     asm volatile("\t.word 0xfffdead0": : :"memory");
>>>   128     while (1);
>>>   129 }
>>>
>>> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
>>> it as an infinite loop.
>>>
>>> If SeaBIOS does not use the expected serial port but another device,
>>> we might poll the console indefinitely while the machine is halted.
>>>
>>> Allow the HPPA machine to 'shutdown'. When it does, we'll get
>>> a qtest error:
>>>
>>>    $ make check-qtest-hppa
>>>      TEST    check-qtest-hppa: tests/qtest/boot-serial-test
>>>    ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604: 
>>> Failed to find expected string. Please check 
>>> '/tmp/qtest-boot-serial-sjxoM6Q'
>>>    ERROR - Bail out! FATAL-ERROR: Failed to find expected string. 
>>> Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>>>    make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1
>>
>> The tests are run with -no-shutdown. Why does qemu exit in that case?
> 
> Because the HPPA firmware called HALT.
> 
>> Sounds like a bug in another place, and not in the boot-serial-test.
> 
> Yes, the bug is elsewhere, but with the bug the boot-serial-test hangs 
> forever.... No output on the console, qtest waiting indefinitely.

Richard explained me on IRC what you probably meant, which was not 
obvious to me at first.

Now I found in check_guest_output():

     /* Poll serial output... */
     while (1) {
         ...
         /* Wait at most 360 seconds.  */
         now = time(NULL);
         if (now - start >= 360) {
             break;
         }
         g_usleep(10000);
     }

$ QTEST_QEMU_BINARY=hppa-softmmu/qemu-system-hppa \
   time tests/qtest/boot-serial-test -k
/hppa/boot-serial/hppa:
** (tests/qtest/boot-serial-test:18604): ERROR **: 22:33:25.010: Failed 
to find expected string. Please check '/tmp/qtest-boot-serial-sZq7ljM'
Command terminated by signal 5
0.31user 0.66system 6:00.07elapsed 0%CPU

Indeed the test fails after 6min, I guess I didn't expect that much 
while testing interactively.

So please disregard this patch.

Thanks,

Phil.

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Thomas Huth]

On 23/01/2020 22.37, Philippe Mathieu-Daudé wrote:
> On 1/23/20 7:29 PM, Philippe Mathieu-Daudé wrote:
>> On 1/23/20 5:39 AM, Thomas Huth wrote:
>>> On 23/01/2020 01.36, Philippe Mathieu-Daudé wrote:
>>>> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
>>>> "SeaBIOS wants SYSTEM HALT" string, see [*]:
>>>>
>>>>   122 void __VISIBLE __noreturn hlt(void)
>>>>   123 {
>>>>   124     if (pdc_debug)
>>>>   125         printf("HALT initiated from %p\n", 
>>>> __builtin_return_address(0));
>>>>   126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>>>>   127     asm volatile("\t.word 0xfffdead0": : :"memory");
>>>>   128     while (1);
>>>>   129 }
>>>>
>>>> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
>>>> it as an infinite loop.
>>>>
>>>> If SeaBIOS does not use the expected serial port but another device,
>>>> we might poll the console indefinitely while the machine is halted.
>>>>
>>>> Allow the HPPA machine to 'shutdown'. When it does, we'll get
>>>> a qtest error:
>>>>
>>>>    $ make check-qtest-hppa
>>>>      TEST    check-qtest-hppa: tests/qtest/boot-serial-test
>>>>    ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604:
>>>> Failed to find expected string. Please check
>>>> '/tmp/qtest-boot-serial-sjxoM6Q'
>>>>    ERROR - Bail out! FATAL-ERROR: Failed to find expected string.
>>>> Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>>>>    make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1
>>>
>>> The tests are run with -no-shutdown. Why does qemu exit in that case?
>>
>> Because the HPPA firmware called HALT.
>>
>>> Sounds like a bug in another place, and not in the boot-serial-test.
>>
>> Yes, the bug is elsewhere, but with the bug the boot-serial-test hangs
>> forever.... No output on the console, qtest waiting indefinitely.
> 
> Richard explained me on IRC what you probably meant, which was not
> obvious to me at first.

I think I also did not really understand what you tried to do here ;-)

The -no-shutdown is required, too, otherwise you could get a race
between the test reading the serial output and the termination of QEMU,
see commit 7150d34a1d60851d73d6ab6783b12b1d25e68f86, and I think we need
that for HPPA, too.

> Now I found in check_guest_output():
> 
>     /* Poll serial output... */
>     while (1) {
>         ...
>         /* Wait at most 360 seconds.  */
>         now = time(NULL);
>         if (now - start >= 360) {
>             break;
>         }
>         g_usleep(10000);
>     }
> 
> $ QTEST_QEMU_BINARY=hppa-softmmu/qemu-system-hppa \
>   time tests/qtest/boot-serial-test -k
> /hppa/boot-serial/hppa:
> ** (tests/qtest/boot-serial-test:18604): ERROR **: 22:33:25.010: Failed
> to find expected string. Please check '/tmp/qtest-boot-serial-sZq7ljM'
> Command terminated by signal 5
> 0.31user 0.66system 6:00.07elapsed 0%CPU
> 
> Indeed the test fails after 6min, I guess I didn't expect that much
> while testing interactively.

Yeah, the huge timeout is ugly, but it is required for very slow hosts,
see commit 627fce617868df87b3757375a2a0318ad2beb381.

So if you want to "fix" your problem, I think you could maybe add a
check for the QEMU events here instead. Or add a check to see whether
the registers of the guest change between iterations to make sure that
the guest is still alive (that way you also handle the case that the
guest crashed and loops forever in a "branch self" instruction). Or
simply continue with the 360s timeout, it's long, but it should trigger
only in case of other bugs, so it's maybe not worth the effort to add
more logic here.

 Thomas

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Philippe Mathieu-Daudé]

On 1/24/20 8:02 AM, Thomas Huth wrote:
> On 23/01/2020 22.37, Philippe Mathieu-Daudé wrote:
>> On 1/23/20 7:29 PM, Philippe Mathieu-Daudé wrote:
>>> On 1/23/20 5:39 AM, Thomas Huth wrote:
>>>> On 23/01/2020 01.36, Philippe Mathieu-Daudé wrote:
>>>>> The boot-serial test uses SeaBIOS on HPPA, and expects to read the
>>>>> "SeaBIOS wants SYSTEM HALT" string, see [*]:
>>>>>
>>>>>    122 void __VISIBLE __noreturn hlt(void)
>>>>>    123 {
>>>>>    124     if (pdc_debug)
>>>>>    125         printf("HALT initiated from %p\n",
>>>>> __builtin_return_address(0));
>>>>>    126     printf("SeaBIOS wants SYSTEM HALT.\n\n");
>>>>>    127     asm volatile("\t.word 0xfffdead0": : :"memory");
>>>>>    128     while (1);
>>>>>    129 }
>>>>>
>>>>> A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
>>>>> it as an infinite loop.
>>>>>
>>>>> If SeaBIOS does not use the expected serial port but another device,
>>>>> we might poll the console indefinitely while the machine is halted.
>>>>>
>>>>> Allow the HPPA machine to 'shutdown'. When it does, we'll get
>>>>> a qtest error:
>>>>>
>>>>>     $ make check-qtest-hppa
>>>>>       TEST    check-qtest-hppa: tests/qtest/boot-serial-test
>>>>>     ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604:
>>>>> Failed to find expected string. Please check
>>>>> '/tmp/qtest-boot-serial-sjxoM6Q'
>>>>>     ERROR - Bail out! FATAL-ERROR: Failed to find expected string.
>>>>> Please check '/tmp/qtest-boot-serial-sjxoM6Q'
>>>>>     make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1
>>>>
>>>> The tests are run with -no-shutdown. Why does qemu exit in that case?
>>>
>>> Because the HPPA firmware called HALT.
>>>
>>>> Sounds like a bug in another place, and not in the boot-serial-test.
>>>
>>> Yes, the bug is elsewhere, but with the bug the boot-serial-test hangs
>>> forever.... No output on the console, qtest waiting indefinitely.
>>
>> Richard explained me on IRC what you probably meant, which was not
>> obvious to me at first.
> 
> I think I also did not really understand what you tried to do here ;-)
> 
> The -no-shutdown is required, too, otherwise you could get a race
> between the test reading the serial output and the termination of QEMU,
> see commit 7150d34a1d60851d73d6ab6783b12b1d25e68f86, and I think we need
> that for HPPA, too.
> 
>> Now I found in check_guest_output():
>>
>>      /* Poll serial output... */
>>      while (1) {
>>          ...
>>          /* Wait at most 360 seconds.  */
>>          now = time(NULL);
>>          if (now - start >= 360) {
>>              break;
>>          }
>>          g_usleep(10000);
>>      }
>>
>> $ QTEST_QEMU_BINARY=hppa-softmmu/qemu-system-hppa \
>>    time tests/qtest/boot-serial-test -k
>> /hppa/boot-serial/hppa:
>> ** (tests/qtest/boot-serial-test:18604): ERROR **: 22:33:25.010: Failed
>> to find expected string. Please check '/tmp/qtest-boot-serial-sZq7ljM'
>> Command terminated by signal 5
>> 0.31user 0.66system 6:00.07elapsed 0%CPU
>>
>> Indeed the test fails after 6min, I guess I didn't expect that much
>> while testing interactively.
> 
> Yeah, the huge timeout is ugly, but it is required for very slow hosts,
> see commit 627fce617868df87b3757375a2a0318ad2beb381.
> 
> So if you want to "fix" your problem, I think you could maybe add a
> check for the QEMU events here instead. Or add a check to see whether
> the registers of the guest change between iterations to make sure that
> the guest is still alive (that way you also handle the case that the
> guest crashed and loops forever in a "branch self" instruction). Or
> simply continue with the 360s timeout, it's long, but it should trigger
> only in case of other bugs, so it's maybe not worth the effort to add
> more logic here.

Good idea. With QMP it is easy:

{ "execute": "query-status" }
{
     "return": {
         "status": "shutdown",
         "singlestep": false,
         "running": false
     }
}