| Message ID | 20190420100056.116305-4-liq3ea@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_cfg_test refactor and add two test cases | expand |
On 20/04/2019 12.00, Li Qiang wrote: > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > 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? Thomas
Thomas Huth <thuth@redhat.com> 于2019年4月24日周三 上午12:29写道： > On 20/04/2019 12.00, Li Qiang wrote: > > Signed-off-by: Li Qiang <liq3ea@163.com> > > --- > > 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. So this raises another question. Should we consider the endianness for this 'file' entry used for 'integer value'? I read the seabios related code function 'qemu_cfg_read_file' and 'romfile_loadint'. It doesn't consider endianess. This seems we should drop the 'endianess convertion' in 'fw_cfg_bootsplash' when we store 'etc/boot-menu-wait'. However what's if the emulation target has a different endianess for the host target. I think there will be inconsistent. 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. 2. definite an endianess for the 'file' entry used as an integer value. When we store this kind of data we uses the definited endianess and when we fetch it out, we also do an endianess convertion. This will change the seabios I think. Gerd, Philippe What do you think of this? Thanks, Li Qiang > Thomas >
On 24/04/2019 03.16, Li Qiang wrote: > > Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>> 于2019年4月24日 > 周三 上午12:29写道： > > On 20/04/2019 12.00, Li Qiang wrote: > > Signed-off-by: Li Qiang <liq3ea@163.com <mailto:liq3ea@163.com>> > > --- > > 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
Thomas Huth <thuth@redhat.com> 于2019年4月24日周三 下午3:31写道： > On 24/04/2019 03.16, Li Qiang wrote: > > > > Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>> 于2019年4月24日 > > 周三 上午12:29写道： > > > > On 20/04/2019 12.00, Li Qiang wrote: > > > Signed-off-by: Li Qiang <liq3ea@163.com <mailto:liq3ea@163.com>> > > > --- > > > 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 >
Thomas Huth <thuth@redhat.com> 于2019年4月24日周三 下午3:31写道： > On 24/04/2019 03.16, Li Qiang wrote: > > > > Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>> 于2019年4月24日 > > 周三 上午12:29写道： > > > > On 20/04/2019 12.00, Li Qiang wrote: > > > Signed-off-by: Li Qiang <liq3ea@163.com <mailto:liq3ea@163.com>> > > > --- > > > 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: > > Hi Thomas, I have sent out a revision patchset, could you please also try the new in the big endian host. Thanks, Li Qiang > $ 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 >
On Wed, Apr 24, 2019 at 09:16:56AM +0800, Li Qiang wrote: > Thomas Huth <thuth@redhat.com> 于2019年4月24日周三 上午12:29写道： > > > 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. No. Integers are defined to be little endian. See fw_cfg_add_i64() for example, there is an explicit cpu_to_le64() call for that. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> 于2019年4月25日周四 下午4:15写道： > On Wed, Apr 24, 2019 at 09:16:56AM +0800, Li Qiang wrote: > > Thomas Huth <thuth@redhat.com> 于2019年4月24日周三 上午12:29写道： > > > > > 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. > > No. Integers are defined to be little endian. See fw_cfg_add_i64() for > example, there is an explicit cpu_to_le64() call for that. > Yes, for the fw_cfg 'integer' entry it is stored as little endian. But for the fw_cfg 'file' entry interpred as an integer, there is no specify the endianess. Thanks, Li Qiang > cheers, > Gerd > >
On 4/25/19 10:29 AM, Li Qiang wrote: > > > Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>> 于2019年4月 > 25日周四 下午4:15写道： > > On Wed, Apr 24, 2019 at 09:16:56AM +0800, Li Qiang wrote: > > Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>> 于2019年4 > 月24日周三 上午12:29写道： > > > > > 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. > > No. Integers are defined to be little endian. See fw_cfg_add_i64() for > example, there is an explicit cpu_to_le64() call for that. > > > > Yes, for the fw_cfg 'integer' entry it is stored as little endian. > But for the fw_cfg 'file' entry interpred as an integer, there is no > specify the endianess. I agree with Li, the endianess of 'reboot-timeout' is not clear. From docs/specs/fw_cfg.txt: === All Other Data Items === Please consult the QEMU source for the most up-to-date and authoritative list of selector keys and their respective items' purpose, format and writeability. So checking the git history, this code was introduced in commit ac05f3492421, very similar to commit 3d3b8303c6f8 for the 'boot-menu-wait' entry, which explicitely use little-endian, so I think it is safe to consider it little-endian and add a comment about its endianess (referring the previous commits in the patch description). Thanks, Phil. > > Thanks, > Li Qiang > > > > cheers, > Gerd >
On 04/25/19 10:15, Gerd Hoffmann wrote: > On Wed, Apr 24, 2019 at 09:16:56AM +0800, Li Qiang wrote: >> Thomas Huth <thuth@redhat.com> 于2019年4月24日周三 上午12:29写道： >> >>> 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. > > No. Integers are defined to be little endian. See fw_cfg_add_i64() for > example, there is an explicit cpu_to_le64() call for that. Endianness may be defined on a case-by-case basis (i.e. if you introduce a new named fw_cfg file, you can make its contents whatever you like); however, the integer-adding/modifying helper functions do express a preference for endianness. Search "include/hw/nvram/fw_cfg.h" for "endian". Thanks Laszlo
On 04/25/19 12:23, Philippe Mathieu-Daudé wrote: > On 4/25/19 10:29 AM, Li Qiang wrote: >> >> >> Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>> 于2019年4月 >> 25日周四 下午4:15写道： >> >> On Wed, Apr 24, 2019 at 09:16:56AM +0800, Li Qiang wrote: >> > Thomas Huth <thuth@redhat.com <mailto:thuth@redhat.com>> 于2019年4 >> 月24日周三 上午12:29写道： >> > >> > > 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. >> >> No. Integers are defined to be little endian. See fw_cfg_add_i64() for >> example, there is an explicit cpu_to_le64() call for that. >> >> >> >> Yes, for the fw_cfg 'integer' entry it is stored as little endian. >> But for the fw_cfg 'file' entry interpred as an integer, there is no >> specify the endianess. > > I agree with Li, the endianess of 'reboot-timeout' is not clear. > > From docs/specs/fw_cfg.txt: > > === All Other Data Items === > > Please consult the QEMU source for the most up-to-date > and authoritative list of selector keys and their respective > items' purpose, format and writeability. > > So checking the git history, this code was introduced in commit > ac05f3492421, very similar to commit 3d3b8303c6f8 for the > 'boot-menu-wait' entry, which explicitely use little-endian, so I think > it is safe to consider it little-endian and add a comment about its > endianess (referring the previous commits in the patch description). OVMF consumes "boot-menu-wait", so that one must remain LE. OVMF doesn't consume "reboot-timeout", so I can't really comment on it. I guess, if a named fw_cfg file that already exists doesn't explicitly define the endiannesses of its integer fields, err for safety and opt for LE. For new fw_cfg files with integers in them, choose the endianness explicitly and document it. Thanks Laszlo
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); + pc_fw_cfg_uninit(fw_cfg); + qtest_quit(s); +} + int main(int argc, char **argv) { int ret; @@ -195,6 +213,7 @@ int main(int argc, char **argv) qtest_add_func("fw_cfg/max_cpus", test_fw_cfg_max_cpus); qtest_add_func("fw_cfg/numa", test_fw_cfg_numa); qtest_add_func("fw_cfg/boot_menu", test_fw_cfg_boot_menu); + qtest_add_func("fw_cfg/reboot_timeout", test_fw_cfg_reboot_timeout); ret = g_test_run();
Signed-off-by: Li Qiang <liq3ea@163.com> --- tests/fw_cfg-test.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)