[PATCH v2 0/7] fw_cfg: Run tests on big-endian

[PATCH v2 0/7] fw_cfg: Run tests on big-endian

[Philippe Mathieu-Daudé]

This series allow fw_cfg tests to run on big-endian targets.

since v1 [1]:
- addressed Laszlo/Laurent/Thomas comments
- added Laszlo R-b tags

This should help us to notice regression such this one
introduced in QEMU v4.0.0:

  commit ee5d0f89de3e53cdb0dcf51acc1502b310ed3bd2
  Date:   Tue Nov 20 21:10:25 2018 -0800

    fw_cfg: Fix -boot reboot-timeout error checking

Later fixed in QEMU v4.1.0:

  commit 04da973501b591525ce68c2925c61c8886badd4d
  Date:   Wed Apr 24 07:06:41 2019 -0700

    hw/nvram/fw_cfg: Store 'reboot-timeout' as little endian

And older one that required manual testing [2], such:

  commit 36b62ae6a58f9a588fd33be9386e18a2b90103f5

    fw_cfg: fix endianness in fw_cfg_data_mem_read() / _write()

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00926.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03762.html

Philippe Mathieu-Daudé (7):
  tests/libqos/fw_cfg: Document io_fw_cfg_init to drop io_fw_cfg_uninit
  tests/libqos/fw_cfg: Document mm_fw_cfg_init to drop mm_fw_cfg_uninit
  tests/libqos/fw_cfg: Document pc_fw_cfg_init to drop pc_fw_cfg_uninit
  tests/fw_cfg: Let the tests use a context
  tests/libqos/fw_cfg: Pass QTestState as argument
  tests/fw_cfg: Declare one QFWCFG for all tests
  tests/fw_cfg: Run the tests on big-endian targets

 tests/Makefile.include   |   2 +
 tests/boot-order-test.c  |  12 +--
 tests/fw_cfg-test.c      | 189 ++++++++++++++++++++++-----------------
 tests/libqos/fw_cfg.c    |  71 +++++++--------
 tests/libqos/fw_cfg.h    |  56 +++++++-----
 tests/libqos/malloc-pc.c |   6 +-
 6 files changed, 186 insertions(+), 150 deletions(-)

-- 
2.21.0

[PATCH v2 1/7] tests/libqos/fw_cfg: Document io_fw_cfg_init to drop io_fw_cfg_uninit

[Philippe Mathieu-Daudé]

Document io_fw_cfg_init() return value must be released
with g_free(). Directly calling g_free() we don't really
need io_fw_cfg_uninit(): remove it.

This partly reverts commit 0729d833d6d6:
"tests/libqos: Add io_fw_cfg_uninit()"

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/libqos/fw_cfg.c |  5 -----
 tests/libqos/fw_cfg.h | 11 +++++++++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index 1f46258f96..37c3f2cf4d 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -157,8 +157,3 @@ 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 13325cc4ff..15604040bd 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -36,8 +36,15 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
 
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
 void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
+/**
+ * io_fw_cfg_init():
+ * @qts: The #QTestState that will be referred to by the QFWCFG object.
+ * @base: The I/O address of the fw_cfg device in the guest.
+ *
+ * Returns a newly allocated QFWCFG object which must be released
+ * with a call to g_free() when no longer required.
+ */
 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)
 {
@@ -46,7 +53,7 @@ static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
 
 static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
 {
-    io_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
 }
 
 #endif
-- 
2.21.0

[PATCH v2 2/7] tests/libqos/fw_cfg: Document mm_fw_cfg_init to drop mm_fw_cfg_uninit

[Philippe Mathieu-Daudé]

Document mm_fw_cfg_init() return value must be released
with g_free(). mm_fw_cfg_uninit() was never used, remove it.

This partly reverts commit 0729d833d6d6:
"tests/libqos: Add mm_fw_cfg_uninit()"

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/libqos/fw_cfg.c |  5 -----
 tests/libqos/fw_cfg.h | 10 +++++++++-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index 37c3f2cf4d..ddeec821db 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -126,11 +126,6 @@ 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);
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 15604040bd..3247fd4000 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -34,8 +34,16 @@ 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);
 
+/**
+ * mm_fw_cfg_init():
+ * @qts: The #QTestState that will be referred to by the QFWCFG object.
+ * @base: The MMIO base address of the fw_cfg device in the guest.
+ *
+ * Returns a newly allocated QFWCFG object which must be released
+ * with a call to g_free() when no longer required.
+ */
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
-void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
+
 /**
  * io_fw_cfg_init():
  * @qts: The #QTestState that will be referred to by the QFWCFG object.
-- 
2.21.0

[PATCH v2 3/7] tests/libqos/fw_cfg: Document pc_fw_cfg_init to drop pc_fw_cfg_uninit

[Philippe Mathieu-Daudé]

Document pc_fw_cfg_init() return value must be released
with g_free(). Directly calling g_free() we don't really
need pc_fw_cfg_uninit(): remove it.

This reverts commit 65461d124363:
"tests/libqos: Add pc_fw_cfg_uninit() and use it"

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c      | 22 +++++++++++-----------
 tests/libqos/fw_cfg.h    | 14 +++++++++-----
 tests/libqos/malloc-pc.c |  2 +-
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 1d3147f821..53ae82f7c8 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -36,7 +36,7 @@ static void test_fw_cfg_signature(void)
     buf[4] = 0;
 
     g_assert_cmpstr(buf, ==, "QEMU");
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -52,7 +52,7 @@ static void test_fw_cfg_id(void)
     id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
     g_assert((id == 1) ||
              (id == 3));
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -73,7 +73,7 @@ static void test_fw_cfg_uuid(void)
     qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
     g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
 
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 
 }
@@ -88,7 +88,7 @@ static void test_fw_cfg_ram_size(void)
 
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
 
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -102,7 +102,7 @@ static void test_fw_cfg_nographic(void)
 
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
 
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -116,7 +116,7 @@ static void test_fw_cfg_nb_cpus(void)
 
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
 
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -129,7 +129,7 @@ static void test_fw_cfg_max_cpus(void)
     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);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -158,7 +158,7 @@ static void test_fw_cfg_numa(void)
 
     g_free(node_mask);
     g_free(cpu_mask);
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -171,7 +171,7 @@ static void test_fw_cfg_boot_menu(void)
     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);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -190,7 +190,7 @@ static void test_fw_cfg_reboot_timeout(void)
     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);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
@@ -209,7 +209,7 @@ static void test_fw_cfg_splash_time(void)
     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);
+    g_free(fw_cfg);
     qtest_quit(s);
 }
 
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 3247fd4000..6316f4c354 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -54,14 +54,18 @@ QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
  */
 QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
 
+/**
+ * pc_fw_cfg_init():
+ * @qts: The #QTestState that will be referred to by the QFWCFG object.
+ *
+ * This function is specific to the PC machine (X86).
+ *
+ * Returns a newly allocated QFWCFG object which must be released
+ * with a call to g_free() when no longer required.
+ */
 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)
-{
-    g_free(fw_cfg);
-}
-
 #endif
diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
index 6f92ce4135..949a99361d 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 */
-    pc_fw_cfg_uninit(fw_cfg);
+    g_free(fw_cfg);
 }
-- 
2.21.0

[PATCH v2 4/7] tests/fw_cfg: Let the tests use a context

[Philippe Mathieu-Daudé]

Introduce the local QTestCtx structure, and register the tests
with qtest_add_data_func(ctx).
For now the context only contains the machine name (which is
fixed to the 'pc' machine, since this test only runs on the
x86 architecture).

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Use const QTestCtx *ctx, do not g_new(QTestCtx) (Laszlo)
---
 tests/fw_cfg-test.c | 87 ++++++++++++++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 53ae82f7c8..65785bca73 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -23,13 +23,18 @@ static uint16_t max_cpus = 1;
 static uint64_t nb_nodes = 0;
 static uint16_t boot_menu = 0;
 
