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

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

[Philippe Mathieu-Daudé]

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

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 [*], such:

  commit 36b62ae6a58f9a588fd33be9386e18a2b90103f5

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

[*] 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      | 180 +++++++++++++++++++++------------------
 tests/libqos/fw_cfg.c    |  71 +++++++--------
 tests/libqos/fw_cfg.h    |  55 +++++++-----
 tests/libqos/malloc-pc.c |   6 +-
 6 files changed, 176 insertions(+), 150 deletions(-)

-- 
2.20.1

[PATCH 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.

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

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

[Laszlo Ersek]

On 10/04/19 00:54, Philippe Mathieu-Daudé wrote:
> 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.
> 
> 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
> 


This more or less reverts 0729d833d6d6 ("tests/libqos: Add
io_fw_cfg_uninit() and mm_fw_cfg_uninit()", 2019-05-23).

I'm fine either way.

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

[PATCH 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.

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

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

[Laszlo Ersek]

On 10/04/19 00:54, Philippe Mathieu-Daudé wrote:
> Document mm_fw_cfg_init() return value must be released
> with g_free(). mm_fw_cfg_uninit() was never used, remove it.
> 
> 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.
> 

Sigh, we've never even called this function :/

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

[PATCH 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.

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

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

[Laszlo Ersek]

On 10/04/19 00:54, Philippe Mathieu-Daudé wrote:
> 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.
> 
> 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);
>  }
> 

I'm getting the impression we're trying to remove "PC" (x86) references
from the cleanup action :)

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

[PATCH 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).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c | 93 ++++++++++++++++++++++++++++-----------------
 1 file changed, 59 insertions(+), 34 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 53ae82f7c8..77a833d50e 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)
 {
+    QTestCtx *ctx = (QTestCtx *)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)
 {
+    QTestCtx *ctx = (QTestCtx *)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)
 {
+    QTestCtx *ctx = (QTestCtx *)opaque;
     QFWCFG *fw_cfg;
     QTestState *s;
 
@@ -67,7 +74,7 @@ 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 +85,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +100,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +115,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +130,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +144,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +174,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +188,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +208,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)
 {
+    QTestCtx *ctx = (QTestCtx *)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 +230,35 @@ static void test_fw_cfg_splash_time(void)
 
 int main(int argc, char **argv)
 {
+    QTestCtx *ctx = g_new(QTestCtx, 1);
+    int ret;
+
     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();
+    ret = g_test_run();
+
+    g_free(ctx);
+
+    return ret;
 }
-- 
2.20.1

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

[Laszlo Ersek]

On 10/04/19 00:54, Philippe Mathieu-Daudé wrote:
> 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).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/fw_cfg-test.c | 93 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 53ae82f7c8..77a833d50e 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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)opaque;

(1) I'd suggest:

    const QTestCtx *ctx = opaque;

(I think that should work fine with the current patch; not sure if it
will work with the rest of the series, when you perhaps introduce more
members to QTestCtx.)

This request applies a few more times to this patch, of course.

Another (different) comment:

>      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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)opaque;
>      QFWCFG *fw_cfg;
>      QTestState *s;
>  
> @@ -67,7 +74,7 @@ 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 +85,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +100,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +115,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +130,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +144,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +174,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +188,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +208,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)
>  {
> +    QTestCtx *ctx = (QTestCtx *)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 +230,35 @@ static void test_fw_cfg_splash_time(void)
>  
>  int main(int argc, char **argv)
>  {
> +    QTestCtx *ctx = g_new(QTestCtx, 1);
> +    int ret;
> +
>      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();
> +    ret = g_test_run();
> +
> +    g_free(ctx);
> +
> +    return ret;
>  }
> 

(2) You don't necessarily have to use g_new / g_free for ctx; I think
you could simply use an auto ("local") variable. But, maybe that doesn't
apply to the rest of the patches, and/or g_new is the idiomatic way,
with regard to other tests. So, up to you.

With an attempt to address (1), and regardless of (2):

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

Thanks
Laszlo

[PATCH 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.

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    | 30 +++++++++-----------
 tests/libqos/malloc-pc.c |  4 +--
 5 files changed, 77 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 77a833d50e..6fb52a4d08 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);
@@ -75,9 +75,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);
@@ -92,9 +92,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);
@@ -107,9 +107,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);
@@ -122,9 +122,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);
@@ -137,9 +137,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);
 }
