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

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

[Philippe Mathieu-Daudé]

This is the work from Li Qiang, I added few changes while preparing
the pull request. If Li is OK with it, I'll send as it.

Since v2:
- split 1st patch
- clarified commit description based on review comments on list

Since v1:
- Add a patch to store the reboot_timeout as little endian

v2: https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg04064.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg05679.html

Li Qiang (5):
  tests: refactor fw_cfg_test
  tests: fw_cfg: add a function to get the fw_cfg file
  hw/nvram/fw_cfg: Store 'reboot-timeout' as little endian
  tests: fw_cfg: add 'reboot-timeout' test case
  tests: fw_cfg: add 'splash-time' test case

Philippe Mathieu-Daudé (2):
  tests/libqos: Add io_fw_cfg_uninit() and mm_fw_cfg_uninit()
  tests/libqos: Add pc_fw_cfg_uninit() and use it

 hw/nvram/fw_cfg.c        |   4 +-
 tests/fw_cfg-test.c      | 127 +++++++++++++++++++++++++++++++++++----
 tests/libqos/fw_cfg.c    |  55 +++++++++++++++++
 tests/libqos/fw_cfg.h    |   9 +++
 tests/libqos/malloc-pc.c |   2 +-
 5 files changed, 184 insertions(+), 13 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/7] tests/libqos: Add io_fw_cfg_uninit() and mm_fw_cfg_uninit()

[Philippe Mathieu-Daudé]

The mm_fw_cfg_init() allocates a QFWCFG object,
add mm_fw_cfg_uninit() to deallocate it.
Similarly with io_fw_cfg_init(), add io_fw_cfg_uninit().

Signed-off-by: Li Qiang <liq3ea@163.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190424140643.62457-2-liq3ea@163.com>
[PMD: Split patch, filled commit description]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/libqos/fw_cfg.c | 10 ++++++++++
 tests/libqos/fw_cfg.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index d0889d1e22a..c6839c53c80 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 0353416af07..391669031a3 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -33,7 +33,9 @@ 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)
 {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 1/7] tests/libqos: Add io_fw_cfg_uninit() and mm_fw_cfg_uninit()

[Laszlo Ersek]

On 05/20/19 23:36, Philippe Mathieu-Daudé wrote:
> The mm_fw_cfg_init() allocates a QFWCFG object,
> add mm_fw_cfg_uninit() to deallocate it.
> Similarly with io_fw_cfg_init(), add io_fw_cfg_uninit().
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> Tested-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20190424140643.62457-2-liq3ea@163.com>
> [PMD: Split patch, filled commit description]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/libqos/fw_cfg.c | 10 ++++++++++
>  tests/libqos/fw_cfg.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
> index d0889d1e22a..c6839c53c80 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 0353416af07..391669031a3 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -33,7 +33,9 @@ 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)
>  {
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH v3 2/7] tests/libqos: Add pc_fw_cfg_uninit() and use it

[Philippe Mathieu-Daudé]

The pc_fw_cfg_init() function allocates an IO QFWCFG object.
Add the pc_fw_cfg_uninit() function to deallocate it (and use it).

Signed-off-by: Li Qiang <liq3ea@163.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20190424140643.62457-2-liq3ea@163.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[PMD: Split patch, fill commit description, call uninit in malloc-pc.c]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c      | 1 +
 tests/libqos/fw_cfg.h    | 5 +++++
 tests/libqos/malloc-pc.c | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 1c5103fe1c5..a370ad56678 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -128,6 +128,7 @@ int main(int argc, char **argv)
 
     ret = g_test_run();
 
+    pc_fw_cfg_uninit(fw_cfg);
     qtest_quit(s);
 
     return ret;
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 391669031a3..60de81e8633 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -42,4 +42,9 @@ 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
diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
index 949a99361d1..6f92ce41350 100644
--- a/tests/libqos/malloc-pc.c
+++ b/tests/libqos/malloc-pc.c
@@ -29,5 +29,5 @@ void pc_alloc_init(QGuestAllocator *s, QTestState *qts, QAllocOpts flags)
     alloc_init(s, flags, 1 << 20, MIN(ram_size, 0xE0000000), PAGE_SIZE);
 
     /* clean-up */
