[Qemu-devel] [PATCH 0/4] fw_cfg_test refactor and add two test cases

[Qemu-devel] [PATCH 0/4] fw_cfg_test refactor and add two test cases

[Li Qiang]

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.


Li Qiang (4):
  tests: refactor fw_cfg_test
  tests: fw_cfg: add a function to get the fw_cfg file
  tests: fw_cfg: add reboot_timeout test case
  tests: fw_cfg: add splash time test case

 tests/fw_cfg-test.c   | 122 +++++++++++++++++++++++++++++++++++++++---
 tests/libqos/fw_cfg.c |  55 +++++++++++++++++++
 tests/libqos/fw_cfg.h |   9 ++++
 3 files changed, 178 insertions(+), 8 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 0/4] fw_cfg_test refactor and add two test cases

[Li Qiang]

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.


Li Qiang (4):
  tests: refactor fw_cfg_test
  tests: fw_cfg: add a function to get the fw_cfg file
  tests: fw_cfg: add reboot_timeout test case
  tests: fw_cfg: add splash time test case

 tests/fw_cfg-test.c   | 122 +++++++++++++++++++++++++++++++++++++++---
 tests/libqos/fw_cfg.c |  55 +++++++++++++++++++
 tests/libqos/fw_cfg.h |   9 ++++
 3 files changed, 178 insertions(+), 8 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/4] tests: refactor fw_cfg_test

[Li Qiang]

Currently, fw_cfg_test uses one QTestState for every test case.
This will add all command lines for every test case and
this is unnecessary. This patch split the test cases and for
every test case it uses his own QTestState. This patch does following
things:

1. Get rid of the global 'fw_cfg', this need add a uninit function

2. Convert every test case in a separate QTestState

After this patch, we can add fw_cfg test case freely and will not
have efect other test cases.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 tests/fw_cfg-test.c   | 86 ++++++++++++++++++++++++++++++++++++++-----
 tests/libqos/fw_cfg.c | 10 +++++
 tests/libqos/fw_cfg.h |  7 ++++
 3 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 1c5103fe1c..c22503619f 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -21,62 +21,127 @@ static uint16_t nb_cpus = 1;
 static uint16_t max_cpus = 1;
 static uint64_t nb_nodes = 0;
 static uint16_t boot_menu = 0;
