[PATCH 0/3] qtest: Provide and use function for doing system reset

[PATCH 0/3] qtest: Provide and use function for doing system reset

[Peter Maydell]

I noticed while reviewing Roque's patchset that adds tests
for the CMSDK watchdog device that we are gradually accumulating
tests in tests/qtest which open-code "now reset the QEMU system".
Moreover, several of those tests get it wrong, by failing to
wait for the QMP RESET event that signals that the reset has
completed.

This series adds new functions qtest_system_reset() and
qtest_system_reset_nowait(), which send the QMP system_reset
command and then either do or do not wait for the RESET event.
It uses them to replace all the places in the test suite
that currently hand-craft sending a system_reset command.

The tests that previously forgot to wait for RESET and now
do so are bios-tables-test, q35-test, stm32l4x5_gpio-test,
and stm32l4x5_syscfg-test.

thanks
-- PMM

Peter Maydell (3):
  tests/qtest: Add qtest_system_reset() utility function
  tests/qtest: Use qtest_system_reset() instead of open-coded versions
  tests/qtest: Use qtest_system_reset_nowait() where appropriate

 tests/qtest/libqtest.h              | 25 +++++++++++++++++++++++++
 tests/qtest/bios-tables-test.c      |  4 ++--
 tests/qtest/boot-order-test.c       |  7 +------
 tests/qtest/device-plug-test.c      | 11 +----------
 tests/qtest/drive_del-test.c        |  7 +------
 tests/qtest/hd-geo-test.c           |  9 +--------
 tests/qtest/libqtest.c              | 16 ++++++++++++++++
 tests/qtest/q35-test.c              | 12 ++----------
 tests/qtest/qos-test.c              |  3 +--
 tests/qtest/stm32l4x5_gpio-test.c   | 10 +---------
 tests/qtest/stm32l4x5_syscfg-test.c | 12 ++----------
 11 files changed, 53 insertions(+), 63 deletions(-)

-- 
2.34.1

[PATCH 1/3] tests/qtest: Add qtest_system_reset() utility function

[Peter Maydell]

We have several qtest tests which want to reset the QEMU under test
during the course of testing something.  They currently generally
have their own functions to do this, which work by sending a
"system_reset" QMP command.  However, "system_reset" only requests a
reset, and many of the tests which send the QMP command forget the
"and then wait for the QMP RESET event" part which is needed to
ensure that the reset has completed.

Provide a qtest_system_reset() function in libqtest so that
we don't need to reimplement this in multiple different tests.

A few tests (for example device hotplug related tests) want to
perform the reset command and then wait for some other event that is
produced during the reset sequence.  For them we provide
qtest_system_reset_nowait() so they can clearly indicate that they
are deliberately not waiting for the RESET event.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/libqtest.h | 25 +++++++++++++++++++++++++
 tests/qtest/libqtest.c | 16 ++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index beb96b18ebd..f23d80e9e5e 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -88,6 +88,31 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
  */
 QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd);
 