@@ -153,15 +153,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);
@@ -181,9 +181,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);
 }
@@ -197,9 +198,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);
@@ -217,9 +218,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..e560cd02ac 100644
--- a/tests/libqos/fw_cfg.h
+++ b/tests/libqos/fw_cfg.h
@@ -20,52 +20,48 @@ 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.20.1

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

[Laszlo Ersek]

On 10/04/19 00:54, Philippe Mathieu-Daudé wrote:
> 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.
> 
> 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    | 30 +++++++++-----------
>  tests/libqos/malloc-pc.c |  4 +--
>  5 files changed, 77 insertions(+), 79 deletions(-)

Not much fun to review. :)

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

[PATCH 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.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/fw_cfg-test.c | 74 +++++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 47 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 6fb52a4d08..12dbaf4e67 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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)opaque;
-    QFWCFG *fw_cfg;
     QTestState *s;
 
     uint8_t buf[16];
@@ -75,12 +70,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);
 
 }
@@ -88,80 +81,71 @@ static void test_fw_cfg_uuid(const void *opaque)
 static void test_fw_cfg_ram_size(const void *opaque)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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);
@@ -170,62 +154,56 @@ 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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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)
 {
     QTestCtx *ctx = (QTestCtx *)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);
 }
 
@@ -237,6 +215,7 @@ int main(int argc, char **argv)
     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);
@@ -259,6 +238,7 @@ int main(int argc, char **argv)
 
     ret = g_test_run();
 
+    g_free(ctx->fw_cfg);
     g_free(ctx);
 
     return ret;
-- 
2.20.1

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

[Laszlo Ersek]

On 10/04/19 00:54, Philippe Mathieu-Daudé wrote:
> It is pointless to create/remove a QFWCFG object for each test.
> Move it to the test context and create/remove it only once.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/fw_cfg-test.c | 74 +++++++++++++++++----------------------------
>  1 file changed, 27 insertions(+), 47 deletions(-)

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

[PATCH 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>
---
 tests/Makefile.include |  2 ++
 tests/fw_cfg-test.c    | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3543451ed3..322bdb36ff 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) += tests/ivshmem-test$(EXESUF)
 check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
 
 check-qtest-sh4-$(CONFIG_ISA_TESTDEV) = tests/endianness-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 12dbaf4e67..39bbc9647e 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -209,13 +209,22 @@ static void test_fw_cfg_splash_time(const void *opaque)
 
 int main(int argc, char **argv)
 {
+    const char *arch = qtest_get_arch();
     QTestCtx *ctx = g_new(QTestCtx, 1);
     int ret;
 
     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")) {
+        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, "ppc64")) {
+        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);
@@ -230,12 +239,15 @@ 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);
 
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
+        qtest_add_data_func("fw_cfg/numa", ctx, test_fw_cfg_numa);
+    }
+
     ret = g_test_run();
 
     g_free(ctx->fw_cfg);
-- 
2.20.1

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

[Laurent Vivier]

On 04/10/2019 00:54, 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>
> ---
>  tests/Makefile.include |  2 ++
>  tests/fw_cfg-test.c    | 18 +++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 3543451ed3..322bdb36ff 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) += tests/display-vga-test$(EXESUF)
>  check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>  check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) += tests/ivshmem-test$(EXESUF)
>  check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
> +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)

Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
(mac99, g3beige and prep) does, so perhaps you should rather add the
test to check-qtest-ppc-y (and it will be inherited by ppc64)?

Thanks,
Laurent

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

[Philippe Mathieu-Daudé]

On 10/4/19 10:05 AM, Laurent Vivier wrote:
> On 04/10/2019 00:54, 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>
>> ---
>>   tests/Makefile.include |  2 ++
>>   tests/fw_cfg-test.c    | 18 +++++++++++++++---
>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 3543451ed3..322bdb36ff 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) += tests/display-vga-test$(EXESUF)
>>   check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>>   check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) += tests/ivshmem-test$(EXESUF)
>>   check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
> 
> Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
> (mac99, g3beige and prep) does, so perhaps you should rather add the
> test to check-qtest-ppc-y (and it will be inherited by ppc64)?