-static void test_fw_cfg_signature(void)
+typedef struct {
+    const char *machine_name;
+} QTestCtx;
+
+static void test_fw_cfg_signature(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
     char buf[5];
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
@@ -40,13 +45,14 @@ static void test_fw_cfg_signature(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_id(void)
+static void test_fw_cfg_id(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
     uint32_t id;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
@@ -56,8 +62,9 @@ static void test_fw_cfg_id(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_uuid(void)
+static void test_fw_cfg_uuid(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
@@ -67,7 +74,8 @@ static void test_fw_cfg_uuid(void)
         0x8a, 0xcb, 0x81, 0xc6, 0xea, 0x54, 0xf2, 0xd8,
     };
 
-    s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
+    s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8",
+                    ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
@@ -78,12 +86,13 @@ static void test_fw_cfg_uuid(void)
 
 }
 
-static void test_fw_cfg_ram_size(void)
+static void test_fw_cfg_ram_size(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
@@ -92,12 +101,13 @@ static void test_fw_cfg_ram_size(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_nographic(void)
+static void test_fw_cfg_nographic(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
@@ -106,12 +116,13 @@ static void test_fw_cfg_nographic(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_nb_cpus(void)
+static void test_fw_cfg_nb_cpus(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
@@ -120,12 +131,13 @@ static void test_fw_cfg_nb_cpus(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_max_cpus(void)
+static void test_fw_cfg_max_cpus(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
@@ -133,14 +145,15 @@ static void test_fw_cfg_max_cpus(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_numa(void)
+static void test_fw_cfg_numa(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
     uint64_t *cpu_mask;
     uint64_t *node_mask;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
@@ -162,12 +175,13 @@ static void test_fw_cfg_numa(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_boot_menu(void)
+static void test_fw_cfg_boot_menu(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
-    s = qtest_init("");
+    s = qtest_initf("-M %s", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, boot_menu);
@@ -175,14 +189,15 @@ static void test_fw_cfg_boot_menu(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_reboot_timeout(void)
+static void test_fw_cfg_reboot_timeout(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
     uint32_t reboot_timeout = 0;
     size_t filesize;
 
-    s = qtest_init("-boot reboot-timeout=15");
+    s = qtest_initf("-M %s -boot reboot-timeout=15", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
@@ -194,14 +209,15 @@ static void test_fw_cfg_reboot_timeout(void)
     qtest_quit(s);
 }
 
-static void test_fw_cfg_splash_time(void)
+static void test_fw_cfg_splash_time(const void *opaque)
 {
+    const QTestCtx *ctx = opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
     uint16_t splash_time = 0;
     size_t filesize;
 
-    s = qtest_init("-boot splash-time=12");
+    s = qtest_initf("-M %s -boot splash-time=12", ctx->machine_name);
     fw_cfg = pc_fw_cfg_init(s);
 
     filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-menu-wait",
@@ -215,25 +231,30 @@ static void test_fw_cfg_splash_time(void)
 
 int main(int argc, char **argv)
 {
+    QTestCtx ctx;
+
     g_test_init(&argc, &argv, NULL);
 
-    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);
-    qtest_add_func("fw_cfg/ram_size", test_fw_cfg_ram_size);
-    qtest_add_func("fw_cfg/nographic", test_fw_cfg_nographic);
-    qtest_add_func("fw_cfg/nb_cpus", test_fw_cfg_nb_cpus);
+    ctx.machine_name = "pc";
+
+    qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
+    qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
+    qtest_add_data_func("fw_cfg/uuid", &ctx, test_fw_cfg_uuid);
+    qtest_add_data_func("fw_cfg/ram_size", &ctx, test_fw_cfg_ram_size);
+    qtest_add_data_func("fw_cfg/nographic", &ctx, test_fw_cfg_nographic);
+    qtest_add_data_func("fw_cfg/nb_cpus", &ctx, test_fw_cfg_nb_cpus);
 #if 0
     qtest_add_func("fw_cfg/machine_id", test_fw_cfg_machine_id);
     qtest_add_func("fw_cfg/kernel", test_fw_cfg_kernel);
     qtest_add_func("fw_cfg/initrd", test_fw_cfg_initrd);
     qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
 #endif
-    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);
-    qtest_add_func("fw_cfg/splash_time", test_fw_cfg_splash_time);
+    qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
+    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
+    qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu);
+    qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
+                        test_fw_cfg_reboot_timeout);
+    qtest_add_data_func("fw_cfg/splash_time", &ctx, test_fw_cfg_splash_time);
 
     return g_test_run();
 }
-- 
2.21.0

[PATCH v2 5/7] tests/libqos/fw_cfg: Pass QTestState as argument

[Philippe Mathieu-Daudé]

Since a QFWCFG object is not tied to a particular test, we can
call *_fw_cfg_init() once before creating QTests and use the same
for all the tests, then release the object with g_free() once all
the tests are run.

Refactor the qfw_cfg* API to take QTestState as argument.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/boot-order-test.c  | 12 ++++----
 tests/fw_cfg-test.c      | 49 ++++++++++++++++----------------
 tests/libqos/fw_cfg.c    | 61 ++++++++++++++++++++--------------------
 tests/libqos/fw_cfg.h    | 31 +++++++++-----------
 tests/libqos/malloc-pc.c |  4 +--
 5 files changed, 78 insertions(+), 79 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index a725bce729..e2d1b7940f 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -134,9 +134,9 @@ static void test_prep_boot_order(void)
 
 static uint64_t read_boot_order_pmac(QTestState *qts)
 {
-    QFWCFG *fw_cfg = mm_fw_cfg_init(qts, 0xf0000510);
+    QFWCFG *fw_cfg = mm_fw_cfg_init(0xf0000510);
 
-    return qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_DEVICE);
+    return qfw_cfg_get_u16(qts, fw_cfg, FW_CFG_BOOT_DEVICE);
 }
 
 static const boot_order_test test_cases_fw_cfg[] = {
@@ -159,9 +159,9 @@ static void test_pmac_newworld_boot_order(void)
 
 static uint64_t read_boot_order_sun4m(QTestState *qts)
 {
-    QFWCFG *fw_cfg = mm_fw_cfg_init(qts, 0xd00000510ULL);
+    QFWCFG *fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
 
-    return qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_DEVICE);
+    return qfw_cfg_get_u16(qts, fw_cfg, FW_CFG_BOOT_DEVICE);
 }
 
 static void test_sun4m_boot_order(void)
@@ -171,9 +171,9 @@ static void test_sun4m_boot_order(void)
 
 static uint64_t read_boot_order_sun4u(QTestState *qts)
 {
-    QFWCFG *fw_cfg = io_fw_cfg_init(qts, 0x510);
+    QFWCFG *fw_cfg = io_fw_cfg_init(0x510);
 
-    return qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_DEVICE);
+    return qfw_cfg_get_u16(qts, fw_cfg, FW_CFG_BOOT_DEVICE);
 }
 
 static void test_sun4u_boot_order(void)
diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 65785bca73..dda9a9fb07 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -35,9 +35,9 @@ static void test_fw_cfg_signature(const void *opaque)
     char buf[5];
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
+    qfw_cfg_get(s, fw_cfg, FW_CFG_SIGNATURE, buf, 4);
     buf[4] = 0;
 
     g_assert_cmpstr(buf, ==, "QEMU");
@@ -53,9 +53,9 @@ static void test_fw_cfg_id(const void *opaque)
     uint32_t id;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
+    id = qfw_cfg_get_u32(s, fw_cfg, FW_CFG_ID);
     g_assert((id == 1) ||
              (id == 3));
     g_free(fw_cfg);
@@ -76,9 +76,9 @@ static void test_fw_cfg_uuid(const void *opaque)
 
     s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8",
                     ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
+    qfw_cfg_get(s, fw_cfg, FW_CFG_UUID, buf, 16);
     g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
 
     g_free(fw_cfg);
@@ -93,9 +93,9 @@ static void test_fw_cfg_ram_size(const void *opaque)
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
+    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
 
     g_free(fw_cfg);
     qtest_quit(s);
@@ -108,9 +108,9 @@ static void test_fw_cfg_nographic(const void *opaque)
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
+    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
 
     g_free(fw_cfg);
     qtest_quit(s);
@@ -123,9 +123,9 @@ static void test_fw_cfg_nb_cpus(const void *opaque)
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
+    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
 
     g_free(fw_cfg);
     qtest_quit(s);
@@ -138,9 +138,9 @@ static void test_fw_cfg_max_cpus(const void *opaque)
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
+    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
     g_free(fw_cfg);
     qtest_quit(s);
 }
@@ -154,15 +154,15 @@ static void test_fw_cfg_numa(const void *opaque)
     uint64_t *node_mask;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
+    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
 
     cpu_mask = g_new0(uint64_t, max_cpus);
     node_mask = g_new0(uint64_t, nb_nodes);
 
-    qfw_cfg_read_data(fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
-    qfw_cfg_read_data(fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
+    qfw_cfg_read_data(s, fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
+    qfw_cfg_read_data(s, fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
 
     if (nb_nodes) {
         g_assert(cpu_mask[0] & 0x01);
@@ -182,9 +182,10 @@ static void test_fw_cfg_boot_menu(const void *opaque)
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, boot_menu);
+    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_BOOT_MENU),
+                    ==, boot_menu);
     g_free(fw_cfg);
     qtest_quit(s);
 }
@@ -198,9 +199,9 @@ static void test_fw_cfg_reboot_timeout(const void *opaque)
     size_t filesize;
 
     s = qtest_initf("-M %s -boot reboot-timeout=15", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
+    filesize = qfw_cfg_get_file(s, 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);
@@ -218,9 +219,9 @@ static void test_fw_cfg_splash_time(const void *opaque)
     size_t filesize;
 
     s = qtest_initf("-M %s -boot splash-time=12", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init(s);
+    fw_cfg = pc_fw_cfg_init();
 
-    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-menu-wait",
+    filesize = qfw_cfg_get_file(s, 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);
diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index ddeec821db..d25a367194 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -18,46 +18,47 @@
 #include "qemu/bswap.h"
 #include "hw/nvram/fw_cfg.h"
 
-void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
+void qfw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
 {
-    fw_cfg->select(fw_cfg, key);
+    fw_cfg->select(qts, fw_cfg, key);
 }
 
-void qfw_cfg_read_data(QFWCFG *fw_cfg, void *data, size_t len)
+void qfw_cfg_read_data(QTestState *qts, QFWCFG *fw_cfg, void *data, size_t len)
 {
-    fw_cfg->read(fw_cfg, data, len);
+    fw_cfg->read(qts, fw_cfg, data, len);
 }
 
-void qfw_cfg_get(QFWCFG *fw_cfg, uint16_t key, void *data, size_t len)
+void qfw_cfg_get(QTestState *qts, QFWCFG *fw_cfg, uint16_t key,
+                 void *data, size_t len)
 {
-    qfw_cfg_select(fw_cfg, key);
-    qfw_cfg_read_data(fw_cfg, data, len);
+    qfw_cfg_select(qts, fw_cfg, key);
+    qfw_cfg_read_data(qts, fw_cfg, data, len);
 }
 
-uint16_t qfw_cfg_get_u16(QFWCFG *fw_cfg, uint16_t key)
+uint16_t qfw_cfg_get_u16(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
 {
     uint16_t value;
-    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
+    qfw_cfg_get(qts, fw_cfg, key, &value, sizeof(value));
     return le16_to_cpu(value);
 }
 
-uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key)
+uint32_t qfw_cfg_get_u32(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
 {
     uint32_t value;
-    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
+    qfw_cfg_get(qts, fw_cfg, key, &value, sizeof(value));
     return le32_to_cpu(value);
 }
 
-uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key)
+uint64_t qfw_cfg_get_u64(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
 {
     uint64_t value;
-    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
+    qfw_cfg_get(qts, fw_cfg, key, &value, sizeof(value));
     return le64_to_cpu(value);
 }
 
-static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
+static void mm_fw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
 {
-    qtest_writew(fw_cfg->qts, fw_cfg->base, key);
+    qtest_writew(qts, fw_cfg->base, key);
 }
 
 /*
@@ -72,8 +73,8 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
  * 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)
+size_t qfw_cfg_get_file(QTestState *qts, QFWCFG *fw_cfg, const char *filename,
+                        void *data, size_t buflen)
 {
     uint32_t count;
     uint32_t i;
@@ -82,11 +83,11 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
     FWCfgFile *pdir_entry;
     size_t filesize = 0;
 
-    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
+    qfw_cfg_get(qts, 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);
+    qfw_cfg_get(qts, 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)) {
@@ -96,7 +97,7 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
             if (len > buflen) {
                 len = buflen;
             }
-            qfw_cfg_get(fw_cfg, sel, data, len);
+            qfw_cfg_get(qts, fw_cfg, sel, data, len);
             break;
         }
     }
@@ -104,49 +105,49 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
     return filesize;
 }
 
-static void mm_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
+static void mm_fw_cfg_read(QTestState *qts, QFWCFG *fw_cfg,
+                           void *data, size_t len)
 {
     uint8_t *ptr = data;
     int i;
 
     for (i = 0; i < len; i++) {
-        ptr[i] = qtest_readb(fw_cfg->qts, fw_cfg->base + 2);
+        ptr[i] = qtest_readb(qts, fw_cfg->base + 2);
     }
 }
 
-QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base)
+QFWCFG *mm_fw_cfg_init(uint64_t base)
 {
     QFWCFG *fw_cfg = g_malloc0(sizeof(*fw_cfg));
 
     fw_cfg->base = base;
-    fw_cfg->qts = qts;
     fw_cfg->select = mm_fw_cfg_select;
     fw_cfg->read = mm_fw_cfg_read;
 
     return fw_cfg;
 }
 
-static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
+static void io_fw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
 {
-    qtest_outw(fw_cfg->qts, fw_cfg->base, key);
+    qtest_outw(qts, fw_cfg->base, key);
 }
 
-static void io_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
+static void io_fw_cfg_read(QTestState *qts, QFWCFG *fw_cfg,
+                           void *data, size_t len)
 {
     uint8_t *ptr = data;
     int i;
 
     for (i = 0; i < len; i++) {
-        ptr[i] = qtest_inb(fw_cfg->qts, fw_cfg->base + 1);
+        ptr[i] = qtest_inb(qts, fw_cfg->base + 1);
     }
 }
 
-QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base)
+QFWCFG *io_fw_cfg_init(uint16_t base)
 {
     QFWCFG *fw_cfg = g_malloc0(sizeof(*fw_cfg));
 
     fw_cfg->base = base;
-    fw_cfg->qts = qts;
     fw_cfg->select = io_fw_cfg_select;
     fw_cfg->read = io_fw_cfg_read;
 
diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
index 6316f4c354..f9e69be450 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -20,52 +20,49 @@ typedef struct QFWCFG QFWCFG;
 struct QFWCFG
 {
     uint64_t base;
-    QTestState *qts;
-    void (*select)(QFWCFG *fw_cfg, uint16_t key);
-    void (*read)(QFWCFG *fw_cfg, void *data, size_t len);
+    void (*select)(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
+    void (*read)(QTestState *qts, QFWCFG *fw_cfg, void *data, size_t len);
 };
 
-void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key);
-void qfw_cfg_read_data(QFWCFG *fw_cfg, void *data, size_t len);
-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 qfw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
+void qfw_cfg_read_data(QTestState *qts, QFWCFG *fw_cfg, void *data, size_t len);
+void qfw_cfg_get(QTestState *qts, QFWCFG *fw_cfg, uint16_t key,
+                 void *data, size_t len);
+uint16_t qfw_cfg_get_u16(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
+uint32_t qfw_cfg_get_u32(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
+uint64_t qfw_cfg_get_u64(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
+size_t qfw_cfg_get_file(QTestState *qts, QFWCFG *fw_cfg, const char *filename,
                         void *data, size_t buflen);
 
 /**
  * mm_fw_cfg_init():
- * @qts: The #QTestState that will be referred to by the QFWCFG object.
  * @base: The MMIO base address of the fw_cfg device in the guest.
  *
  * Returns a newly allocated QFWCFG object which must be released
  * with a call to g_free() when no longer required.
  */
-QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
+QFWCFG *mm_fw_cfg_init(uint64_t base);
 
 /**
  * io_fw_cfg_init():
- * @qts: The #QTestState that will be referred to by the QFWCFG object.
  * @base: The I/O address of the fw_cfg device in the guest.
  *
  * Returns a newly allocated QFWCFG object which must be released
  * with a call to g_free() when no longer required.
  */
-QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
+QFWCFG *io_fw_cfg_init(uint16_t base);
 
 /**
  * pc_fw_cfg_init():
- * @qts: The #QTestState that will be referred to by the QFWCFG object.
  *
  * This function is specific to the PC machine (X86).
  *
  * Returns a newly allocated QFWCFG object which must be released
  * with a call to g_free() when no longer required.
  */
-static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
+static inline QFWCFG *pc_fw_cfg_init(void)
 {
-    return io_fw_cfg_init(qts, 0x510);
+    return io_fw_cfg_init(0x510);
 }
 
 #endif
diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
index 949a99361d..4932ae092d 100644
--- a/tests/libqos/malloc-pc.c
+++ b/tests/libqos/malloc-pc.c
@@ -23,9 +23,9 @@
 void pc_alloc_init(QGuestAllocator *s, QTestState *qts, QAllocOpts flags)
 {
     uint64_t ram_size;
-    QFWCFG *fw_cfg = pc_fw_cfg_init(qts);
+    QFWCFG *fw_cfg = pc_fw_cfg_init();
 
-    ram_size = qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE);
+    ram_size = qfw_cfg_get_u64(qts, fw_cfg, FW_CFG_RAM_SIZE);
     alloc_init(s, flags, 1 << 20, MIN(ram_size, 0xE0000000), PAGE_SIZE);
 
     /* clean-up */
-- 
2.21.0

[PATCH v2 6/7] tests/fw_cfg: Declare one QFWCFG for all tests

[Philippe Mathieu-Daudé]

It is pointless to create/remove a QFWCFG object for each test.
Move it to the test context and create/remove it only once.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c | 80 ++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 48 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index dda9a9fb07..35af0de7e6 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -25,47 +25,42 @@ static uint16_t boot_menu = 0;
 
 typedef struct {
     const char *machine_name;
+    QFWCFG *fw_cfg;
 } QTestCtx;
 
 static void test_fw_cfg_signature(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
     char buf[5];
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    qfw_cfg_get(s, fw_cfg, FW_CFG_SIGNATURE, buf, 4);
+    qfw_cfg_get(s, ctx->fw_cfg, FW_CFG_SIGNATURE, buf, 4);
     buf[4] = 0;
-
     g_assert_cmpstr(buf, ==, "QEMU");
-    g_free(fw_cfg);
+
     qtest_quit(s);
 }
 
 static void test_fw_cfg_id(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
     uint32_t id;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    id = qfw_cfg_get_u32(s, fw_cfg, FW_CFG_ID);
+    id = qfw_cfg_get_u32(s, ctx->fw_cfg, FW_CFG_ID);
     g_assert((id == 1) ||
              (id == 3));
-    g_free(fw_cfg);
+
     qtest_quit(s);
 }
 
 static void test_fw_cfg_uuid(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     uint8_t buf[16];
@@ -76,12 +71,10 @@ static void test_fw_cfg_uuid(const void *opaque)
 
     s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8",
                     ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    qfw_cfg_get(s, fw_cfg, FW_CFG_UUID, buf, 16);
+    qfw_cfg_get(s, ctx->fw_cfg, FW_CFG_UUID, buf, 16);
     g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
 
-    g_free(fw_cfg);
     qtest_quit(s);
 
 }
@@ -89,80 +82,71 @@ static void test_fw_cfg_uuid(const void *opaque)
 static void test_fw_cfg_ram_size(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
+    g_assert_cmpint(qfw_cfg_get_u64(s, ctx->fw_cfg, FW_CFG_RAM_SIZE),
+                    ==, ram_size);
 
-    g_free(fw_cfg);
     qtest_quit(s);
 }
 
 static void test_fw_cfg_nographic(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
+    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
 
-    g_free(fw_cfg);
     qtest_quit(s);
 }
 
 static void test_fw_cfg_nb_cpus(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
+    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_NB_CPUS),
+                    ==, nb_cpus);
 
-    g_free(fw_cfg);
     qtest_quit(s);
 }
 
 static void test_fw_cfg_max_cpus(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
-    g_free(fw_cfg);
+    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_MAX_CPUS),
+                    ==, max_cpus);
+
     qtest_quit(s);
 }
 
 static void test_fw_cfg_numa(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
     uint64_t *cpu_mask;
     uint64_t *node_mask;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
+    g_assert_cmpint(qfw_cfg_get_u64(s, ctx->fw_cfg, FW_CFG_NUMA),
+                    ==, nb_nodes);
 
     cpu_mask = g_new0(uint64_t, max_cpus);
     node_mask = g_new0(uint64_t, nb_nodes);
 
-    qfw_cfg_read_data(s, fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
-    qfw_cfg_read_data(s, fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
+    qfw_cfg_read_data(s, ctx->fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
+    qfw_cfg_read_data(s, ctx->fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
 
     if (nb_nodes) {
         g_assert(cpu_mask[0] & 0x01);
@@ -171,72 +155,68 @@ static void test_fw_cfg_numa(const void *opaque)
 
     g_free(node_mask);
     g_free(cpu_mask);
-    g_free(fw_cfg);
+
     qtest_quit(s);
 }
 
 static void test_fw_cfg_boot_menu(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     s = qtest_initf("-M %s", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_BOOT_MENU),
+    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_BOOT_MENU),
                     ==, boot_menu);
-    g_free(fw_cfg);
+
     qtest_quit(s);
 }
 
 static void test_fw_cfg_reboot_timeout(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
     uint32_t reboot_timeout = 0;
     size_t filesize;
 
     s = qtest_initf("-M %s -boot reboot-timeout=15", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    filesize = qfw_cfg_get_file(s, fw_cfg, "etc/boot-fail-wait",
+    filesize = qfw_cfg_get_file(s, ctx->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);
-    g_free(fw_cfg);
+
     qtest_quit(s);
 }
 
 static void test_fw_cfg_splash_time(const void *opaque)
 {
     const QTestCtx *ctx = opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
     uint16_t splash_time = 0;
     size_t filesize;
 
     s = qtest_initf("-M %s -boot splash-time=12", ctx->machine_name);
-    fw_cfg = pc_fw_cfg_init();
 
-    filesize = qfw_cfg_get_file(s, fw_cfg, "etc/boot-menu-wait",
+    filesize = qfw_cfg_get_file(s, ctx->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);
-    g_free(fw_cfg);
+
     qtest_quit(s);
 }
 
 int main(int argc, char **argv)
 {
     QTestCtx ctx;
+    int ret;
 
     g_test_init(&argc, &argv, NULL);
 
     ctx.machine_name = "pc";
+    ctx.fw_cfg = pc_fw_cfg_init();
 
     qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
     qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
@@ -257,5 +237,9 @@ int main(int argc, char **argv)
                         test_fw_cfg_reboot_timeout);
     qtest_add_data_func("fw_cfg/splash_time", &ctx, test_fw_cfg_splash_time);
 
-    return g_test_run();
+    ret = g_test_run();
+
+    g_free(ctx.fw_cfg);
+
+    return ret;
 }
-- 
2.21.0

[PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Philippe Mathieu-Daudé]

We have been restricting our fw_cfg tests to the PC machine,
which is a little-endian architecture.
The fw_cfg device is also used on the SPARC and PowerPC
architectures, which can run in big-endian configuration.

Since we want to be sure our device does not regress
regardless the endianess used, enable this test one
these targets.

The NUMA selector is X86 specific, restrict it to this arch.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: test ppc32 too (lvivier)
---
 tests/Makefile.include |  2 ++
 tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3543451ed3..4ae3d5140a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
 check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
 check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
 check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
+check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
 
 check-qtest-ppc64-y += $(check-qtest-ppc-y)
 check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF)
@@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
 check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
 check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
 check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
 
 check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
 check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 35af0de7e6..1250e87097 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void *opaque)
 
 int main(int argc, char **argv)
 {
-    QTestCtx ctx;
-    int ret;
+    const char *arch = qtest_get_arch();
+    bool has_numa = false;
+    QTestCtx ctx = {};
+    int ret = 0;
 
     g_test_init(&argc, &argv, NULL);
 
-    ctx.machine_name = "pc";
-    ctx.fw_cfg = pc_fw_cfg_init();
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
+        has_numa = true;
+        ctx.machine_name = "pc";
+        ctx.fw_cfg = pc_fw_cfg_init();
+    } else if (g_str_equal(arch, "sparc")) {
+        ctx.machine_name = "SS-5";
+        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
+    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) {
+        /*
+         * The mac99 machine is different for 32/64-bit target:
+         *
+         * ppc(32): the G4 which can be either little or big endian,
+         * ppc64:   the G5 (970FX) is only big-endian.
+         */
+        ctx.machine_name = "mac99";
+        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
+    }
 
     qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
     qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
@@ -231,14 +248,18 @@ int main(int argc, char **argv)
     qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
 #endif
     qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
-    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
     qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu);
     qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
                         test_fw_cfg_reboot_timeout);
     qtest_add_data_func("fw_cfg/splash_time", &ctx, test_fw_cfg_splash_time);
 
-    ret = g_test_run();
+    if (has_numa) {
+        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
+    }
 
+    if (ctx.machine_name) {
+        ret = g_test_run();
+    }
     g_free(ctx.fw_cfg);
 
     return ret;
-- 
2.21.0

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Laszlo Ersek]

On 10/07/19 17:19, Philippe Mathieu-Daudé wrote:
> We have been restricting our fw_cfg tests to the PC machine,
> which is a little-endian architecture.
> The fw_cfg device is also used on the SPARC and PowerPC
> architectures, which can run in big-endian configuration.
> 
> Since we want to be sure our device does not regress
> regardless the endianess used, enable this test one
> these targets.
> 
> The NUMA selector is X86 specific, restrict it to this arch.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: test ppc32 too (lvivier)
> ---
>  tests/Makefile.include |  2 ++
>  tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 3543451ed3..4ae3d5140a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>  check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
>  check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
>  check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
> +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
>  
>  check-qtest-ppc64-y += $(check-qtest-ppc-y)
>  check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF)
> @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
>  check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>  check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>  check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
>  
>  check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
>  check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 35af0de7e6..1250e87097 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void *opaque)
>  
>  int main(int argc, char **argv)
>  {
> -    QTestCtx ctx;
> -    int ret;
> +    const char *arch = qtest_get_arch();
> +    bool has_numa = false;
> +    QTestCtx ctx = {};
> +    int ret = 0;
>  
>      g_test_init(&argc, &argv, NULL);
>  
> -    ctx.machine_name = "pc";
> -    ctx.fw_cfg = pc_fw_cfg_init();
> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> +        has_numa = true;
> +        ctx.machine_name = "pc";
> +        ctx.fw_cfg = pc_fw_cfg_init();
> +    } else if (g_str_equal(arch, "sparc")) {
> +        ctx.machine_name = "SS-5";
> +        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
> +    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) {
> +        /*
> +         * The mac99 machine is different for 32/64-bit target:
> +         *
> +         * ppc(32): the G4 which can be either little or big endian,
> +         * ppc64:   the G5 (970FX) is only big-endian.
> +         */
> +        ctx.machine_name = "mac99";
> +        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
> +    }
>  
>      qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
>      qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
> @@ -231,14 +248,18 @@ int main(int argc, char **argv)
>      qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
>  #endif
>      qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
> -    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
>      qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu);
>      qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
>                          test_fw_cfg_reboot_timeout);
>      qtest_add_data_func("fw_cfg/splash_time", &ctx, test_fw_cfg_splash_time);
>  
> -    ret = g_test_run();
> +    if (has_numa) {
> +        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> +    }
>  
> +    if (ctx.machine_name) {
> +        ret = g_test_run();
> +    }
>      g_free(ctx.fw_cfg);
>  
>      return ret;
> 

Too many foreign arch bits in this patch for me to give an R-b, but
structurally it looks OK to me.

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

Thanks
Laszlo

Re: [PATCH v2 1/7] tests/libqos/fw_cfg: Document io_fw_cfg_init to drop io_fw_cfg_uninit

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:19写道：

> Document io_fw_cfg_init() return value must be released
> with g_free(). Directly calling g_free() we don't really
> need io_fw_cfg_uninit(): remove it.
>
> This partly reverts commit 0729d833d6d6:
> "tests/libqos: Add io_fw_cfg_uninit()"
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>



> ---
>  tests/libqos/fw_cfg.c |  5 -----
>  tests/libqos/fw_cfg.h | 11 +++++++++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
> index 1f46258f96..37c3f2cf4d 100644
> --- a/tests/libqos/fw_cfg.c
> +++ b/tests/libqos/fw_cfg.c
> @@ -157,8 +157,3 @@ 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 13325cc4ff..15604040bd 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -36,8 +36,15 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
> *filename,
>
>  QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
>  void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
> +/**
> + * io_fw_cfg_init():
> + * @qts: The #QTestState that will be referred to by the QFWCFG object.
> + * @base: The I/O address of the fw_cfg device in the guest.
> + *
> + * Returns a newly allocated QFWCFG object which must be released
> + * with a call to g_free() when no longer required.
> + */
>  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)
>  {
> @@ -46,7 +53,7 @@ static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
>
>  static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
>  {
> -    io_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>  }
>
>  #endif
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 2896 bytes --]

Re: [PATCH v2 2/7] tests/libqos/fw_cfg: Document mm_fw_cfg_init to drop mm_fw_cfg_uninit

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:19写道：

> Document mm_fw_cfg_init() return value must be released
> with g_free(). mm_fw_cfg_uninit() was never used, remove it.
>
> This partly reverts commit 0729d833d6d6:
> "tests/libqos: Add mm_fw_cfg_uninit()"
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>


Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  tests/libqos/fw_cfg.c |  5 -----
>  tests/libqos/fw_cfg.h | 10 +++++++++-
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
> index 37c3f2cf4d..ddeec821db 100644
> --- a/tests/libqos/fw_cfg.c
> +++ b/tests/libqos/fw_cfg.c
> @@ -126,11 +126,6 @@ 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);
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index 15604040bd..3247fd4000 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -34,8 +34,16 @@ 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);
>
> +/**
> + * mm_fw_cfg_init():
> + * @qts: The #QTestState that will be referred to by the QFWCFG object.
> + * @base: The MMIO base address of the fw_cfg device in the guest.
> + *
> + * Returns a newly allocated QFWCFG object which must be released
> + * with a call to g_free() when no longer required.
> + */
>  QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
> -void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
> +
>  /**
>   * io_fw_cfg_init():
>   * @qts: The #QTestState that will be referred to by the QFWCFG object.
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 2849 bytes --]

Re: [PATCH v2 3/7] tests/libqos/fw_cfg: Document pc_fw_cfg_init to drop pc_fw_cfg_uninit

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 5010 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:19写道：

> Document pc_fw_cfg_init() return value must be released
> with g_free(). Directly calling g_free() we don't really
> need pc_fw_cfg_uninit(): remove it.
>
> This reverts commit 65461d124363:
> "tests/libqos: Add pc_fw_cfg_uninit() and use it"
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  tests/fw_cfg-test.c      | 22 +++++++++++-----------
>  tests/libqos/fw_cfg.h    | 14 +++++++++-----
>  tests/libqos/malloc-pc.c |  2 +-
>  3 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 1d3147f821..53ae82f7c8 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -36,7 +36,7 @@ static void test_fw_cfg_signature(void)
>      buf[4] = 0;
>
>      g_assert_cmpstr(buf, ==, "QEMU");
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -52,7 +52,7 @@ static void test_fw_cfg_id(void)
>      id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
>      g_assert((id == 1) ||
>               (id == 3));
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -73,7 +73,7 @@ static void test_fw_cfg_uuid(void)
>      qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
>      g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
>
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>
>  }
> @@ -88,7 +88,7 @@ static void test_fw_cfg_ram_size(void)
>
>      g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==,
> ram_size);
>
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -102,7 +102,7 @@ static void test_fw_cfg_nographic(void)
>
>      g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
>
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -116,7 +116,7 @@ static void test_fw_cfg_nb_cpus(void)
>
>      g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
>
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -129,7 +129,7 @@ static void test_fw_cfg_max_cpus(void)
>      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);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -158,7 +158,7 @@ static void test_fw_cfg_numa(void)
>
>      g_free(node_mask);
>      g_free(cpu_mask);
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -171,7 +171,7 @@ static void test_fw_cfg_boot_menu(void)
>      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);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -190,7 +190,7 @@ static void test_fw_cfg_reboot_timeout(void)
>      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);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> @@ -209,7 +209,7 @@ static void test_fw_cfg_splash_time(void)
>      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);
> +    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index 3247fd4000..6316f4c354 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -54,14 +54,18 @@ QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
>   */
>  QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
>
> +/**
> + * pc_fw_cfg_init():
> + * @qts: The #QTestState that will be referred to by the QFWCFG object.
> + *
> + * This function is specific to the PC machine (X86).
> + *
> + * Returns a newly allocated QFWCFG object which must be released
> + * with a call to g_free() when no longer required.
> + */
>  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)
> -{
> -    g_free(fw_cfg);
> -}
> -
>  #endif
> diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
> index 6f92ce4135..949a99361d 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 */
> -    pc_fw_cfg_uninit(fw_cfg);
> +    g_free(fw_cfg);
>  }
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 6221 bytes --]

