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
[Qemu-devel] [PATCH v2 0/5] fw_cfg_test refactor and add two test cases
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