-    g_free(fw_cfg);
+    pc_fw_cfg_uninit(fw_cfg);
 }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 2/7] tests/libqos: Add pc_fw_cfg_uninit() and use it

[Laszlo Ersek]

On 05/20/19 23:36, Philippe Mathieu-Daudé wrote:
> The pc_fw_cfg_init() function allocates an IO QFWCFG object.
> Add the pc_fw_cfg_uninit() function to deallocate it (and use it).
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> Tested-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20190424140643.62457-2-liq3ea@163.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> [PMD: Split patch, fill commit description, call uninit in malloc-pc.c]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/fw_cfg-test.c      | 1 +
>  tests/libqos/fw_cfg.h    | 5 +++++
>  tests/libqos/malloc-pc.c | 2 +-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 1c5103fe1c5..a370ad56678 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -128,6 +128,7 @@ int main(int argc, char **argv)
>  
>      ret = g_test_run();
>  
> +    pc_fw_cfg_uninit(fw_cfg);
>      qtest_quit(s);
>  
>      return ret;
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index 391669031a3..60de81e8633 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -42,4 +42,9 @@ 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
> diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
> index 949a99361d1..6f92ce41350 100644
> --- a/tests/libqos/malloc-pc.c
> +++ b/tests/libqos/malloc-pc.c
> @@ -29,5 +29,5 @@ void pc_alloc_init(QGuestAllocator *s, QTestState *qts, QAllocOpts flags)
>      alloc_init(s, flags, 1 << 20, MIN(ram_size, 0xE0000000), PAGE_SIZE);
>  
>      /* clean-up */
> -    g_free(fw_cfg);
> +    pc_fw_cfg_uninit(fw_cfg);
>  }
> 

The 2nd part of the patch is a refactoring, but the first patch adds a
brand new g_free(), in effect. I think it would be better to separate
them -- in theory anyway. But I realize this patch is already the result
of splitting another patch. I guess we wouldn't want an army of 1-liner
patches...

If you split this patch even further, that's great, you can add my R-b
to both resultant patches. If you decide to keep it as-is, you can also
add my

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(I'm going to skip the rest of the patches, as they are from Li Qiang,
and you reviewed them already, without implementing changes on top, IIUC.)

Thanks
Laszlo

[Qemu-devel] [PATCH v3 3/7] tests: refactor fw_cfg_test

[Philippe Mathieu-Daudé]

From: Li Qiang <liq3ea@163.com>

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 effect on other test cases.

Signed-off-by: Li Qiang <liq3ea@163.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190424140643.62457-2-liq3ea@163.com>
[PMD: Removed 'ret' local variable in main()]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c | 93 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 15 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index a370ad56678..4597626dd78 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,27 @@ 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);
@@ -126,10 +194,5 @@ 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);
 
-    ret = g_test_run();
-
-    pc_fw_cfg_uninit(fw_cfg);
-    qtest_quit(s);
-
-    return ret;
+    return g_test_run();
 }
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

From: Li Qiang <liq3ea@163.com>

This is useful to write qtest about fw_cfg file entry.

Signed-off-by: Li Qiang <liq3ea@163.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190424140643.62457-3-liq3ea@163.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.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 c6839c53c80..1f46258f96b 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 60de81e8633..13325cc4ffe 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.20.1

[Qemu-devel] [PATCH v3 5/7] hw/nvram/fw_cfg: Store 'reboot-timeout' as little endian

[Philippe Mathieu-Daudé]

From: Li Qiang <liq3ea@163.com>

The current codebase is not specific about the endianess of the
fw_cfg 'file' entry 'reboot-timeout'.

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

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.

OVMF consumes 'boot-menu-wait' as little-endian, however it does
not consume 'reboot-timeout'.

Regarding the git history and OVMF use, we choose to explicit
'reboot-timeout' endianess as little-endian.