Re: [PATCH v2 4/7] tests/fw_cfg: Let the tests use a context

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 8900 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道：

> Introduce the local QTestCtx structure, and register the tests
> with qtest_add_data_func(ctx).
> For now the context only contains the machine name (which is
> fixed to the 'pc' machine, since this test only runs on the
> x86 architecture).
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
> v2: Use const QTestCtx *ctx, do not g_new(QTestCtx) (Laszlo)
> ---
>  tests/fw_cfg-test.c | 87 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 53ae82f7c8..65785bca73 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -23,13 +23,18 @@ static uint16_t max_cpus = 1;
>  static uint64_t nb_nodes = 0;
>  static uint16_t boot_menu = 0;
>
> -static void test_fw_cfg_signature(void)
> +typedef struct {
> +    const char *machine_name;
> +} QTestCtx;
> +
> +static void test_fw_cfg_signature(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>      char buf[5];
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
> @@ -40,13 +45,14 @@ static void test_fw_cfg_signature(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_id(void)
> +static void test_fw_cfg_id(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>      uint32_t id;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
> @@ -56,8 +62,9 @@ static void test_fw_cfg_id(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_uuid(void)
> +static void test_fw_cfg_uuid(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>
> @@ -67,7 +74,8 @@ static void test_fw_cfg_uuid(void)
>          0x8a, 0xcb, 0x81, 0xc6, 0xea, 0x54, 0xf2, 0xd8,
>      };
>
> -    s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
> +    s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8",
> +                    ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
> @@ -78,12 +86,13 @@ static void test_fw_cfg_uuid(void)
>
>  }
>
> -static void test_fw_cfg_ram_size(void)
> +static void test_fw_cfg_ram_size(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==,
> ram_size);
> @@ -92,12 +101,13 @@ static void test_fw_cfg_ram_size(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_nographic(void)
> +static void test_fw_cfg_nographic(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
> @@ -106,12 +116,13 @@ static void test_fw_cfg_nographic(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_nb_cpus(void)
> +static void test_fw_cfg_nb_cpus(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
> @@ -120,12 +131,13 @@ static void test_fw_cfg_nb_cpus(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_max_cpus(void)
> +static void test_fw_cfg_max_cpus(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==,
> max_cpus);
> @@ -133,14 +145,15 @@ static void test_fw_cfg_max_cpus(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_numa(void)
> +static void test_fw_cfg_numa(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>      uint64_t *cpu_mask;
>      uint64_t *node_mask;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
> @@ -162,12 +175,13 @@ static void test_fw_cfg_numa(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_boot_menu(void)
> +static void test_fw_cfg_boot_menu(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>
> -    s = qtest_init("");
> +    s = qtest_initf("-M %s", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==,
> boot_menu);
> @@ -175,14 +189,15 @@ static void test_fw_cfg_boot_menu(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_reboot_timeout(void)
> +static void test_fw_cfg_reboot_timeout(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>      uint32_t reboot_timeout = 0;
>      size_t filesize;
>
> -    s = qtest_init("-boot reboot-timeout=15");
> +    s = qtest_initf("-M %s -boot reboot-timeout=15", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> @@ -194,14 +209,15 @@ static void test_fw_cfg_reboot_timeout(void)
>      qtest_quit(s);
>  }
>
> -static void test_fw_cfg_splash_time(void)
> +static void test_fw_cfg_splash_time(const void *opaque)
>  {
> +    const QTestCtx *ctx = opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>      uint16_t splash_time = 0;
>      size_t filesize;
>
> -    s = qtest_init("-boot splash-time=12");
> +    s = qtest_initf("-M %s -boot splash-time=12", ctx->machine_name);
>      fw_cfg = pc_fw_cfg_init(s);
>
>      filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-menu-wait",
> @@ -215,25 +231,30 @@ static void test_fw_cfg_splash_time(void)
>
>  int main(int argc, char **argv)
>  {
> +    QTestCtx ctx;
> +
>      g_test_init(&argc, &argv, NULL);
>
> -    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);
> -    qtest_add_func("fw_cfg/ram_size", test_fw_cfg_ram_size);
> -    qtest_add_func("fw_cfg/nographic", test_fw_cfg_nographic);
> -    qtest_add_func("fw_cfg/nb_cpus", test_fw_cfg_nb_cpus);
> +    ctx.machine_name = "pc";
> +
> +    qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
> +    qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
> +    qtest_add_data_func("fw_cfg/uuid", &ctx, test_fw_cfg_uuid);
> +    qtest_add_data_func("fw_cfg/ram_size", &ctx, test_fw_cfg_ram_size);
> +    qtest_add_data_func("fw_cfg/nographic", &ctx, test_fw_cfg_nographic);
> +    qtest_add_data_func("fw_cfg/nb_cpus", &ctx, test_fw_cfg_nb_cpus);
>  #if 0
>      qtest_add_func("fw_cfg/machine_id", test_fw_cfg_machine_id);
>      qtest_add_func("fw_cfg/kernel", test_fw_cfg_kernel);
>      qtest_add_func("fw_cfg/initrd", test_fw_cfg_initrd);
>      qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
>  #endif
> -    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);
> -    qtest_add_func("fw_cfg/splash_time", test_fw_cfg_splash_time);
> +    qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
> +    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> +    qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu);
> +    qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
> +                        test_fw_cfg_reboot_timeout);
> +    qtest_add_data_func("fw_cfg/splash_time", &ctx,
> test_fw_cfg_splash_time);
>
>      return g_test_run();
>  }
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 11075 bytes --]

Re: [PATCH v2 5/7] tests/libqos/fw_cfg: Pass QTestState as argument

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 17572 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道：

> Since a QFWCFG object is not tied to a particular test, we can
> call *_fw_cfg_init() once before creating QTests and use the same
> for all the tests, then release the object with g_free() once all
> the tests are run.
>
> Refactor the qfw_cfg* API to take QTestState as argument.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  tests/boot-order-test.c  | 12 ++++----
>  tests/fw_cfg-test.c      | 49 ++++++++++++++++----------------
>  tests/libqos/fw_cfg.c    | 61 ++++++++++++++++++++--------------------
>  tests/libqos/fw_cfg.h    | 31 +++++++++-----------
>  tests/libqos/malloc-pc.c |  4 +--
>  5 files changed, 78 insertions(+), 79 deletions(-)
>
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> index a725bce729..e2d1b7940f 100644
> --- a/tests/boot-order-test.c
> +++ b/tests/boot-order-test.c
> @@ -134,9 +134,9 @@ static void test_prep_boot_order(void)
>
>  static uint64_t read_boot_order_pmac(QTestState *qts)
>  {
> -    QFWCFG *fw_cfg = mm_fw_cfg_init(qts, 0xf0000510);
> +    QFWCFG *fw_cfg = mm_fw_cfg_init(0xf0000510);
>
> -    return qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_DEVICE);
> +    return qfw_cfg_get_u16(qts, fw_cfg, FW_CFG_BOOT_DEVICE);
>  }
>
>  static const boot_order_test test_cases_fw_cfg[] = {
> @@ -159,9 +159,9 @@ static void test_pmac_newworld_boot_order(void)
>
>  static uint64_t read_boot_order_sun4m(QTestState *qts)
>  {
> -    QFWCFG *fw_cfg = mm_fw_cfg_init(qts, 0xd00000510ULL);
> +    QFWCFG *fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
>
> -    return qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_DEVICE);
> +    return qfw_cfg_get_u16(qts, fw_cfg, FW_CFG_BOOT_DEVICE);
>  }
>
>  static void test_sun4m_boot_order(void)
> @@ -171,9 +171,9 @@ static void test_sun4m_boot_order(void)
>
>  static uint64_t read_boot_order_sun4u(QTestState *qts)
>  {
> -    QFWCFG *fw_cfg = io_fw_cfg_init(qts, 0x510);
> +    QFWCFG *fw_cfg = io_fw_cfg_init(0x510);
>
> -    return qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_DEVICE);
> +    return qfw_cfg_get_u16(qts, fw_cfg, FW_CFG_BOOT_DEVICE);
>  }
>
>  static void test_sun4u_boot_order(void)
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 65785bca73..dda9a9fb07 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -35,9 +35,9 @@ static void test_fw_cfg_signature(const void *opaque)
>      char buf[5];
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
> +    qfw_cfg_get(s, fw_cfg, FW_CFG_SIGNATURE, buf, 4);
>      buf[4] = 0;
>
>      g_assert_cmpstr(buf, ==, "QEMU");
> @@ -53,9 +53,9 @@ static void test_fw_cfg_id(const void *opaque)
>      uint32_t id;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
> +    id = qfw_cfg_get_u32(s, fw_cfg, FW_CFG_ID);
>      g_assert((id == 1) ||
>               (id == 3));
>      g_free(fw_cfg);
> @@ -76,9 +76,9 @@ static void test_fw_cfg_uuid(const void *opaque)
>
>      s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8",
>                      ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
> +    qfw_cfg_get(s, fw_cfg, FW_CFG_UUID, buf, 16);
>      g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
>
>      g_free(fw_cfg);
> @@ -93,9 +93,9 @@ static void test_fw_cfg_ram_size(const void *opaque)
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==,
> ram_size);
> +    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_RAM_SIZE), ==,
> ram_size);
>
>      g_free(fw_cfg);
>      qtest_quit(s);
> @@ -108,9 +108,9 @@ static void test_fw_cfg_nographic(const void *opaque)
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
>
>      g_free(fw_cfg);
>      qtest_quit(s);
> @@ -123,9 +123,9 @@ static void test_fw_cfg_nb_cpus(const void *opaque)
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NB_CPUS), ==,
> nb_cpus);
>
>      g_free(fw_cfg);
>      qtest_quit(s);
> @@ -138,9 +138,9 @@ static void test_fw_cfg_max_cpus(const void *opaque)
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==,
> max_cpus);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_MAX_CPUS), ==,
> max_cpus);
>      g_free(fw_cfg);
>      qtest_quit(s);
>  }
> @@ -154,15 +154,15 @@ static void test_fw_cfg_numa(const void *opaque)
>      uint64_t *node_mask;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
> +    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_NUMA), ==,
> nb_nodes);
>
>      cpu_mask = g_new0(uint64_t, max_cpus);
>      node_mask = g_new0(uint64_t, nb_nodes);
>
> -    qfw_cfg_read_data(fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
> -    qfw_cfg_read_data(fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
> +    qfw_cfg_read_data(s, fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
> +    qfw_cfg_read_data(s, fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
>
>      if (nb_nodes) {
>          g_assert(cpu_mask[0] & 0x01);
> @@ -182,9 +182,10 @@ static void test_fw_cfg_boot_menu(const void *opaque)
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==,
> boot_menu);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_BOOT_MENU),
> +                    ==, boot_menu);
>      g_free(fw_cfg);
>      qtest_quit(s);
>  }
> @@ -198,9 +199,9 @@ static void test_fw_cfg_reboot_timeout(const void
> *opaque)
>      size_t filesize;
>
>      s = qtest_initf("-M %s -boot reboot-timeout=15", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> +    filesize = qfw_cfg_get_file(s, 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);
> @@ -218,9 +219,9 @@ static void test_fw_cfg_splash_time(const void *opaque)
>      size_t filesize;
>
>      s = qtest_initf("-M %s -boot splash-time=12", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init(s);
> +    fw_cfg = pc_fw_cfg_init();
>
> -    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-menu-wait",
> +    filesize = qfw_cfg_get_file(s, 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);
> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
> index ddeec821db..d25a367194 100644
> --- a/tests/libqos/fw_cfg.c
> +++ b/tests/libqos/fw_cfg.c
> @@ -18,46 +18,47 @@
>  #include "qemu/bswap.h"
>  #include "hw/nvram/fw_cfg.h"
>
> -void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
> +void qfw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
>  {
> -    fw_cfg->select(fw_cfg, key);
> +    fw_cfg->select(qts, fw_cfg, key);
>  }
>
> -void qfw_cfg_read_data(QFWCFG *fw_cfg, void *data, size_t len)
> +void qfw_cfg_read_data(QTestState *qts, QFWCFG *fw_cfg, void *data,
> size_t len)
>  {
> -    fw_cfg->read(fw_cfg, data, len);
> +    fw_cfg->read(qts, fw_cfg, data, len);
>  }
>
> -void qfw_cfg_get(QFWCFG *fw_cfg, uint16_t key, void *data, size_t len)
> +void qfw_cfg_get(QTestState *qts, QFWCFG *fw_cfg, uint16_t key,
> +                 void *data, size_t len)
>  {
> -    qfw_cfg_select(fw_cfg, key);
> -    qfw_cfg_read_data(fw_cfg, data, len);
> +    qfw_cfg_select(qts, fw_cfg, key);
> +    qfw_cfg_read_data(qts, fw_cfg, data, len);
>  }
>
> -uint16_t qfw_cfg_get_u16(QFWCFG *fw_cfg, uint16_t key)
> +uint16_t qfw_cfg_get_u16(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
>  {
>      uint16_t value;
> -    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
> +    qfw_cfg_get(qts, fw_cfg, key, &value, sizeof(value));
>      return le16_to_cpu(value);
>  }
>
> -uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key)
> +uint32_t qfw_cfg_get_u32(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
>  {
>      uint32_t value;
> -    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
> +    qfw_cfg_get(qts, fw_cfg, key, &value, sizeof(value));
>      return le32_to_cpu(value);
>  }
>
> -uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key)
> +uint64_t qfw_cfg_get_u64(QTestState *qts, QFWCFG *fw_cfg, uint16_t key)
>  {
>      uint64_t value;
> -    qfw_cfg_get(fw_cfg, key, &value, sizeof(value));
> +    qfw_cfg_get(qts, fw_cfg, key, &value, sizeof(value));
>      return le64_to_cpu(value);
>  }
>
> -static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
> +static void mm_fw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t
> key)
>  {
> -    qtest_writew(fw_cfg->qts, fw_cfg->base, key);
> +    qtest_writew(qts, fw_cfg->base, key);
>  }
>
>  /*
> @@ -72,8 +73,8 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t
> key)
>   * 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)
> +size_t qfw_cfg_get_file(QTestState *qts, QFWCFG *fw_cfg, const char
> *filename,
> +                        void *data, size_t buflen)
>  {
>      uint32_t count;
>      uint32_t i;
> @@ -82,11 +83,11 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
> *filename,
>      FWCfgFile *pdir_entry;
>      size_t filesize = 0;
>
> -    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
> +    qfw_cfg_get(qts, 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);
> +    qfw_cfg_get(qts, 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)) {
> @@ -96,7 +97,7 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
> *filename,
>              if (len > buflen) {
>                  len = buflen;
>              }
> -            qfw_cfg_get(fw_cfg, sel, data, len);
> +            qfw_cfg_get(qts, fw_cfg, sel, data, len);
>              break;
>          }
>      }
> @@ -104,49 +105,49 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
> *filename,
>      return filesize;
>  }
>
> -static void mm_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
> +static void mm_fw_cfg_read(QTestState *qts, QFWCFG *fw_cfg,
> +                           void *data, size_t len)
>  {
>      uint8_t *ptr = data;
>      int i;
>
>      for (i = 0; i < len; i++) {
> -        ptr[i] = qtest_readb(fw_cfg->qts, fw_cfg->base + 2);
> +        ptr[i] = qtest_readb(qts, fw_cfg->base + 2);
>      }
>  }
>
> -QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base)
> +QFWCFG *mm_fw_cfg_init(uint64_t base)
>  {
>      QFWCFG *fw_cfg = g_malloc0(sizeof(*fw_cfg));
>
>      fw_cfg->base = base;
> -    fw_cfg->qts = qts;
>      fw_cfg->select = mm_fw_cfg_select;
>      fw_cfg->read = mm_fw_cfg_read;
>
>      return fw_cfg;
>  }
>
> -static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
> +static void io_fw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t
> key)
>  {
> -    qtest_outw(fw_cfg->qts, fw_cfg->base, key);
> +    qtest_outw(qts, fw_cfg->base, key);
>  }
>
> -static void io_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
> +static void io_fw_cfg_read(QTestState *qts, QFWCFG *fw_cfg,
> +                           void *data, size_t len)
>  {
>      uint8_t *ptr = data;
>      int i;
>
>      for (i = 0; i < len; i++) {
> -        ptr[i] = qtest_inb(fw_cfg->qts, fw_cfg->base + 1);
> +        ptr[i] = qtest_inb(qts, fw_cfg->base + 1);
>      }
>  }
>
> -QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base)
> +QFWCFG *io_fw_cfg_init(uint16_t base)
>  {
>      QFWCFG *fw_cfg = g_malloc0(sizeof(*fw_cfg));
>
>      fw_cfg->base = base;
> -    fw_cfg->qts = qts;
>      fw_cfg->select = io_fw_cfg_select;
>      fw_cfg->read = io_fw_cfg_read;
>
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index 6316f4c354..f9e69be450 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -20,52 +20,49 @@ typedef struct QFWCFG QFWCFG;
>  struct QFWCFG
>  {
>      uint64_t base;
> -    QTestState *qts;
> -    void (*select)(QFWCFG *fw_cfg, uint16_t key);
> -    void (*read)(QFWCFG *fw_cfg, void *data, size_t len);
> +    void (*select)(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
> +    void (*read)(QTestState *qts, QFWCFG *fw_cfg, void *data, size_t len);
>  };
>
> -void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key);
> -void qfw_cfg_read_data(QFWCFG *fw_cfg, void *data, size_t len);
> -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 qfw_cfg_select(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
> +void qfw_cfg_read_data(QTestState *qts, QFWCFG *fw_cfg, void *data,
> size_t len);
> +void qfw_cfg_get(QTestState *qts, QFWCFG *fw_cfg, uint16_t key,
> +                 void *data, size_t len);
> +uint16_t qfw_cfg_get_u16(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
> +uint32_t qfw_cfg_get_u32(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
> +uint64_t qfw_cfg_get_u64(QTestState *qts, QFWCFG *fw_cfg, uint16_t key);
> +size_t qfw_cfg_get_file(QTestState *qts, QFWCFG *fw_cfg, const char
> *filename,
>                          void *data, size_t buflen);
>
>  /**
>   * mm_fw_cfg_init():
> - * @qts: The #QTestState that will be referred to by the QFWCFG object.
>   * @base: The MMIO base address of the fw_cfg device in the guest.
>   *
>   * Returns a newly allocated QFWCFG object which must be released
>   * with a call to g_free() when no longer required.
>   */
> -QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
> +QFWCFG *mm_fw_cfg_init(uint64_t base);
>
>  /**
>   * io_fw_cfg_init():
> - * @qts: The #QTestState that will be referred to by the QFWCFG object.
>   * @base: The I/O address of the fw_cfg device in the guest.
>   *
>   * Returns a newly allocated QFWCFG object which must be released
>   * with a call to g_free() when no longer required.
>   */
> -QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
> +QFWCFG *io_fw_cfg_init(uint16_t base);
>
>  /**
>   * pc_fw_cfg_init():
> - * @qts: The #QTestState that will be referred to by the QFWCFG object.
>   *
>   * This function is specific to the PC machine (X86).
>   *
>   * Returns a newly allocated QFWCFG object which must be released
>   * with a call to g_free() when no longer required.
>   */
> -static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
> +static inline QFWCFG *pc_fw_cfg_init(void)
>  {
> -    return io_fw_cfg_init(qts, 0x510);
> +    return io_fw_cfg_init(0x510);
>  }
>
>  #endif
> diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
> index 949a99361d..4932ae092d 100644
> --- a/tests/libqos/malloc-pc.c
> +++ b/tests/libqos/malloc-pc.c
> @@ -23,9 +23,9 @@
>  void pc_alloc_init(QGuestAllocator *s, QTestState *qts, QAllocOpts flags)
>  {
>      uint64_t ram_size;
> -    QFWCFG *fw_cfg = pc_fw_cfg_init(qts);
> +    QFWCFG *fw_cfg = pc_fw_cfg_init();
>
> -    ram_size = qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE);
> +    ram_size = qfw_cfg_get_u64(qts, fw_cfg, FW_CFG_RAM_SIZE);
>      alloc_init(s, flags, 1 << 20, MIN(ram_size, 0xE0000000), PAGE_SIZE);
>
>      /* clean-up */
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 20245 bytes --]

Re: [PATCH v2 6/7] tests/fw_cfg: Declare one QFWCFG for all tests

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 8395 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道：

> It is pointless to create/remove a QFWCFG object for each test.
> Move it to the test context and create/remove it only once.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  tests/fw_cfg-test.c | 80 ++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 48 deletions(-)
>
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index dda9a9fb07..35af0de7e6 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -25,47 +25,42 @@ static uint16_t boot_menu = 0;
>
>  typedef struct {
>      const char *machine_name;
> +    QFWCFG *fw_cfg;
>  } QTestCtx;
>
>  static void test_fw_cfg_signature(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>      char buf[5];
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    qfw_cfg_get(s, fw_cfg, FW_CFG_SIGNATURE, buf, 4);
> +    qfw_cfg_get(s, ctx->fw_cfg, FW_CFG_SIGNATURE, buf, 4);
>      buf[4] = 0;
> -
>      g_assert_cmpstr(buf, ==, "QEMU");
> -    g_free(fw_cfg);
> +
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_id(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>      uint32_t id;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    id = qfw_cfg_get_u32(s, fw_cfg, FW_CFG_ID);
> +    id = qfw_cfg_get_u32(s, ctx->fw_cfg, FW_CFG_ID);
>      g_assert((id == 1) ||
>               (id == 3));
> -    g_free(fw_cfg);
> +
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_uuid(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>
>      uint8_t buf[16];
> @@ -76,12 +71,10 @@ static void test_fw_cfg_uuid(const void *opaque)
>
>      s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8",
>                      ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    qfw_cfg_get(s, fw_cfg, FW_CFG_UUID, buf, 16);
> +    qfw_cfg_get(s, ctx->fw_cfg, FW_CFG_UUID, buf, 16);
>      g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
>
> -    g_free(fw_cfg);
>      qtest_quit(s);
>
>  }
> @@ -89,80 +82,71 @@ static void test_fw_cfg_uuid(const void *opaque)
>  static void test_fw_cfg_ram_size(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_RAM_SIZE), ==,
> ram_size);
> +    g_assert_cmpint(qfw_cfg_get_u64(s, ctx->fw_cfg, FW_CFG_RAM_SIZE),
> +                    ==, ram_size);
>
> -    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_nographic(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_NOGRAPHIC),
> ==, 0);
>
> -    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_nb_cpus(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_NB_CPUS), ==,
> nb_cpus);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_NB_CPUS),
> +                    ==, nb_cpus);
>
> -    g_free(fw_cfg);
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_max_cpus(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_MAX_CPUS), ==,
> max_cpus);
> -    g_free(fw_cfg);
> +    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_MAX_CPUS),
> +                    ==, max_cpus);
> +
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_numa(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>      uint64_t *cpu_mask;
>      uint64_t *node_mask;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u64(s, fw_cfg, FW_CFG_NUMA), ==,
> nb_nodes);
> +    g_assert_cmpint(qfw_cfg_get_u64(s, ctx->fw_cfg, FW_CFG_NUMA),
> +                    ==, nb_nodes);
>
>      cpu_mask = g_new0(uint64_t, max_cpus);
>      node_mask = g_new0(uint64_t, nb_nodes);
>
> -    qfw_cfg_read_data(s, fw_cfg, cpu_mask, sizeof(uint64_t) * max_cpus);
> -    qfw_cfg_read_data(s, fw_cfg, node_mask, sizeof(uint64_t) * nb_nodes);
> +    qfw_cfg_read_data(s, ctx->fw_cfg, cpu_mask, sizeof(uint64_t) *
> max_cpus);
> +    qfw_cfg_read_data(s, ctx->fw_cfg, node_mask, sizeof(uint64_t) *
> nb_nodes);
>
>      if (nb_nodes) {
>          g_assert(cpu_mask[0] & 0x01);
> @@ -171,72 +155,68 @@ static void test_fw_cfg_numa(const void *opaque)
>
>      g_free(node_mask);
>      g_free(cpu_mask);
> -    g_free(fw_cfg);
> +
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_boot_menu(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>
>      s = qtest_initf("-M %s", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    g_assert_cmpint(qfw_cfg_get_u16(s, fw_cfg, FW_CFG_BOOT_MENU),
> +    g_assert_cmpint(qfw_cfg_get_u16(s, ctx->fw_cfg, FW_CFG_BOOT_MENU),
>                      ==, boot_menu);
> -    g_free(fw_cfg);
> +
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_reboot_timeout(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>      uint32_t reboot_timeout = 0;
>      size_t filesize;
>
>      s = qtest_initf("-M %s -boot reboot-timeout=15", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    filesize = qfw_cfg_get_file(s, fw_cfg, "etc/boot-fail-wait",
> +    filesize = qfw_cfg_get_file(s, ctx->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);
> -    g_free(fw_cfg);
> +
>      qtest_quit(s);
>  }
>
>  static void test_fw_cfg_splash_time(const void *opaque)
>  {
>      const QTestCtx *ctx = opaque;
> -    QFWCFG *fw_cfg;
>      QTestState *s;
>      uint16_t splash_time = 0;
>      size_t filesize;
>
>      s = qtest_initf("-M %s -boot splash-time=12", ctx->machine_name);
> -    fw_cfg = pc_fw_cfg_init();
>
> -    filesize = qfw_cfg_get_file(s, fw_cfg, "etc/boot-menu-wait",
> +    filesize = qfw_cfg_get_file(s, ctx->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);
> -    g_free(fw_cfg);
> +
>      qtest_quit(s);
>  }
>
>  int main(int argc, char **argv)
>  {
>      QTestCtx ctx;
> +    int ret;
>
>      g_test_init(&argc, &argv, NULL);
>
>      ctx.machine_name = "pc";
> +    ctx.fw_cfg = pc_fw_cfg_init();
>
>      qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
>      qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
> @@ -257,5 +237,9 @@ int main(int argc, char **argv)
>                          test_fw_cfg_reboot_timeout);
>      qtest_add_data_func("fw_cfg/splash_time", &ctx,
> test_fw_cfg_splash_time);
>
> -    return g_test_run();
> +    ret = g_test_run();
> +
> +    g_free(ctx.fw_cfg);
> +
> +    return ret;
>  }
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 10436 bytes --]

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 4281 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道：

> We have been restricting our fw_cfg tests to the PC machine,
> which is a little-endian architecture.
> The fw_cfg device is also used on the SPARC and PowerPC
> architectures, which can run in big-endian configuration.
>
> Since we want to be sure our device does not regress
> regardless the endianess used, enable this test one
> these targets.
>
> The NUMA selector is X86 specific, restrict it to this arch.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: test ppc32 too (lvivier)
> ---
>  tests/Makefile.include |  2 ++
>  tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 3543451ed3..4ae3d5140a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>  check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
>  check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
>  check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
> +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
>
>  check-qtest-ppc64-y += $(check-qtest-ppc-y)
>  check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF)
> @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) =
> tests/endianness-test$(EXESUF)
>  check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>  check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>  check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
>
>  check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
>  check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 35af0de7e6..1250e87097 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void
> *opaque)
>
>  int main(int argc, char **argv)
>  {
> -    QTestCtx ctx;
> -    int ret;
> +    const char *arch = qtest_get_arch();
> +    bool has_numa = false;
> +    QTestCtx ctx = {};
> +    int ret = 0;
>
>      g_test_init(&argc, &argv, NULL);
>
> -    ctx.machine_name = "pc";
> -    ctx.fw_cfg = pc_fw_cfg_init();
> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> +        has_numa = true;
> +        ctx.machine_name = "pc";
> +        ctx.fw_cfg = pc_fw_cfg_init();
> +    } else if (g_str_equal(arch, "sparc")) {
> +        ctx.machine_name = "SS-5";
> +        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
> +    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) {
> +        /*
> +         * The mac99 machine is different for 32/64-bit target:
> +         *
> +         * ppc(32): the G4 which can be either little or big endian,
> +         * ppc64:   the G5 (970FX) is only big-endian.
> +         */
> +        ctx.machine_name = "mac99";
> +        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
> +    }
>
>      qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
>      qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
> @@ -231,14 +248,18 @@ int main(int argc, char **argv)
>      qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
>  #endif
>      qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
> -    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
>      qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu);
>      qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
>                          test_fw_cfg_reboot_timeout);
>      qtest_add_data_func("fw_cfg/splash_time", &ctx,
> test_fw_cfg_splash_time);
>
> -    ret = g_test_run();
> +    if (has_numa) {
> +        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> +    }
>
> +    if (ctx.machine_name) {
> +        ret = g_test_run();
> +    }
>