+/**
+ * qtest_system_reset:
+ * @s: #QTestState instance to operate on.
+ *
+ * Send a "system_reset" command to the QEMU under test, and wait for
+ * the reset to complete before returning.
+ */
+void qtest_system_reset(QTestState *s);
+
+/**
+ * qtest_system_reset_nowait:
+ * @s: #QTestState instance to operate on.
+ *
+ * Send a "system_reset" command to the QEMU under test, but do not
+ * wait for the reset to complete before returning. The caller is
+ * responsible for waiting for either the RESET event or some other
+ * event of interest to them before proceeding.
+ *
+ * This function should only be used if you're specifically testing
+ * for some other event; in that case you can't use qtest_system_reset()
+ * because it will read and discard any other QMP events that arrive
+ * before the RESET event.
+ */
+void qtest_system_reset_nowait(QTestState *s);
+
 /**
  * qtest_wait_qemu:
  * @s: #QTestState instance to operate on.
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 817fd7aac52..8de5f1fde30 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -215,6 +215,22 @@ static void qtest_check_status(QTestState *s)
 #endif
 }
 
+void qtest_system_reset_nowait(QTestState *s)
+{
+    /* Request the system reset, but do not wait for it to complete */
+    qtest_qmp_assert_success(s, "{'execute': 'system_reset' }");
+}
+
+void qtest_system_reset(QTestState *s)
+{
+    qtest_system_reset_nowait(s);
+    /*
+     * Wait for the RESET event, which is sent once the system reset
+     * has actually completed.
+     */
+    qtest_qmp_eventwait(s, "RESET");
+}
+
 void qtest_wait_qemu(QTestState *s)
 {
     if (s->qemu_pid != -1) {
-- 
2.34.1

[PATCH 2/3] tests/qtest: Use qtest_system_reset() instead of open-coded versions

[Peter Maydell]

Use the qtest_system_reset() function in various tests that were
previously open-coding the system-reset. Note that in several
cases this fixes a bug where the test did not wait for the RESET
QMP event before continuing.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I can split this patch up if people prefer, but for test code
and given the size of the diffstat it didn't seem worthwhile.
---
 tests/qtest/bios-tables-test.c      |  4 ++--
 tests/qtest/boot-order-test.c       |  7 +------
 tests/qtest/hd-geo-test.c           |  9 +--------
 tests/qtest/q35-test.c              | 12 ++----------
 tests/qtest/qos-test.c              |  3 +--
 tests/qtest/stm32l4x5_gpio-test.c   | 10 +---------
 tests/qtest/stm32l4x5_syscfg-test.c | 12 ++----------
 7 files changed, 10 insertions(+), 47 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index e79f3a03df0..da42e6e3eb5 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -959,7 +959,7 @@ static void test_acpi_piix4_tcg_bridge(void)
     free_test_data(&data);
 
     /* check that reboot/reset doesn't change any ACPI tables  */
-    qtest_qmp_send(data.qts, "{'execute':'system_reset' }");
+    qtest_system_reset(data.qts);
     process_acpi_tables(&data);
     free_test_data(&data);
 }
@@ -1216,7 +1216,7 @@ static void test_acpi_q35_multif_bridge(void)
     free_test_data(&data);
 
     /* check that reboot/reset doesn't change any ACPI tables  */
-    qtest_qmp_send(data.qts, "{'execute':'system_reset' }");
+    qtest_system_reset(data.qts);
     process_acpi_tables(&data);
     free_test_data(&data);
 }
diff --git a/tests/qtest/boot-order-test.c b/tests/qtest/boot-order-test.c
index c67b8cfe169..4c851c2cdb9 100644
--- a/tests/qtest/boot-order-test.c
+++ b/tests/qtest/boot-order-test.c
@@ -40,12 +40,7 @@ static void test_a_boot_order(const char *machine,
                       machine ?: "", test_args);
     actual = read_boot_order(qts);
     g_assert_cmphex(actual, ==, expected_boot);
-    qtest_qmp_assert_success(qts, "{ 'execute': 'system_reset' }");
-    /*
-     * system_reset only requests reset.  We get a RESET event after
-     * the actual reset completes.  Need to wait for that.
-     */
-    qtest_qmp_eventwait(qts, "RESET");
+    qtest_system_reset(qts);
     actual = read_boot_order(qts);
     g_assert_cmphex(actual, ==, expected_reboot);
     qtest_quit(qts);
diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
index 85eb8d76687..1c73dea8f75 100644
--- a/tests/qtest/hd-geo-test.c
+++ b/tests/qtest/hd-geo-test.c
@@ -900,7 +900,6 @@ static void test_override_hot_unplug(TestArgs *args, const char *devid,
     QTestState *qts;
     char *joined_args;
     QFWCFG *fw_cfg;
-    QDict *response;
     int i;
 
     joined_args = g_strjoinv(" ", args->argv);
@@ -913,13 +912,7 @@ static void test_override_hot_unplug(TestArgs *args, const char *devid,
     /* unplug device an restart */
     qtest_qmp_device_del_send(qts, devid);
 
-    response = qtest_qmp(qts,
-                         "{ 'execute': 'system_reset', 'arguments': { }}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    qobject_unref(response);
-
-    qtest_qmp_eventwait(qts, "RESET");
+    qtest_system_reset(qts);
 
     read_bootdevices(fw_cfg, expected2);
 
diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
index c922d81bc02..7f58fc37469 100644
--- a/tests/qtest/q35-test.c
+++ b/tests/qtest/q35-test.c
@@ -83,7 +83,6 @@ static void test_smram_lock(void)
 {
     QPCIBus *pcibus;
     QPCIDevice *pcidev;
-    QDict *response;
     QTestState *qts;
 
     qts = qtest_init("-M q35");
@@ -107,10 +106,7 @@ static void test_smram_lock(void)
     g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
 
     /* reset */
-    response = qtest_qmp(qts, "{'execute': 'system_reset', 'arguments': {} }");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    qobject_unref(response);
+    qtest_system_reset(qts);
 
     /* check open is settable again */
     smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, false);
@@ -194,7 +190,6 @@ static void test_smram_smbase_lock(void)
 {
     QPCIBus *pcibus;
     QPCIDevice *pcidev;
-    QDict *response;
     QTestState *qts;
     int i;
 
@@ -237,10 +232,7 @@ static void test_smram_smbase_lock(void)
     }
 
     /* reset */
-    response = qtest_qmp(qts, "{'execute': 'system_reset', 'arguments': {} }");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    qobject_unref(response);
+    qtest_system_reset(qts);
 
     /* check RAM at SMBASE is available after reset */
     g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 114f6bef273..2f7e75a3392 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -103,8 +103,7 @@ static void restart_qemu_or_continue(char *path)
         old_path = g_strdup(path);
         qtest_start(path);
     } else { /* if cmd line is the same, reset the guest */
-        qobject_unref(qmp("{ 'execute': 'system_reset' }"));
-        qmp_eventwait("RESET");
+        qtest_system_reset(global_qtest);
     }
 }
 
diff --git a/tests/qtest/stm32l4x5_gpio-test.c b/tests/qtest/stm32l4x5_gpio-test.c
index c0686c7b306..3c6ea71febf 100644
--- a/tests/qtest/stm32l4x5_gpio-test.c
+++ b/tests/qtest/stm32l4x5_gpio-test.c
@@ -169,14 +169,6 @@ static uint32_t reset(uint32_t gpio, unsigned int offset)
     return 0x0;
 }
 
-static void system_reset(void)
-{
-    QDict *r;
-    r = qtest_qmp(global_qtest, "{'execute': 'system_reset'}");
-    g_assert_false(qdict_haskey(r, "error"));
-    qobject_unref(r);
-}
-
 static void test_idr_reset_value(void)
 {
     /*
@@ -214,7 +206,7 @@ static void test_idr_reset_value(void)
     gpio_writel(GPIO_H, OTYPER, 0xDEADBEEF);
     gpio_writel(GPIO_H, PUPDR, 0xDEADBEEF);
 
-    system_reset();
+    qtest_system_reset(global_qtest);
 
     uint32_t moder = gpio_readl(GPIO_A, MODER);
     uint32_t odr = gpio_readl(GPIO_A, ODR);
diff --git a/tests/qtest/stm32l4x5_syscfg-test.c b/tests/qtest/stm32l4x5_syscfg-test.c
index d5c71e2c0e7..376c80e31ca 100644
--- a/tests/qtest/stm32l4x5_syscfg-test.c
+++ b/tests/qtest/stm32l4x5_syscfg-test.c
@@ -47,14 +47,6 @@ static void syscfg_set_irq(int num, int level)
    qtest_set_irq_in(global_qtest, SOC, NULL, num, level);
 }
 
-static void system_reset(void)
-{
-    QDict *response;
-    response = qtest_qmp(global_qtest, "{'execute': 'system_reset'}");
-    g_assert(qdict_haskey(response, "return"));
-    qobject_unref(response);
-}
-
 static void test_reset(void)
 {
     /*
@@ -182,7 +174,7 @@ static void test_set_only_bits(void)
     syscfg_writel(SYSCFG_SWPR2, 0x00000000);
     g_assert_cmphex(syscfg_readl(SYSCFG_SWPR2), ==, 0xFFFFFFFF);
 
-    system_reset();
+    qtest_system_reset(global_qtest);
 }
 
 static void test_clear_only_bits(void)
@@ -194,7 +186,7 @@ static void test_clear_only_bits(void)
     syscfg_writel(SYSCFG_CFGR1, 0x00000001);
     g_assert_cmphex(syscfg_readl(SYSCFG_CFGR1), ==, 0x00000000);
 
-    system_reset();
+    qtest_system_reset(global_qtest);
 }
 
 static void test_interrupt(void)
-- 
2.34.1

[PATCH 3/3] tests/qtest: Use qtest_system_reset_nowait() where appropriate

[Peter Maydell]

In the device and drive plug/unplug tests we want to trigger
a system reset and then see if we get the appropriate
DEVICE_DELETED event. Use qtest_system_reset_nowait() here.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/device-plug-test.c | 11 +----------
 tests/qtest/drive_del-test.c   |  7 +------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index c6f33153eb4..127a7f9efea 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -15,15 +15,6 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
 
-static void system_reset(QTestState *qtest)
-{
-    QDict *resp;
-
-    resp = qtest_qmp(qtest, "{'execute': 'system_reset'}");
-    g_assert(qdict_haskey(resp, "return"));
-    qobject_unref(resp);
-}
-
 static void wait_device_deleted_event(QTestState *qtest, const char *id)
 {
     QDict *resp, *data;
@@ -58,7 +49,7 @@ static void process_device_remove(QTestState *qtest, const char *id)
      * handled, removing the device.
      */
     qtest_qmp_device_del_send(qtest, id);
-    system_reset(qtest);
+    qtest_system_reset_nowait(qtest);
     wait_device_deleted_event(qtest, id);
 }
 
diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
index 7b67a4bbee4..99f6fc2de1b 100644
--- a/tests/qtest/drive_del-test.c
+++ b/tests/qtest/drive_del-test.c
@@ -154,15 +154,10 @@ static void device_add(QTestState *qts)
 
 static void device_del(QTestState *qts, bool and_reset)
 {
-    QDict *response;
-
     qtest_qmp_device_del_send(qts, "dev0");
 
     if (and_reset) {
-        response = qtest_qmp(qts, "{'execute': 'system_reset' }");
-        g_assert(response);
-        g_assert(qdict_haskey(response, "return"));
-        qobject_unref(response);
+        qtest_system_reset_nowait(qts);
     }
 
     qtest_qmp_eventwait(qts, "DEVICE_DELETED");
-- 
2.34.1

Re: [PATCH 2/3] tests/qtest: Use qtest_system_reset() instead of open-coded versions

[Philippe Mathieu-Daudé]

On 15/11/24 16:50, Peter Maydell wrote:
> Use the qtest_system_reset() function in various tests that were
> previously open-coding the system-reset. Note that in several
> cases this fixes a bug where the test did not wait for the RESET
> QMP event before continuing.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I can split this patch up if people prefer, but for test code
> and given the size of the diffstat it didn't seem worthwhile.
> ---
>   tests/qtest/bios-tables-test.c      |  4 ++--
>   tests/qtest/boot-order-test.c       |  7 +------
>   tests/qtest/hd-geo-test.c           |  9 +--------
>   tests/qtest/q35-test.c              | 12 ++----------
>   tests/qtest/qos-test.c              |  3 +--
>   tests/qtest/stm32l4x5_gpio-test.c   | 10 +---------
>   tests/qtest/stm32l4x5_syscfg-test.c | 12 ++----------
>   7 files changed, 10 insertions(+), 47 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/3] tests/qtest: Add qtest_system_reset() utility function

[Philippe Mathieu-Daudé]

On 15/11/24 16:50, Peter Maydell wrote:
> We have several qtest tests which want to reset the QEMU under test
> during the course of testing something.  They currently generally
> have their own functions to do this, which work by sending a
> "system_reset" QMP command.  However, "system_reset" only requests a
> reset, and many of the tests which send the QMP command forget the
> "and then wait for the QMP RESET event" part which is needed to
> ensure that the reset has completed.
> 
> Provide a qtest_system_reset() function in libqtest so that
> we don't need to reimplement this in multiple different tests.
> 
> A few tests (for example device hotplug related tests) want to
> perform the reset command and then wait for some other event that is
> produced during the reset sequence.  For them we provide
> qtest_system_reset_nowait() so they can clearly indicate that they
> are deliberately not waiting for the RESET event.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   tests/qtest/libqtest.h | 25 +++++++++++++++++++++++++
>   tests/qtest/libqtest.c | 16 ++++++++++++++++
>   2 files changed, 41 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/3] tests/qtest: Use qtest_system_reset_nowait() where appropriate

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> In the device and drive plug/unplug tests we want to trigger
> a system reset and then see if we get the appropriate
> DEVICE_DELETED event. Use qtest_system_reset_nowait() here.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 0/3] qtest: Provide and use function for doing system reset

[Fabiano Rosas]

On Fri, 15 Nov 2024 16:50:38 +0000, Peter Maydell wrote:
> I noticed while reviewing Roque's patchset that adds tests
> for the CMSDK watchdog device that we are gradually accumulating
> tests in tests/qtest which open-code "now reset the QEMU system".
> Moreover, several of those tests get it wrong, by failing to
> wait for the QMP RESET event that signals that the reset has
> completed.
> 
> [...]

Queued, thanks!