The test only runs the mac99 machine.

What happens when running "qemu-system-ppc64 -M mac99"? Does it runs in 
64-bit? Else it is probably pointless to run the test in ppc64, and I 
should restrict it the ppc[32].

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

[Laurent Vivier]

On 04/10/2019 10:53, Philippe Mathieu-Daudé wrote:
> On 10/4/19 10:05 AM, Laurent Vivier wrote:
>> On 04/10/2019 00:54, 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>
>>> ---
>>>   tests/Makefile.include |  2 ++
>>>   tests/fw_cfg-test.c    | 18 +++++++++++++++---
>>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>>> index 3543451ed3..322bdb36ff 100644
>>> --- a/tests/Makefile.include
>>> +++ b/tests/Makefile.include
>>> @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) +=
>>> tests/display-vga-test$(EXESUF)
>>>   check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>>>   check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) +=
>>> tests/ivshmem-test$(EXESUF)
>>>   check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>>> +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
>>
>> Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
>> (mac99, g3beige and prep) does, so perhaps you should rather add the
>> test to check-qtest-ppc-y (and it will be inherited by ppc64)?
> 
> The test only runs the mac99 machine.
> 
> What happens when running "qemu-system-ppc64 -M mac99"? Does it runs in
> 64-bit? 

Yes, it's way used to emulate a ppc64 powermac (G5)

$ qemu-system-ppc64 -M mac99 -serial tdio
>> =============================================================
>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>> Configuration device id QEMU version 1 machine id 3
>> CPUs: 1
>> Memory: 128M
>> UUID: 00000000-0000-0000-0000-000000000000
>> CPU type PowerPC,970FX

$ qemu-system-ppc -M mac99 -serial stdio
>> =============================================================
>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>> Configuration device id QEMU version 1 machine id 1
>> CPUs: 1
>> Memory: 128M
>> UUID: 00000000-0000-0000-0000-000000000000
>> CPU type PowerPC,G4

Thanks,
Laurent

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

[Philippe Mathieu-Daudé]

On 10/4/19 10:59 AM, Laurent Vivier wrote:
> On 04/10/2019 10:53, Philippe Mathieu-Daudé wrote:
>> On 10/4/19 10:05 AM, Laurent Vivier wrote:
>>> On 04/10/2019 00:54, 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>
>>>> ---
>>>>    tests/Makefile.include |  2 ++
>>>>    tests/fw_cfg-test.c    | 18 +++++++++++++++---
>>>>    2 files changed, 17 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>>>> index 3543451ed3..322bdb36ff 100644
>>>> --- a/tests/Makefile.include
>>>> +++ b/tests/Makefile.include
>>>> @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) +=
>>>> tests/display-vga-test$(EXESUF)
>>>>    check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>>>>    check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) +=
>>>> tests/ivshmem-test$(EXESUF)
>>>>    check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>>>> +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
>>>
>>> Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
>>> (mac99, g3beige and prep) does, so perhaps you should rather add the
>>> test to check-qtest-ppc-y (and it will be inherited by ppc64)?
>>
>> The test only runs the mac99 machine.
>>
>> What happens when running "qemu-system-ppc64 -M mac99"? Does it runs in
>> 64-bit?
> 
> Yes, it's way used to emulate a ppc64 powermac (G5)

Oh.

> $ qemu-system-ppc64 -M mac99 -serial tdio
>>> =============================================================
>>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>>> Configuration device id QEMU version 1 machine id 3
>>> CPUs: 1
>>> Memory: 128M
>>> UUID: 00000000-0000-0000-0000-000000000000
>>> CPU type PowerPC,970FX

So this would test the 64-bit/big-endian,

> $ qemu-system-ppc -M mac99 -serial stdio
>>> =============================================================
>>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>>> Configuration device id QEMU version 1 machine id 1
>>> CPUs: 1
>>> Memory: 128M
>>> UUID: 00000000-0000-0000-0000-000000000000
>>> CPU type PowerPC,G4

and this the 32-bit/big-endian device, is that correct?

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

[Laurent Vivier]