I think we can omit this if statement. In which case the ctx.machine_name
will be NULL?

Thanks,
Li Qiang



>      g_free(ctx.fw_cfg);
>
>      return ret;
> --
> 2.21.0
>
>

[-- Attachment #2: Type: text/html, Size: 5441 bytes --]

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Philippe Mathieu-Daudé]

Hi Li,

On 10/8/19 5:04 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 
> 2019年10月7日周一 下午11:20写道：
> 
>     We have been restricting our fw_cfg tests to the PC machine,
>     which is a little-endian architecture.
>     The fw_cfg device is also used on the SPARC and PowerPC
>     architectures, which can run in big-endian configuration.
> 
>     Since we want to be sure our device does not regress
>     regardless the endianess used, enable this test one
>     these targets.
> 
>     The NUMA selector is X86 specific, restrict it to this arch.
> 
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>     v2: test ppc32 too (lvivier)
>     ---
>       tests/Makefile.include |  2 ++
>       tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
>       2 files changed, 29 insertions(+), 6 deletions(-)
> 
>     diff --git a/tests/Makefile.include b/tests/Makefile.include
>     index 3543451ed3..4ae3d5140a 100644
>     --- a/tests/Makefile.include
>     +++ b/tests/Makefile.include
>     @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>       check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
>       check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
>       check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
>     +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
> 
>       check-qtest-ppc64-y += $(check-qtest-ppc-y)
>       check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF)
>     @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) =
>     tests/endianness-test$(EXESUF)
>       check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>       check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>       check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
>     +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
> 
>       check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) =
>     tests/endianness-test$(EXESUF)
>       check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>     diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
>     index 35af0de7e6..1250e87097 100644
>     --- a/tests/fw_cfg-test.c
>     +++ b/tests/fw_cfg-test.c
>     @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void
>     *opaque)
> 
>       int main(int argc, char **argv)
>       {
>     -    QTestCtx ctx;
>     -    int ret;
>     +    const char *arch = qtest_get_arch();
>     +    bool has_numa = false;
>     +    QTestCtx ctx = {};
>     +    int ret = 0;
> 
>           g_test_init(&argc, &argv, NULL);
> 
>     -    ctx.machine_name = "pc";
>     -    ctx.fw_cfg = pc_fw_cfg_init();
>     +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
>     +        has_numa = true;
>     +        ctx.machine_name = "pc";
>     +        ctx.fw_cfg = pc_fw_cfg_init();
>     +    } else if (g_str_equal(arch, "sparc")) {
>     +        ctx.machine_name = "SS-5";
>     +        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
>     +    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch,
>     "ppc64")) {
>     +        /*
>     +         * The mac99 machine is different for 32/64-bit target:
>     +         *
>     +         * ppc(32): the G4 which can be either little or big endian,
>     +         * ppc64:   the G5 (970FX) is only big-endian.
>     +         */
>     +        ctx.machine_name = "mac99";
>     +        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
>     +    }
> 
>           qtest_add_data_func("fw_cfg/signature", &ctx,
>     test_fw_cfg_signature);
>           qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
>     @@ -231,14 +248,18 @@ int main(int argc, char **argv)
>           qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
>       #endif
>           qtest_add_data_func("fw_cfg/max_cpus", &ctx,
>     test_fw_cfg_max_cpus);
>     -    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
>           qtest_add_data_func("fw_cfg/boot_menu", &ctx,
>     test_fw_cfg_boot_menu);
>           qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
>                               test_fw_cfg_reboot_timeout);
>           qtest_add_data_func("fw_cfg/splash_time", &ctx,
>     test_fw_cfg_splash_time);
> 
>     -    ret = g_test_run();
>     +    if (has_numa) {
>     +        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
>     +    }
> 
>     +    if (ctx.machine_name) {
>     +        ret = g_test_run();
>     +    }
> 
> 
> I think we can omit this if statement. In which case the 
> ctx.machine_name will be NULL?