-static QFWCFG *fw_cfg = NULL;
 
 static void test_fw_cfg_signature(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
     char buf[5];
 
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
     buf[4] = 0;
 
     g_assert_cmpstr(buf, ==, "QEMU");
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_id(void)
 {
-    uint32_t id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
+    QFWCFG *fw_cfg;
+    QTestState *s;
+    uint32_t id;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
+    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
     g_assert((id == 1) ||
              (id == 3));
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_uuid(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
     uint8_t buf[16];
     static const uint8_t uuid[16] = {
         0x46, 0x00, 0xcb, 0x32, 0x38, 0xec, 0x4b, 0x2f,
         0x8a, 0xcb, 0x81, 0xc6, 0xea, 0x54, 0xf2, 0xd8,
     };
 
+    s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
+    fw_cfg = pc_fw_cfg_init(s);
+
     qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
     g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
+
 }
 
 static void test_fw_cfg_ram_size(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_nographic(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_nb_cpus(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_max_cpus(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_numa(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
     uint64_t *cpu_mask;
     uint64_t *node_mask;
 
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
 
     cpu_mask = g_new0(uint64_t, max_cpus);
@@ -92,24 +157,29 @@ static void test_fw_cfg_numa(void)
 
     g_free(node_mask);
     g_free(cpu_mask);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_boot_menu(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, boot_menu);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 int main(int argc, char **argv)
 {
-    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
-    s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
-
-    fw_cfg = pc_fw_cfg_init(s);
-
     qtest_add_func("fw_cfg/signature", test_fw_cfg_signature);
     qtest_add_func("fw_cfg/id", test_fw_cfg_id);
     qtest_add_func("fw_cfg/uuid", test_fw_cfg_uuid);
@@ -128,7 +198,5 @@ int main(int argc, char **argv)
 
     ret = g_test_run();
 
-    qtest_quit(s);
-
     return ret;
 }
diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index d0889d1e22..c6839c53c8 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -81,6 +81,11 @@ QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base)
     return fw_cfg;
 }
 
+void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
+{
+    g_free(fw_cfg);
+}
+
 static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
 {
     qtest_outw(fw_cfg->qts, fw_cfg->base, key);
@@ -107,3 +112,8 @@ QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base)
 
     return fw_cfg;
 }
+
+void io_fw_cfg_uninit(QFWCFG *fw_cfg)
+{
+    g_free(fw_cfg);
+}
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 0353416af0..60de81e863 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -33,11 +33,18 @@ uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key);
 uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key);
 
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
+void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
 QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
+void io_fw_cfg_uninit(QFWCFG *fw_cfg);
 
 static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
 {
     return io_fw_cfg_init(qts, 0x510);
 }
 
+static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
+{
+    io_fw_cfg_uninit(fw_cfg);
+}
+
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH 1/4] tests: refactor fw_cfg_test

[Li Qiang]

Currently, fw_cfg_test uses one QTestState for every test case.
This will add all command lines for every test case and
this is unnecessary. This patch split the test cases and for
every test case it uses his own QTestState. This patch does following
things:

1. Get rid of the global 'fw_cfg', this need add a uninit function

2. Convert every test case in a separate QTestState

After this patch, we can add fw_cfg test case freely and will not
have efect other test cases.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 tests/fw_cfg-test.c   | 86 ++++++++++++++++++++++++++++++++++++++-----
 tests/libqos/fw_cfg.c | 10 +++++
 tests/libqos/fw_cfg.h |  7 ++++
 3 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 1c5103fe1c..c22503619f 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -21,62 +21,127 @@ static uint16_t nb_cpus = 1;
 static uint16_t max_cpus = 1;
 static uint64_t nb_nodes = 0;
 static uint16_t boot_menu = 0;
-static QFWCFG *fw_cfg = NULL;
 
 static void test_fw_cfg_signature(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
     char buf[5];
 
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
     buf[4] = 0;
 
     g_assert_cmpstr(buf, ==, "QEMU");
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_id(void)
 {
-    uint32_t id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
+    QFWCFG *fw_cfg;
+    QTestState *s;
+    uint32_t id;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
+    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
     g_assert((id == 1) ||
              (id == 3));
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_uuid(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
     uint8_t buf[16];
     static const uint8_t uuid[16] = {
         0x46, 0x00, 0xcb, 0x32, 0x38, 0xec, 0x4b, 0x2f,
         0x8a, 0xcb, 0x81, 0xc6, 0xea, 0x54, 0xf2, 0xd8,
     };
 
+    s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
+    fw_cfg = pc_fw_cfg_init(s);
+
     qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
     g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
+
 }
 
 static void test_fw_cfg_ram_size(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_nographic(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_nb_cpus(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
+
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_max_cpus(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_numa(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
     uint64_t *cpu_mask;
     uint64_t *node_mask;
 
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
 
     cpu_mask = g_new0(uint64_t, max_cpus);
@@ -92,24 +157,29 @@ static void test_fw_cfg_numa(void)
 
     g_free(node_mask);
     g_free(cpu_mask);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 static void test_fw_cfg_boot_menu(void)
 {
+    QFWCFG *fw_cfg;
+    QTestState *s;
+
+    s = qtest_init("");
+    fw_cfg = pc_fw_cfg_init(s);
+
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, boot_menu);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
 }
 
 int main(int argc, char **argv)
 {
-    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
-    s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
-
-    fw_cfg = pc_fw_cfg_init(s);
-
     qtest_add_func("fw_cfg/signature", test_fw_cfg_signature);
     qtest_add_func("fw_cfg/id", test_fw_cfg_id);
     qtest_add_func("fw_cfg/uuid", test_fw_cfg_uuid);
@@ -128,7 +198,5 @@ int main(int argc, char **argv)
 
     ret = g_test_run();
 
-    qtest_quit(s);
-
     return ret;
 }
diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index d0889d1e22..c6839c53c8 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -81,6 +81,11 @@ QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base)
     return fw_cfg;
 }
 
+void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
+{
+    g_free(fw_cfg);
+}
+
 static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
 {
     qtest_outw(fw_cfg->qts, fw_cfg->base, key);
@@ -107,3 +112,8 @@ QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base)
 
     return fw_cfg;
 }
+
+void io_fw_cfg_uninit(QFWCFG *fw_cfg)
+{
+    g_free(fw_cfg);
+}
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 0353416af0..60de81e863 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -33,11 +33,18 @@ uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key);
 uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key);
 
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
+void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
 QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
+void io_fw_cfg_uninit(QFWCFG *fw_cfg);
 
 static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
 {
     return io_fw_cfg_init(qts, 0x510);
 }
 
+static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
+{
+    io_fw_cfg_uninit(fw_cfg);
+}
+
 #endif
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/4] tests: refactor fw_cfg_test

[Thomas Huth]

On 20/04/2019 12.00, Li Qiang wrote:
> Currently, fw_cfg_test uses one QTestState for every test case.
> This will add all command lines for every test case and
> this is unnecessary. This patch split the test cases and for
> every test case it uses his own QTestState. This patch does following
> things:
> 
> 1. Get rid of the global 'fw_cfg', this need add a uninit function
> 
> 2. Convert every test case in a separate QTestState
> 
> After this patch, we can add fw_cfg test case freely and will not
> have efect other test cases.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/fw_cfg-test.c   | 86 ++++++++++++++++++++++++++++++++++++++-----
>  tests/libqos/fw_cfg.c | 10 +++++
>  tests/libqos/fw_cfg.h |  7 ++++
>  3 files changed, 94 insertions(+), 9 deletions(-)

Sounds like a good idea.

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 1/4] tests: refactor fw_cfg_test

