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

[Li Qiang]

Thomas Huth <address@hidden> 于2019年4月24日周三 下午3:31写道：

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

So I think we should define a deterministic endianness for this.

I will push another revision patchset and using little endian.

Thanks,
Li Qiang


>  Thomas
>