Here I thought about the PPC64 tests inheriting the PPC32 ones, and 
maybe someone update the tests/Makefile.include and this test will run 
on unexpected architectures. So if the machine is NULL (another arch) we 
don't crash and return successfully, so the testsuite continue.

I might add a comment such:

   if (ctx.machine_name) {
       /* Only run whitelisted architecture. */
       ret = g_test_run();
   }

But maybe it is simpler to do at the beginning of main():

   if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
   ...
       ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
   } else {
       return 0;
   }

What do you think?

Thanks for reviewing the whole series :)

> Thanks,
> Li Qiang
> 
>           g_free(ctx.fw_cfg);
> 
>           return ret;
>     -- 
>     2.21.0
> 

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 6137 bytes --]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月8日周二 下午11:14写道：

> Hi Li,
>
> On 10/8/19 5:04 PM, Li Qiang wrote:
> > Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> > 2019年10月7日周一 下午11:20写道：
> >
> >     We have been restricting our fw_cfg tests to the PC machine,
> >     which is a little-endian architecture.
> >     The fw_cfg device is also used on the SPARC and PowerPC
> >     architectures, which can run in big-endian configuration.
> >
> >     Since we want to be sure our device does not regress
> >     regardless the endianess used, enable this test one
> >     these targets.
> >
> >     The NUMA selector is X86 specific, restrict it to this arch.
> >
> >     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
> >     <mailto:philmd@redhat.com>>
> >     ---
> >     v2: test ppc32 too (lvivier)
> >     ---
> >       tests/Makefile.include |  2 ++
> >       tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
> >       2 files changed, 29 insertions(+), 6 deletions(-)
> >
> >     diff --git a/tests/Makefile.include b/tests/Makefile.include
> >     index 3543451ed3..4ae3d5140a 100644
> >     --- a/tests/Makefile.include
> >     +++ b/tests/Makefile.include
> >     @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >       check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
> >       check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
> >       check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
> >     +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
> >
> >       check-qtest-ppc64-y += $(check-qtest-ppc-y)
> >       check-qtest-ppc64-$(CONFIG_PSERIES) +=
> tests/device-plug-test$(EXESUF)
> >     @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) =
> >     tests/endianness-test$(EXESUF)
> >       check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> >       check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
> >       check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
> >     +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
> >
> >       check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) =
> >     tests/endianness-test$(EXESUF)
> >       check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> >     diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> >     index 35af0de7e6..1250e87097 100644
> >     --- a/tests/fw_cfg-test.c
> >     +++ b/tests/fw_cfg-test.c
> >     @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void
> >     *opaque)
> >
> >       int main(int argc, char **argv)
> >       {
> >     -    QTestCtx ctx;
> >     -    int ret;
> >     +    const char *arch = qtest_get_arch();
> >     +    bool has_numa = false;
> >     +    QTestCtx ctx = {};
> >     +    int ret = 0;
> >
> >           g_test_init(&argc, &argv, NULL);
> >
> >     -    ctx.machine_name = "pc";
> >     -    ctx.fw_cfg = pc_fw_cfg_init();
> >     +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> >     +        has_numa = true;
> >     +        ctx.machine_name = "pc";
> >     +        ctx.fw_cfg = pc_fw_cfg_init();
> >     +    } else if (g_str_equal(arch, "sparc")) {
> >     +        ctx.machine_name = "SS-5";
> >     +        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
> >     +    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch,
> >     "ppc64")) {
> >     +        /*
> >     +         * The mac99 machine is different for 32/64-bit target:
> >     +         *
> >     +         * ppc(32): the G4 which can be either little or big endian,
> >     +         * ppc64:   the G5 (970FX) is only big-endian.
> >     +         */
> >     +        ctx.machine_name = "mac99";
> >     +        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
> >     +    }
> >
> >           qtest_add_data_func("fw_cfg/signature", &ctx,
> >     test_fw_cfg_signature);
> >           qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
> >     @@ -231,14 +248,18 @@ int main(int argc, char **argv)
> >           qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
> >       #endif
> >           qtest_add_data_func("fw_cfg/max_cpus", &ctx,
> >     test_fw_cfg_max_cpus);
> >     -    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> >           qtest_add_data_func("fw_cfg/boot_menu", &ctx,
> >     test_fw_cfg_boot_menu);
> >           qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
> >                               test_fw_cfg_reboot_timeout);
> >           qtest_add_data_func("fw_cfg/splash_time", &ctx,
> >     test_fw_cfg_splash_time);
> >
> >     -    ret = g_test_run();
> >     +    if (has_numa) {
> >     +        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> >     +    }
> >
> >     +    if (ctx.machine_name) {
> >     +        ret = g_test_run();
> >     +    }
> >
> >
> > I think we can omit this if statement. In which case the
> > ctx.machine_name will be NULL?
>
> Here I thought about the PPC64 tests inheriting the PPC32 ones, and
> maybe someone update the tests/Makefile.include and this test will run
> on unexpected architectures.