[Thomas Huth]

On 20/04/2019 12.00, Li Qiang wrote:
> Currently, fw_cfg_test uses one QTestState for every test case.
> This will add all command lines for every test case and
> this is unnecessary. This patch split the test cases and for
> every test case it uses his own QTestState. This patch does following
> things:
> 
> 1. Get rid of the global 'fw_cfg', this need add a uninit function
> 
> 2. Convert every test case in a separate QTestState
> 
> After this patch, we can add fw_cfg test case freely and will not
> have efect other test cases.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/fw_cfg-test.c   | 86 ++++++++++++++++++++++++++++++++++++++-----
>  tests/libqos/fw_cfg.c | 10 +++++
>  tests/libqos/fw_cfg.h |  7 ++++
>  3 files changed, 94 insertions(+), 9 deletions(-)

Sounds like a good idea.

Acked-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH 2/4] tests: fw_cfg: add a function to get the fw_cfg file

[Li Qiang]

This is useful to write qtest about fw_cfg file entry.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 tests/libqos/fw_cfg.h |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index c6839c53c8..1f46258f96 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -16,6 +16,7 @@
 #include "libqos/fw_cfg.h"
 #include "libqtest.h"
 #include "qemu/bswap.h"
+#include "hw/nvram/fw_cfg.h"
 
 void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
 {
@@ -59,6 +60,50 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
     qtest_writew(fw_cfg->qts, fw_cfg->base, key);
 }
 
+/*
+ * The caller need check the return value. When the return value is
+ * nonzero, it means that some bytes have been transferred.
+ *
+ * If the fw_cfg file in question is smaller than the allocated & passed-in
+ * buffer, then the buffer has been populated only in part.
+ *
+ * If the fw_cfg file in question is larger than the passed-in
+ * buffer, then the return value explains how much room would have been
+ * necessary in total. And, while the caller's buffer has been fully
+ * populated, it has received only a starting slice of the fw_cfg file.
+ */
+size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
+                      void *data, size_t buflen)
+{
+    uint32_t count;
+    uint32_t i;
+    unsigned char *filesbuf = NULL;
+    size_t dsize;
+    FWCfgFile *pdir_entry;
+    size_t filesize = 0;
+
+    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
+    count = be32_to_cpu(count);
+    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
+    filesbuf = g_malloc(dsize);
+    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
+    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
+    for (i = 0; i < count; ++i, ++pdir_entry) {
+        if (!strcmp(pdir_entry->name, filename)) {
+            uint32_t len = be32_to_cpu(pdir_entry->size);
+            uint16_t sel = be16_to_cpu(pdir_entry->select);
+            filesize = len;
+            if (len > buflen) {
+                len = buflen;
+            }
+            qfw_cfg_get(fw_cfg, sel, data, len);
+            break;
+        }
+    }
+    g_free(filesbuf);
+    return filesize;
+}
+
 static void mm_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
 {
     uint8_t *ptr = data;
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 60de81e863..13325cc4ff 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -31,6 +31,8 @@ void qfw_cfg_get(QFWCFG *fw_cfg, uint16_t key, void *data, size_t len);
 uint16_t qfw_cfg_get_u16(QFWCFG *fw_cfg, uint16_t key);
 uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key);
 uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key);
+size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
+                        void *data, size_t buflen);
 
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
 void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
-- 
2.17.1

[Qemu-devel] [PATCH 2/4] tests: fw_cfg: add a function to get the fw_cfg file

[Li Qiang]

This is useful to write qtest about fw_cfg file entry.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 tests/libqos/fw_cfg.h |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index c6839c53c8..1f46258f96 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -16,6 +16,7 @@
 #include "libqos/fw_cfg.h"
 #include "libqtest.h"
 #include "qemu/bswap.h"
+#include "hw/nvram/fw_cfg.h"
 
 void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
 {
@@ -59,6 +60,50 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
     qtest_writew(fw_cfg->qts, fw_cfg->base, key);
 }
 
