Re: [Qemu-devel] [PATCH 3/4] tests: fw_cfg: add reboot_timeout test case

[Thomas Huth]

On 24/04/2019 03.16, Li Qiang wrote:
> 
> Thomas Huth <address@hidden <mailto:address@hidden>> 于2019年4月24日
> 周三 上午12:29写道：
> 
>     On 20/04/2019 12.00, Li Qiang wrote:
>     > Signed-off-by: Li Qiang <address@hidden <mailto:address@hidden>>
>     > ---
>     >  tests/fw_cfg-test.c | 19 +++++++++++++++++++
>     >  1 file changed, 19 insertions(+)
>     >
>     > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
>     > index c22503619f..9f75dbb5f4 100644
>     > --- a/tests/fw_cfg-test.c
>     > +++ b/tests/fw_cfg-test.c
>     > @@ -174,6 +174,24 @@ static void test_fw_cfg_boot_menu(void)
>     >      qtest_quit(s);
>     >  }
>     > 
>     > +static void test_fw_cfg_reboot_timeout(void)
>     > +{
>     > +    QFWCFG *fw_cfg;
>     > +    QTestState *s;
>     > +    uint32_t reboot_timeout = 0;
>     > +    size_t filesize;
>     > +
>     > +    s = qtest_init("-boot reboot-timeout=15");
>     > +    fw_cfg = pc_fw_cfg_init(s);
>     > +
>     > +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
>     > +                     &reboot_timeout, sizeof(reboot_timeout));
>     > +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
>     > +    g_assert_cmpint(reboot_timeout, ==, 15);
> 
>     Is this endianess-safe? Or do you need to byteswap reboot_timeout if the
>     host and guest endianess does not match?
> 
> Good question!
> 
> IIUC, the qemu fw_cfg store the 'file' entry data just in byte stream.
> So there is no need
> to consider the file's endianess. If when the file entry is stored
> without considering endianess,
> it will be ok to ignore the endianess. So for this patch, the
> 'reboot_timeout' is ok as when we stored 
> it we don't consider endianess. But for 'splash-time', when we stored
> it  we convert it to little endian
> so when we fetches it, we should also convert it from little endian to
> cpu-endian.

I just gave these patches a try on a big endian host, and the test fails
indeed:

$ QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/fw_cfg-test
/x86_64/fw_cfg/signature: OK
/x86_64/fw_cfg/id: OK
/x86_64/fw_cfg/uuid: OK
/x86_64/fw_cfg/ram_size: OK
/x86_64/fw_cfg/nographic: OK
/x86_64/fw_cfg/nb_cpus: OK
/x86_64/fw_cfg/max_cpus: OK
/x86_64/fw_cfg/numa: OK
/x86_64/fw_cfg/boot_menu: OK
/x86_64/fw_cfg/reboot_timeout: **
ERROR:tests/fw_cfg-test.c:190:test_fw_cfg_reboot_timeout: assertion
failed (reboot_timeout == 15): (0 == 15)
Aborted

=> it also fails for reboot_timeout already, not only for splash_time.

> So I think we can have following choose:
> 
> 1. Don't consider the endianess, this is ok when emaulated target and
> host target has the same endianess.

Sorry, but that's not a real choice. qtests must also work when you e.g.
run qemu-system-x86_64 on a big endian host.

 Thomas