Sorry, I don't get your point here(PPC64 tests inheriting the PPC32),
could you please explain this more?



> So if the machine is NULL (another arch) we
>

IIUC the "-M" option must has an argument, maybe?

Thanks,
Li Qiang



> don't crash and return successfully, so the testsuite continue.
>
> I might add a comment such:
>
>    if (ctx.machine_name) {
>        /* Only run whitelisted architecture. */
>        ret = g_test_run();
>    }
>
> But maybe it is simpler to do at the beginning of main():
>
>    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
>    ...
>        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
>    } else {
>        return 0;
>    }
>
> What do you think?
>
> Thanks for reviewing the whole series :)
>
> > Thanks,
> > Li Qiang
> >
> >           g_free(ctx.fw_cfg);
> >
> >           return ret;
> >     --
> >     2.21.0
> >
>

[-- Attachment #2: Type: text/html, Size: 8505 bytes --]

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Laszlo Ersek]

On 10/08/19 17:04, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道：
> 
>> We have been restricting our fw_cfg tests to the PC machine,
>> which is a little-endian architecture.
>> The fw_cfg device is also used on the SPARC and PowerPC
>> architectures, which can run in big-endian configuration.
>>
>> Since we want to be sure our device does not regress
>> regardless the endianess used, enable this test one
>> these targets.
>>
>> The NUMA selector is X86 specific, restrict it to this arch.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v2: test ppc32 too (lvivier)
>> ---
>>  tests/Makefile.include |  2 ++
>>  tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
>>  2 files changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 3543451ed3..4ae3d5140a 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>>  check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
>>  check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
>>  check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
>> +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
>>
>>  check-qtest-ppc64-y += $(check-qtest-ppc-y)
>>  check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF)
>> @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) =
>> tests/endianness-test$(EXESUF)
>>  check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>>  check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>>  check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
>> +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
>>
>>  check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
>>  check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
>> index 35af0de7e6..1250e87097 100644
>> --- a/tests/fw_cfg-test.c
>> +++ b/tests/fw_cfg-test.c
>> @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void
>> *opaque)
>>
>>  int main(int argc, char **argv)
>>  {
>> -    QTestCtx ctx;
>> -    int ret;
>> +    const char *arch = qtest_get_arch();
>> +    bool has_numa = false;
>> +    QTestCtx ctx = {};
>> +    int ret = 0;
>>
>>      g_test_init(&argc, &argv, NULL);
>>
>> -    ctx.machine_name = "pc";
>> -    ctx.fw_cfg = pc_fw_cfg_init();
>> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
>> +        has_numa = true;
>> +        ctx.machine_name = "pc";
>> +        ctx.fw_cfg = pc_fw_cfg_init();
>> +    } else if (g_str_equal(arch, "sparc")) {
>> +        ctx.machine_name = "SS-5";
>> +        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
>> +    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64")) {
>> +        /*
>> +         * The mac99 machine is different for 32/64-bit target:
>> +         *
>> +         * ppc(32): the G4 which can be either little or big endian,
>> +         * ppc64:   the G5 (970FX) is only big-endian.
>> +         */
>> +        ctx.machine_name = "mac99";
>> +        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
>> +    }
>>
>>      qtest_add_data_func("fw_cfg/signature", &ctx, test_fw_cfg_signature);
>>      qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
>> @@ -231,14 +248,18 @@ int main(int argc, char **argv)
>>      qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
>>  #endif
>>      qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
>> -    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
>>      qtest_add_data_func("fw_cfg/boot_menu", &ctx, test_fw_cfg_boot_menu);
>>      qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
>>                          test_fw_cfg_reboot_timeout);
>>      qtest_add_data_func("fw_cfg/splash_time", &ctx,
>> test_fw_cfg_splash_time);
>>
>> -    ret = g_test_run();
>> +    if (has_numa) {
>> +        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
>> +    }
>>
>> +    if (ctx.machine_name) {
>> +        ret = g_test_run();
>> +    }
>>
> 
> I think we can omit this if statement. In which case the ctx.machine_name
> will be NULL?