+/*
+ * The caller need check the return value. When the return value is
+ * nonzero, it means that some bytes have been transferred.
+ *
+ * If the fw_cfg file in question is smaller than the allocated & passed-in
+ * buffer, then the buffer has been populated only in part.
+ *
+ * If the fw_cfg file in question is larger than the passed-in
+ * buffer, then the return value explains how much room would have been
+ * necessary in total. And, while the caller's buffer has been fully
+ * populated, it has received only a starting slice of the fw_cfg file.
+ */
+size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
+                      void *data, size_t buflen)
+{
+    uint32_t count;
+    uint32_t i;
+    unsigned char *filesbuf = NULL;
+    size_t dsize;
+    FWCfgFile *pdir_entry;
+    size_t filesize = 0;
+
+    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
+    count = be32_to_cpu(count);
+    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
+    filesbuf = g_malloc(dsize);
+    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
+    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
+    for (i = 0; i < count; ++i, ++pdir_entry) {
+        if (!strcmp(pdir_entry->name, filename)) {
+            uint32_t len = be32_to_cpu(pdir_entry->size);
+            uint16_t sel = be16_to_cpu(pdir_entry->select);
+            filesize = len;
+            if (len > buflen) {
+                len = buflen;
+            }
+            qfw_cfg_get(fw_cfg, sel, data, len);
+            break;
+        }
+    }
+    g_free(filesbuf);
+    return filesize;
+}
+
 static void mm_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
 {
     uint8_t *ptr = data;
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 60de81e863..13325cc4ff 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -31,6 +31,8 @@ void qfw_cfg_get(QFWCFG *fw_cfg, uint16_t key, void *data, size_t len);
 uint16_t qfw_cfg_get_u16(QFWCFG *fw_cfg, uint16_t key);
 uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key);
 uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key);
+size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
+                        void *data, size_t buflen);
 
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
 void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/4] tests: fw_cfg: add a function to get the fw_cfg file

[Thomas Huth]

On 20/04/2019 12.00, Li Qiang wrote:
> This is useful to write qtest about fw_cfg file entry.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/fw_cfg.h |  2 ++
>  2 files changed, 47 insertions(+)

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] tests: fw_cfg: add a function to get the fw_cfg file

[Thomas Huth]

On 20/04/2019 12.00, Li Qiang wrote:
> This is useful to write qtest about fw_cfg file entry.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/fw_cfg.h |  2 ++
>  2 files changed, 47 insertions(+)

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Li Qiang]

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);
+    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();
 
-- 
2.17.1

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

[Li Qiang]

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);
+    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();
 
-- 
2.17.1

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

[Thomas Huth]

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

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

[Thomas Huth]

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

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

[Li Qiang]

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
>

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

[Li Qiang]

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
>

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

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

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

[Li Qiang]

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
>

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

[Li Qiang]

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
>

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

[Li Qiang]

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
>

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

[Li Qiang]

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
>

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

[Gerd Hoffmann]

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

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

[Gerd Hoffmann]

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

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

[Li Qiang]

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

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

[Li Qiang]

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

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

[Philippe Mathieu-Daudé]

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
> 

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

[Philippe Mathieu-Daudé]

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
> 

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

[Qemu-devel] [PATCH 4/4] tests: fw_cfg: add splash time test case

[Li Qiang]

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 9f75dbb5f4..de8e81ea9d 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -192,6 +192,24 @@ static void test_fw_cfg_reboot_timeout(void)
     qtest_quit(s);
 }
 
+static void test_fw_cfg_splash_time(void)
+{
+    QFWCFG *fw_cfg;
+    QTestState *s;
+    uint16_t splash_time = 0;
+    size_t filesize;
+
+    s = qtest_init("-boot splash-time=12");
+    fw_cfg = pc_fw_cfg_init(s);
+
+    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-menu-wait",
+                     &splash_time, sizeof(splash_time));
+    g_assert_cmpint(filesize, ==, sizeof(splash_time));
+    g_assert_cmpint(splash_time, ==, 12);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -214,6 +232,7 @@ int main(int argc, char **argv)
     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);
+    qtest_add_func("fw_cfg/splash_time", test_fw_cfg_splash_time);
 
     ret = g_test_run();
 
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] tests: fw_cfg: add splash time test case

[Li Qiang]

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 9f75dbb5f4..de8e81ea9d 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -192,6 +192,24 @@ static void test_fw_cfg_reboot_timeout(void)
     qtest_quit(s);
 }
 
+static void test_fw_cfg_splash_time(void)
+{
+    QFWCFG *fw_cfg;
+    QTestState *s;
+    uint16_t splash_time = 0;
+    size_t filesize;
+
+    s = qtest_init("-boot splash-time=12");
+    fw_cfg = pc_fw_cfg_init(s);
+
+    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-menu-wait",
+                     &splash_time, sizeof(splash_time));
+    g_assert_cmpint(filesize, ==, sizeof(splash_time));
+    g_assert_cmpint(splash_time, ==, 12);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -214,6 +232,7 @@ int main(int argc, char **argv)
     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);
+    qtest_add_func("fw_cfg/splash_time", test_fw_cfg_splash_time);
 
     ret = g_test_run();
 
-- 
2.17.1