Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Philippe Mathieu-Daudé 于2019年5月21日周二 上午5:29写道： > Hi Li, > > On 5/17/19 4:28 AM, Li Qiang wrote: > > Ping. > > > > Li Qiang mailto:liq...@gmail.com>> 于2019年5月9日周四 > > 下午5:57写道： > > > > Ping this serials. > > I apologize I hold this series for too long. > With your v1 I wanted to clarify the commit descriptions without asking > you to send a v2, then I reword your patches and the same day you sent > your v2, then I had mixed feeling about how to do to not frustrate you > asking to respin again, but I ended it worst :( > Hi Philippe, not afraid to frustrate me next time, just send out the review email. I don't mind to make revisions to improve the patches. > I adapted the descriptions on your v2 and will repost as v3, then merge > if you are OK with v3. > > I have no objection for this, just merge it. Thanks, Li Qiang > Regards, > > Phil. > > > > > Thanks, > > Li Qiang > > > > Li Qiang mailto:liq...@163.com>> 于2019年4月24日周 > > 三 下午10:07写道： > > > > In the disscuss of adding reboot timeout test case: > > > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > > > Philippe suggested we should uses the only related option for one > > specific test. However currently we uses one QTestState for all > the > > test cases. In order to achieve Philippe's idea, I split the > > test case > > for its own QTestState. As this patchset has changed a lot, I > > don't bump > > the version. > > > > Change since v1: > > Add a patch to store the reboot_timeout as little endian > > Fix the endian issue per Thomas's review > > > > Li Qiang (5): > > tests: refactor fw_cfg_test > > tests: fw_cfg: add a function to get the fw_cfg file > > fw_cfg: reboot: store reboot-timeout as little endian > > tests: fw_cfg: add reboot_timeout test case > > tests: fw_cfg: add splash time test case > > > > hw/nvram/fw_cfg.c | 4 +- > > tests/fw_cfg-test.c | 125 > > +++--- > > tests/libqos/fw_cfg.c | 55 +++ > > tests/libqos/fw_cfg.h | 9 +++ > > 4 files changed, 184 insertions(+), 9 deletions(-) > > > > -- > > 2.17.1 > > > > >
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Hi Li, On 5/17/19 4:28 AM, Li Qiang wrote: > Ping. > > Li Qiang mailto:liq...@gmail.com>> 于2019年5月9日周四 > 下午5:57写道： > > Ping this serials. I apologize I hold this series for too long. With your v1 I wanted to clarify the commit descriptions without asking you to send a v2, then I reword your patches and the same day you sent your v2, then I had mixed feeling about how to do to not frustrate you asking to respin again, but I ended it worst :( I adapted the descriptions on your v2 and will repost as v3, then merge if you are OK with v3. Regards, Phil. > > Thanks, > Li Qiang > > Li Qiang mailto:liq...@163.com>> 于2019年4月24日周 > 三 下午10:07写道： > > In the disscuss of adding reboot timeout test case: > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > Philippe suggested we should uses the only related option for one > specific test. However currently we uses one QTestState for all the > test cases. In order to achieve Philippe's idea, I split the > test case > for its own QTestState. As this patchset has changed a lot, I > don't bump > the version. > > Change since v1: > Add a patch to store the reboot_timeout as little endian > Fix the endian issue per Thomas's review > > Li Qiang (5): > tests: refactor fw_cfg_test > tests: fw_cfg: add a function to get the fw_cfg file > fw_cfg: reboot: store reboot-timeout as little endian > tests: fw_cfg: add reboot_timeout test case > tests: fw_cfg: add splash time test case > > hw/nvram/fw_cfg.c | 4 +- > tests/fw_cfg-test.c | 125 > +++--- > tests/libqos/fw_cfg.c | 55 +++ > tests/libqos/fw_cfg.h | 9 +++ > 4 files changed, 184 insertions(+), 9 deletions(-) > > -- > 2.17.1 > >
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Ping. Li Qiang 于2019年5月9日周四 下午5:57写道： > Ping this serials. > > Thanks, > Li Qiang > > Li Qiang 于2019年4月24日周三 下午10:07写道： > >> In the disscuss of adding reboot timeout test case: >> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html >> >> Philippe suggested we should uses the only related option for one >> specific test. However currently we uses one QTestState for all the >> test cases. In order to achieve Philippe's idea, I split the test case >> for its own QTestState. As this patchset has changed a lot, I don't bump >> the version. >> >> Change since v1: >> Add a patch to store the reboot_timeout as little endian >> Fix the endian issue per Thomas's review >> >> Li Qiang (5): >> tests: refactor fw_cfg_test >> tests: fw_cfg: add a function to get the fw_cfg file >> fw_cfg: reboot: store reboot-timeout as little endian >> tests: fw_cfg: add reboot_timeout test case >> tests: fw_cfg: add splash time test case >> >> hw/nvram/fw_cfg.c | 4 +- >> tests/fw_cfg-test.c | 125 +++--- >> tests/libqos/fw_cfg.c | 55 +++ >> tests/libqos/fw_cfg.h | 9 +++ >> 4 files changed, 184 insertions(+), 9 deletions(-) >> >> -- >> 2.17.1 >> >> >>
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Ping this serials. Thanks, Li Qiang Li Qiang 于2019年4月24日周三 下午10:07写道： > In the disscuss of adding reboot timeout test case: > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > Philippe suggested we should uses the only related option for one > specific test. However currently we uses one QTestState for all the > test cases. In order to achieve Philippe's idea, I split the test case > for its own QTestState. As this patchset has changed a lot, I don't bump > the version. > > Change since v1: > Add a patch to store the reboot_timeout as little endian > Fix the endian issue per Thomas's review > > Li Qiang (5): > tests: refactor fw_cfg_test > tests: fw_cfg: add a function to get the fw_cfg file > fw_cfg: reboot: store reboot-timeout as little endian > tests: fw_cfg: add reboot_timeout test case > tests: fw_cfg: add splash time test case > > hw/nvram/fw_cfg.c | 4 +- > tests/fw_cfg-test.c | 125 +++--- > tests/libqos/fw_cfg.c | 55 +++ > tests/libqos/fw_cfg.h | 9 +++ > 4 files changed, 184 insertions(+), 9 deletions(-) > > -- > 2.17.1 > > >
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Thomas Huth 于2019年4月29日周一 下午9:18写道： > On 29/04/2019 07.09, Li Qiang wrote: > > > > > > Li Qiang mailto:liq...@gmail.com>> 于2019年4月25日周 > > 四 下午10:29写道： > > > > > > > > Thomas Huth mailto:th...@redhat.com>> 于2019年4月 > > 25日周四 下午5:57写道： > > > > On 24/04/2019 16.06, Li Qiang wrote: > > > In the disscuss of adding reboot timeout test case: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > > > > > Philippe suggested we should uses the only related option for > one > > > specific test. However currently we uses one QTestState for > > all the > > > test cases. In order to achieve Philippe's idea, I split the > > test case > > > for its own QTestState. As this patchset has changed a lot, I > > don't bump > > > the version. > > > > > > Change since v1: > > > Add a patch to store the reboot_timeout as little endian > > > Fix the endian issue per Thomas's review > > > > The test still aborts on a big endian host: > > > > $ 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:/home/thuth/devel/qemu/tests/fw_cfg-test.c:190:test_fw_cfg_reboot_timeout: > > assertion failed (reboot_timeout == 15): (251658240 == 15) > > Aborted > > > > 251658240 is 0x0F00, i.e. a byte-swapped 0xf = 15 ... i.e. > > you still > > got an endianess issue somewhere in the code. > > > > > > > > H, > > > > I have thought a long time, still can't point where is wrong. > > > > Let's from the result: > > 0x0f00 in the big endian laid as this: > > low ---> high > > 0x0f 00 00 00 > > > > As I have swapped before the compare so it is read as this: > > low ---> high > > 00 00 00 0x0f > > > > However from the store side: > > the 15 in big endian is: > > low ---> high > > 00 00 00 0x0f > > > > But Before I store it, I convert it to little endian, so following > > should be stored: > > low ---> high > > 0x0f 00 00 00 > > > > Do you apply the patch 3 and recompile the qemu binary? > > > > > > > > Hello Thomas, > > I have tested again this and just store it as big endian(so that the > > store/load has different endianness), > > I don't see any error. > > Uh, now this is embarrassing... I just tried again to see whether I > could find the issue, but now the test passes without problems! > I guess I simply only did a "make tests/fw_cfg-test" before and forgot > to recompile qemu itself. Big sorry for this! > > Anyway, patch series works fine for me, so for the series: > > Tested-by: Thomas Huth > > OK, Thanks Thomas. Philippe maybe you can take a look at this series and merge it. Thanks, Li Qiang > > Also, can we add these test sceneries(big-endian host) in our CI? so > > that the bot can report for every commit. > > Patchew only runs on x86, but Peter has some big endian hosts for his > acceptance tests - so problems should be found during PULL requests at > least. > > Thomas >
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
On 29/04/2019 07.09, Li Qiang wrote: > > > Li Qiang mailto:liq...@gmail.com>> 于2019年4月25日周 > 四 下午10:29写道： > > > > Thomas Huth mailto:th...@redhat.com>> 于2019年4月 > 25日周四 下午5:57写道： > > On 24/04/2019 16.06, Li Qiang wrote: > > In the disscuss of adding reboot timeout test case: > > > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > > > Philippe suggested we should uses the only related option for one > > specific test. However currently we uses one QTestState for > all the > > test cases. In order to achieve Philippe's idea, I split the > test case > > for its own QTestState. As this patchset has changed a lot, I > don't bump > > the version. > > > > Change since v1: > > Add a patch to store the reboot_timeout as little endian > > Fix the endian issue per Thomas's review > > The test still aborts on a big endian host: > > $ 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:/home/thuth/devel/qemu/tests/fw_cfg-test.c:190:test_fw_cfg_reboot_timeout: > assertion failed (reboot_timeout == 15): (251658240 == 15) > Aborted > > 251658240 is 0x0F00, i.e. a byte-swapped 0xf = 15 ... i.e. > you still > got an endianess issue somewhere in the code. > > > > H, > > I have thought a long time, still can't point where is wrong. > > Let's from the result: > 0x0f00 in the big endian laid as this: > low ---> high > 0x0f 00 00 00 > > As I have swapped before the compare so it is read as this: > low ---> high > 00 00 00 0x0f > > However from the store side: > the 15 in big endian is: > low ---> high > 00 00 00 0x0f > > But Before I store it, I convert it to little endian, so following > should be stored: > low ---> high > 0x0f 00 00 00 > > Do you apply the patch 3 and recompile the qemu binary? > > > > Hello Thomas, > I have tested again this and just store it as big endian(so that the > store/load has different endianness), > I don't see any error. Uh, now this is embarrassing... I just tried again to see whether I could find the issue, but now the test passes without problems! I guess I simply only did a "make tests/fw_cfg-test" before and forgot to recompile qemu itself. Big sorry for this! Anyway, patch series works fine for me, so for the series: Tested-by: Thomas Huth > Also, can we add these test sceneries(big-endian host) in our CI? so > that the bot can report for every commit. Patchew only runs on x86, but Peter has some big endian hosts for his acceptance tests - so problems should be found during PULL requests at least. Thomas
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Li Qiang 于2019年4月25日周四 下午10:29写道： > > > Thomas Huth 于2019年4月25日周四 下午5:57写道： > >> On 24/04/2019 16.06, Li Qiang wrote: >> > In the disscuss of adding reboot timeout test case: >> > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html >> > >> > Philippe suggested we should uses the only related option for one >> > specific test. However currently we uses one QTestState for all the >> > test cases. In order to achieve Philippe's idea, I split the test case >> > for its own QTestState. As this patchset has changed a lot, I don't bump >> > the version. >> > >> > Change since v1: >> > Add a patch to store the reboot_timeout as little endian >> > Fix the endian issue per Thomas's review >> >> The test still aborts on a big endian host: >> >> $ 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:/home/thuth/devel/qemu/tests/fw_cfg-test.c:190:test_fw_cfg_reboot_timeout: >> assertion failed (reboot_timeout == 15): (251658240 == 15) >> Aborted >> >> 251658240 is 0x0F00, i.e. a byte-swapped 0xf = 15 ... i.e. you still >> got an endianess issue somewhere in the code. >> > > > H, > > I have thought a long time, still can't point where is wrong. > > Let's from the result: > 0x0f00 in the big endian laid as this: > low ---> high > 0x0f 00 00 00 > > As I have swapped before the compare so it is read as this: > low ---> high > 00 00 00 0x0f > > However from the store side: > the 15 in big endian is: > low ---> high > 00 00 00 0x0f > > But Before I store it, I convert it to little endian, so following should > be stored: > low ---> high > 0x0f 00 00 00 > > Do you apply the patch 3 and recompile the qemu binary? > Hello Thomas, I have tested again this and just store it as big endian(so that the store/load has different endianness), I don't see any error. Also, can we add these test sceneries(big-endian host) in our CI? so that the bot can report for every commit. Thanks, Li Qiang If it is, I may need your help as I have no big endian host device. > > You can debug and inspect the memory layout and point out where is wrong. > > Thanks, > Li Qiang > > > > > > > >> >> Thomas >> >
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
Thomas Huth 于2019年4月25日周四 下午5:57写道： > On 24/04/2019 16.06, Li Qiang wrote: > > In the disscuss of adding reboot timeout test case: > > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > > > Philippe suggested we should uses the only related option for one > > specific test. However currently we uses one QTestState for all the > > test cases. In order to achieve Philippe's idea, I split the test case > > for its own QTestState. As this patchset has changed a lot, I don't bump > > the version. > > > > Change since v1: > > Add a patch to store the reboot_timeout as little endian > > Fix the endian issue per Thomas's review > > The test still aborts on a big endian host: > > $ 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:/home/thuth/devel/qemu/tests/fw_cfg-test.c:190:test_fw_cfg_reboot_timeout: > assertion failed (reboot_timeout == 15): (251658240 == 15) > Aborted > > 251658240 is 0x0F00, i.e. a byte-swapped 0xf = 15 ... i.e. you still > got an endianess issue somewhere in the code. > H, I have thought a long time, still can't point where is wrong. Let's from the result: 0x0f00 in the big endian laid as this: low ---> high 0x0f 00 00 00 As I have swapped before the compare so it is read as this: low ---> high 00 00 00 0x0f However from the store side: the 15 in big endian is: low ---> high 00 00 00 0x0f But Before I store it, I convert it to little endian, so following should be stored: low ---> high 0x0f 00 00 00 Do you apply the patch 3 and recompile the qemu binary? If it is, I may need your help as I have no big endian host device. You can debug and inspect the memory layout and point out where is wrong. Thanks, Li Qiang > > Thomas >
Re: [Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
On 24/04/2019 16.06, Li Qiang wrote: > In the disscuss of adding reboot timeout test case: > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03304.html > > Philippe suggested we should uses the only related option for one > specific test. However currently we uses one QTestState for all the > test cases. In order to achieve Philippe's idea, I split the test case > for its own QTestState. As this patchset has changed a lot, I don't bump > the version. > > Change since v1: > Add a patch to store the reboot_timeout as little endian > Fix the endian issue per Thomas's review The test still aborts on a big endian host: $ 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:/home/thuth/devel/qemu/tests/fw_cfg-test.c:190:test_fw_cfg_reboot_timeout: assertion failed (reboot_timeout == 15): (251658240 == 15) Aborted 251658240 is 0x0F00, i.e. a byte-swapped 0xf = 15 ... i.e. you still got an endianess issue somewhere in the code. Thomas