When "arch" differs from all of i386, x86_64, sparc, ppc, ppc64.

In that case, the original initializer will remain in effect, from:

  QTestCtx ctx = {};

(Admittedly, this is an ugly GNU-ism; for standard C, it should be

  QTestCtx ctx = { 0 };

but the GNU-ism is used quite frequently in QEMU elsewhere, so meh :) )

Thanks
Laszlo

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Peter Maydell]

On Tue, 8 Oct 2019 at 21:29, Laszlo Ersek <lersek@redhat.com> wrote:
> In that case, the original initializer will remain in effect, from:
>
>   QTestCtx ctx = {};
>
> (Admittedly, this is an ugly GNU-ism; for standard C, it should be
>
>   QTestCtx ctx = { 0 };
>
> but the GNU-ism is used quite frequently in QEMU elsewhere, so meh :) )

This is deliberate -- some compilers grumble about "{ 0 }" in
some situations, so we generally recommend using "{}" which
is accepted by all compilers we care about. (Cf commit message
for commit 039d4e3df0049bdd.)

thanks
-- PMM

Re: [PATCH v2 7/7] tests/fw_cfg: Run the tests on big-endian targets

[Li Qiang]

[-- Attachment #1: Type: text/plain, Size: 5341 bytes --]

Laszlo Ersek <lersek@redhat.com> 于2019年10月9日周三 上午4:27写道：

> On 10/08/19 17:04, Li Qiang wrote:
> > Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月7日周一 下午11:20写道：
> >
> >> We have been restricting our fw_cfg tests to the PC machine,
> >> which is a little-endian architecture.
> >> The fw_cfg device is also used on the SPARC and PowerPC
> >> architectures, which can run in big-endian configuration.
> >>
> >> Since we want to be sure our device does not regress
> >> regardless the endianess used, enable this test one
> >> these targets.
> >>
> >> The NUMA selector is X86 specific, restrict it to this arch.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> v2: test ppc32 too (lvivier)
> >> ---
> >>  tests/Makefile.include |  2 ++
> >>  tests/fw_cfg-test.c    | 33 +++++++++++++++++++++++++++------
> >>  2 files changed, 29 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 3543451ed3..4ae3d5140a 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -226,6 +226,7 @@ check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >>  check-qtest-ppc-y += tests/drive_del-test$(EXESUF)
> >>  check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
> >>  check-qtest-ppc-$(CONFIG_M48T59) += tests/m48t59-test$(EXESUF)
> >> +check-qtest-ppc-y += tests/fw_cfg-test$(EXESUF)
> >>
> >>  check-qtest-ppc64-y += $(check-qtest-ppc-y)
> >>  check-qtest-ppc64-$(CONFIG_PSERIES) += tests/device-plug-test$(EXESUF)
> >> @@ -250,6 +251,7 @@ check-qtest-sh4eb-$(CONFIG_ISA_TESTDEV) =
> >> tests/endianness-test$(EXESUF)
> >>  check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> >>  check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
> >>  check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
> >> +check-qtest-sparc-y += tests/fw_cfg-test$(EXESUF)
> >>
> >>  check-qtest-sparc64-$(CONFIG_ISA_TESTDEV) =
> tests/endianness-test$(EXESUF)
> >>  check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> >> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> >> index 35af0de7e6..1250e87097 100644
> >> --- a/tests/fw_cfg-test.c
> >> +++ b/tests/fw_cfg-test.c
> >> @@ -210,13 +210,30 @@ static void test_fw_cfg_splash_time(const void
> >> *opaque)
> >>
> >>  int main(int argc, char **argv)
> >>  {
> >> -    QTestCtx ctx;
> >> -    int ret;
> >> +    const char *arch = qtest_get_arch();
> >> +    bool has_numa = false;
> >> +    QTestCtx ctx = {};
> >> +    int ret = 0;
> >>
> >>      g_test_init(&argc, &argv, NULL);
> >>
> >> -    ctx.machine_name = "pc";
> >> -    ctx.fw_cfg = pc_fw_cfg_init();
> >> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> >> +        has_numa = true;
> >> +        ctx.machine_name = "pc";
> >> +        ctx.fw_cfg = pc_fw_cfg_init();
> >> +    } else if (g_str_equal(arch, "sparc")) {
> >> +        ctx.machine_name = "SS-5";
> >> +        ctx.fw_cfg = mm_fw_cfg_init(0xd00000510ULL);
> >> +    } else if (g_str_equal(arch, "ppc") || g_str_equal(arch, "ppc64"))
> {
> >> +        /*
> >> +         * The mac99 machine is different for 32/64-bit target:
> >> +         *
> >> +         * ppc(32): the G4 which can be either little or big endian,
> >> +         * ppc64:   the G5 (970FX) is only big-endian.
> >> +         */
> >> +        ctx.machine_name = "mac99";
> >> +        ctx.fw_cfg = mm_fw_cfg_init(0xf0000510);
> >> +    }
> >>
> >>      qtest_add_data_func("fw_cfg/signature", &ctx,
> test_fw_cfg_signature);
> >>      qtest_add_data_func("fw_cfg/id", &ctx, test_fw_cfg_id);
> >> @@ -231,14 +248,18 @@ int main(int argc, char **argv)
> >>      qtest_add_func("fw_cfg/boot_device", test_fw_cfg_boot_device);
> >>  #endif
> >>      qtest_add_data_func("fw_cfg/max_cpus", &ctx, test_fw_cfg_max_cpus);
> >> -    qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> >>      qtest_add_data_func("fw_cfg/boot_menu", &ctx,
> test_fw_cfg_boot_menu);
> >>      qtest_add_data_func("fw_cfg/reboot_timeout", &ctx,
> >>                          test_fw_cfg_reboot_timeout);
> >>      qtest_add_data_func("fw_cfg/splash_time", &ctx,
> >> test_fw_cfg_splash_time);
> >>
> >> -    ret = g_test_run();
> >> +    if (has_numa) {
> >> +        qtest_add_data_func("fw_cfg/numa", &ctx, test_fw_cfg_numa);
> >> +    }
> >>
> >> +    if (ctx.machine_name) {
> >> +        ret = g_test_run();
> >> +    }
> >>
> >
> > I think we can omit this if statement. In which case the ctx.machine_name
> > will be NULL?
>
> When "arch" differs from all of i386, x86_64, sparc, ppc, ppc64.
>

Yes, I just found the setting 'arch' lost 'else' branch.
Then in this case, I prefer 'just return'.  There is no need do other
things for non-support arch.
This is just like some other case, such as usb-hcd-ehci-test.c and
ahci-test.c.
Also, I don't find any conditional 'g_test_run' calling.

Thanks,
Li Qiang



>
> In that case, the original initializer will remain in effect, from:
>
>   QTestCtx ctx = {};
>
> (Admittedly, this is an ugly GNU-ism; for standard C, it should be
>
>   QTestCtx ctx = { 0 };
>
> but the GNU-ism is used quite frequently in QEMU elsewhere, so meh :) )
>
> Thanks
> Laszlo
>

[-- Attachment #2: Type: text/html, Size: 7277 bytes --]