On 04/10/2019 11:03, Philippe Mathieu-Daudé wrote:
> On 10/4/19 10:59 AM, Laurent Vivier wrote:
>> On 04/10/2019 10:53, Philippe Mathieu-Daudé wrote:
>>> On 10/4/19 10:05 AM, Laurent Vivier wrote:
>>>> On 04/10/2019 00:54, 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>
>>>>> ---
>>>>>    tests/Makefile.include |  2 ++
>>>>>    tests/fw_cfg-test.c    | 18 +++++++++++++++---
>>>>>    2 files changed, 17 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>>>>> index 3543451ed3..322bdb36ff 100644
>>>>> --- a/tests/Makefile.include
>>>>> +++ b/tests/Makefile.include
>>>>> @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) +=
>>>>> tests/display-vga-test$(EXESUF)
>>>>>    check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>>>>>    check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) +=
>>>>> tests/ivshmem-test$(EXESUF)
>>>>>    check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>>>>> +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
>>>>
>>>> Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
>>>> (mac99, g3beige and prep) does, so perhaps you should rather add the
>>>> test to check-qtest-ppc-y (and it will be inherited by ppc64)?
>>>
>>> The test only runs the mac99 machine.
>>>
>>> What happens when running "qemu-system-ppc64 -M mac99"? Does it runs in
>>> 64-bit?
>>
>> Yes, it's way used to emulate a ppc64 powermac (G5)
> 
> Oh.
> 
>> $ qemu-system-ppc64 -M mac99 -serial tdio
>>>> =============================================================
>>>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>>>> Configuration device id QEMU version 1 machine id 3
>>>> CPUs: 1
>>>> Memory: 128M
>>>> UUID: 00000000-0000-0000-0000-000000000000
>>>> CPU type PowerPC,970FX
> 
> So this would test the 64-bit/big-endian,
> 
>> $ qemu-system-ppc -M mac99 -serial stdio
>>>> =============================================================
>>>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>>>> Configuration device id QEMU version 1 machine id 1
>>>> CPUs: 1
>>>> Memory: 128M
>>>> UUID: 00000000-0000-0000-0000-000000000000
>>>> CPU type PowerPC,G4
> 
> and this the 32-bit/big-endian device, is that correct?

Yes.

Note: G4 (ppc) can be either little or big endian, but 970FX, aka G5,
(ppc64) is only big-endian.

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

[Philippe Mathieu-Daudé]

On 10/4/19 11:14 AM, Laurent Vivier wrote:
> On 04/10/2019 11:03, Philippe Mathieu-Daudé wrote:
>> On 10/4/19 10:59 AM, Laurent Vivier wrote:
>>> On 04/10/2019 10:53, Philippe Mathieu-Daudé wrote:
>>>> On 10/4/19 10:05 AM, Laurent Vivier wrote:
>>>>> On 04/10/2019 00:54, 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>
>>>>>> ---
>>>>>>     tests/Makefile.include |  2 ++
>>>>>>     tests/fw_cfg-test.c    | 18 +++++++++++++++---
>>>>>>     2 files changed, 17 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>>>>>> index 3543451ed3..322bdb36ff 100644
>>>>>> --- a/tests/Makefile.include
>>>>>> +++ b/tests/Makefile.include
>>>>>> @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) +=
>>>>>> tests/display-vga-test$(EXESUF)
>>>>>>     check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>>>>>>     check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) +=
>>>>>> tests/ivshmem-test$(EXESUF)
>>>>>>     check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>>>>>> +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
>>>>>
>>>>> Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
>>>>> (mac99, g3beige and prep) does, so perhaps you should rather add the
>>>>> test to check-qtest-ppc-y (and it will be inherited by ppc64)?
>>>>
>>>> The test only runs the mac99 machine.
>>>>
>>>> What happens when running "qemu-system-ppc64 -M mac99"? Does it runs in
>>>> 64-bit?
>>>
>>> Yes, it's way used to emulate a ppc64 powermac (G5)
>>
>> Oh.
>>
>>> $ qemu-system-ppc64 -M mac99 -serial tdio
>>>>> =============================================================
>>>>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>>>>> Configuration device id QEMU version 1 machine id 3
>>>>> CPUs: 1
>>>>> Memory: 128M
>>>>> UUID: 00000000-0000-0000-0000-000000000000
>>>>> CPU type PowerPC,970FX
>>
>> So this would test the 64-bit/big-endian,
>>
>>> $ qemu-system-ppc -M mac99 -serial stdio
>>>>> =============================================================
>>>>> OpenBIOS 1.1 [Feb 2 2019 05:05]
>>>>> Configuration device id QEMU version 1 machine id 1
>>>>> CPUs: 1
>>>>> Memory: 128M
>>>>> UUID: 00000000-0000-0000-0000-000000000000
>>>>> CPU type PowerPC,G4
>>
>> and this the 32-bit/big-endian device, is that correct?
> 
> Yes.
> 
> Note: G4 (ppc) can be either little or big endian, but 970FX, aka G5,
> (ppc64) is only big-endian.