Signed-off-by: Li Qiang <liq3ea@163.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190424140643.62457-4-liq3ea@163.com>
[PMD: Reword commit description based on review comments]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5c3a46ce6f2..df4242fc9cb 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -178,6 +178,7 @@ static void fw_cfg_reboot(FWCfgState *s)
 {
     const char *reboot_timeout = NULL;
     int64_t rt_val = -1;
+    uint32_t rt_le32;
 
     /* get user configuration */
     QemuOptsList *plist = qemu_find_opts("boot-opts");
@@ -194,7 +195,8 @@ static void fw_cfg_reboot(FWCfgState *s)
         }
     }
 
-    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
+    rt_le32 = cpu_to_le32(rt_val);
+    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_le32, 4), 4);
 }
 
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
-- 
2.20.1

[Qemu-devel] [PATCH v3 6/7] tests: fw_cfg: add 'reboot-timeout' test case

[Philippe Mathieu-Daudé]

From: Li Qiang <liq3ea@163.com>

Signed-off-by: Li Qiang <liq3ea@163.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190424140643.62457-5-liq3ea@163.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 4597626dd78..20b1eb75f4d 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -15,6 +15,7 @@
 #include "libqtest.h"
 #include "standard-headers/linux/qemu_fw_cfg.h"
 #include "libqos/fw_cfg.h"
+#include "qemu/bswap.h"
 
 static uint64_t ram_size = 128 << 20;
 static uint16_t nb_cpus = 1;
@@ -174,6 +175,25 @@ 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));
+    reboot_timeout = le32_to_cpu(reboot_timeout);
+    g_assert_cmpint(reboot_timeout, ==, 15);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -193,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);
 
     return g_test_run();
 }
-- 
2.20.1

[Qemu-devel] [PATCH v3 7/7] tests: fw_cfg: add 'splash-time' test case

[Philippe Mathieu-Daudé]

From: Li Qiang <liq3ea@163.com>

Signed-off-by: Li Qiang <liq3ea@163.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190424140643.62457-6-liq3ea@163.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 20b1eb75f4d..1d3147f8214 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -194,6 +194,25 @@ 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));
+    splash_time = le16_to_cpu(splash_time);
+    g_assert_cmpint(splash_time, ==, 12);
+    pc_fw_cfg_uninit(fw_cfg);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -214,6 +233,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);
 
     return g_test_run();
 }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 0/7] fw_cfg_test refactor and add two test cases

[Philippe Mathieu-Daudé]

On 5/20/19 11:36 PM, Philippe Mathieu-Daudé wrote:
> This is the work from Li Qiang, I added few changes while preparing
> the pull request. If Li is OK with it, I'll send as it.
> 
> Since v2:
> - split 1st patch
> - clarified commit description based on review comments on list
> 
> Since v1:
> - Add a patch to store the reboot_timeout as little endian
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg04064.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg05679.html
> 
> Li Qiang (5):
>   tests: refactor fw_cfg_test
>   tests: fw_cfg: add a function to get the fw_cfg file
>   hw/nvram/fw_cfg: Store 'reboot-timeout' as little endian
>   tests: fw_cfg: add 'reboot-timeout' test case
>   tests: fw_cfg: add 'splash-time' test case
> 
> Philippe Mathieu-Daudé (2):
>   tests/libqos: Add io_fw_cfg_uninit() and mm_fw_cfg_uninit()
>   tests/libqos: Add pc_fw_cfg_uninit() and use it
> 
>  hw/nvram/fw_cfg.c        |   4 +-
>  tests/fw_cfg-test.c      | 127 +++++++++++++++++++++++++++++++++++----
>  tests/libqos/fw_cfg.c    |  55 +++++++++++++++++
>  tests/libqos/fw_cfg.h    |   9 +++
>  tests/libqos/malloc-pc.c |   2 +-
>  5 files changed, 184 insertions(+), 13 deletions(-)
> 

Series queued (with Laszlo's comment on patch #2 addressed).