OK thanks! I'll update the tests and add few comments.

Regards,

Phil.

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

[Thomas Huth]

----- Original Message -----
> Sent: Friday, October 4, 2019 10:05:09 AM
> On 04/10/2019 00:54, Philippe Mathieu-Daudé wrote:
[...]
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 3543451ed3..322bdb36ff 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -242,6 +242,7 @@ check-qtest-ppc64-$(CONFIG_VGA) +=
> > tests/display-vga-test$(EXESUF)
> >  check-qtest-ppc64-y += tests/numa-test$(EXESUF)
> >  check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) += tests/ivshmem-test$(EXESUF)
> >  check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
> > +check-qtest-ppc64-y += tests/fw_cfg-test$(EXESUF)
> 
> Perhaps only a detail, but ppc64 (pseries) doesn't use fw_cfg, but ppc
> (mac99, g3beige and prep) does, so perhaps you should rather add the
> test to check-qtest-ppc-y (and it will be inherited by ppc64)?

Yes, this should go into check-qtest-ppc-y instead of check-qtest-ppc64-y.

 Thanks,
  Thomas

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191003225437.16651-1-philmd@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20191003225437.16651-1-philmd@redhat.com
Subject: [PATCH 0/7] fw_cfg: Run tests on big-endian

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20190916141544.17540-1-peter.maydell@linaro.org -> patchew/20190916141544.17540-1-peter.maydell@linaro.org
Switched to a new branch 'test'
6066ad4 tests/fw_cfg: Run the tests on big-endian targets
442cb00 tests/fw_cfg: Declare one QFWCFG for all tests
5a2d274 tests/libqos/fw_cfg: Pass QTestState as argument
3eafe62 tests/fw_cfg: Let the tests use a context
8c3aa7f tests/libqos/fw_cfg: Document pc_fw_cfg_init to drop pc_fw_cfg_uninit
44f19e3 tests/libqos/fw_cfg: Document mm_fw_cfg_init to drop mm_fw_cfg_uninit
2d6a0e2 tests/libqos/fw_cfg: Document io_fw_cfg_init to drop io_fw_cfg_uninit

=== OUTPUT BEGIN ===
1/7 Checking commit 2d6a0e2efe76 (tests/libqos/fw_cfg: Document io_fw_cfg_init to drop io_fw_cfg_uninit)
2/7 Checking commit 44f19e328030 (tests/libqos/fw_cfg: Document mm_fw_cfg_init to drop mm_fw_cfg_uninit)
3/7 Checking commit 8c3aa7f6b166 (tests/libqos/fw_cfg: Document pc_fw_cfg_init to drop pc_fw_cfg_uninit)
4/7 Checking commit 3eafe6274b71 (tests/fw_cfg: Let the tests use a context)
ERROR: line over 90 characters
#74: FILE: tests/fw_cfg-test.c:77:
+    s = qtest_initf("-M %s -uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8", ctx->machine_name);

total: 1 errors, 0 warnings, 227 lines checked

Patch 4/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/7 Checking commit 5a2d274af675 (tests/libqos/fw_cfg: Pass QTestState as argument)
WARNING: line over 80 characters
#387: FILE: tests/libqos/fw_cfg.h:29:
+void qfw_cfg_get(QTestState *qts, QFWCFG *fw_cfg, uint16_t key, void *data, size_t len);

total: 0 errors, 1 warnings, 391 lines checked

Patch 5/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/7 Checking commit 442cb008de98 (tests/fw_cfg: Declare one QFWCFG for all tests)
7/7 Checking commit 6066ad4b1834 (tests/fw_cfg: Run the tests on big-endian targets)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191003225437.16651-1-philmd@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com