[Qemu-devel] [PATCH v4 0/3] tests/microbit-test: Add UART device test

[Qemu-devel] [PATCH v4 0/3] tests/microbit-test: Add UART device test

[Julia Suvorova]

v4:
    * Replace sprintf with g_strdup_printf [Peter]
    * Move socket connection to qtest library [Peter]
    * Use memcmp instead of strcmp [Stefan]
    * Remove using global_qtest [Thomas]
v3:
    * Fix directory leak [Stefan]

Based-on: <20190110094020.18354-1-stefanha@redhat.com>

Julia Suvorova (3):
  tests/libqtest: Introduce qtest_init_with_serial()
  tests/microbit-test: Make test independent of global_qtest
  tests/microbit-test: Check nRF51 UART functionality

 tests/libqtest.c      |  26 ++++
 tests/libqtest.h      |  11 ++
 tests/microbit-test.c | 331 +++++++++++++++++++++++++++---------------
 3 files changed, 250 insertions(+), 118 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v4 1/3] tests/libqtest: Introduce qtest_init_with_serial()

[Julia Suvorova]

Run qtest with a socket that connects QEMU chardev and test code.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 tests/libqtest.c | 26 ++++++++++++++++++++++++++
 tests/libqtest.h | 11 +++++++++++
 2 files changed, 37 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 55750dd68d..3a015cfe13 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -315,6 +315,32 @@ QTestState *qtest_initf(const char *fmt, ...)
     return s;
 }
 
+QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd)
+{
+    int sock_fd_init;
+    char *sock_path, sock_dir[] = "/tmp/qtest-serial-XXXXXX";
+    QTestState *qts;
+
+    g_assert(mkdtemp(sock_dir));
+    sock_path = g_strdup_printf("%s/sock", sock_dir);
+
+    sock_fd_init = init_socket(sock_path);
+
+    qts = qtest_initf("-chardev socket,id=s0,path=%s,nowait "
+                      "-serial chardev:s0 %s",
+                      sock_path, extra_args);
+
+    *sock_fd = socket_accept(sock_fd_init);
+
+    unlink(sock_path);
+    g_free(sock_path);
+    rmdir(sock_dir);
+
+    g_assert(*sock_fd >= 0);
+
+    return qts;
+}
+
 void qtest_quit(QTestState *s)
 {
     g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 7ea94139b0..5937f91912 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -62,6 +62,17 @@ QTestState *qtest_init(const char *extra_args);
  */
 QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
 
+/**
+ * qtest_init_with_serial:
+ * @extra_args: other arguments to pass to QEMU.  CAUTION: these
+ * arguments are subject to word splitting and shell evaluation.
+ * @sock_fd: pointer to store the socket file descriptor for
+ * connection with serial.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd);
+
 /**
  * qtest_quit:
  * @s: #QTestState instance to operate on.
-- 
2.17.1

Re: [Qemu-devel] [PATCH v4 1/3] tests/libqtest: Introduce qtest_init_with_serial()

[Thomas Huth]

On 2019-01-17 17:16, Julia Suvorova wrote:
> Run qtest with a socket that connects QEMU chardev and test code.
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  tests/libqtest.c | 26 ++++++++++++++++++++++++++
>  tests/libqtest.h | 11 +++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 55750dd68d..3a015cfe13 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -315,6 +315,32 @@ QTestState *qtest_initf(const char *fmt, ...)
>      return s;
>  }
>  
> +QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd)
> +{
> +    int sock_fd_init;
> +    char *sock_path, sock_dir[] = "/tmp/qtest-serial-XXXXXX";
> +    QTestState *qts;
> +
> +    g_assert(mkdtemp(sock_dir));

We normally don't do it in QEMU, but still, it theoretically possible to
turn of g_assert() by defining G_DISABLE_ASSER, so the content should be
free of side effects. See also the doc of g_assert() here:

 https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert

Thus could you please rewrite this as:

    ptr = mkdtemp(sock_dir);
    g_assert(ptr != NULL);

?

> +    sock_path = g_strdup_printf("%s/sock", sock_dir);
> +
> +    sock_fd_init = init_socket(sock_path);
> +
> +    qts = qtest_initf("-chardev socket,id=s0,path=%s,nowait "

Daniel just posted a patch that will disallow "nowait" without "server":

 https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg03334.html

So I think you've got to drop the "nowait" here.

> +                      "-serial chardev:s0 %s",
> +                      sock_path, extra_args);
> +
> +    *sock_fd = socket_accept(sock_fd_init);
> +
> +    unlink(sock_path);
> +    g_free(sock_path);
> +    rmdir(sock_dir);
> +
> +    g_assert(*sock_fd >= 0);
> +
> +    return qts;
> +}
> +
>  void qtest_quit(QTestState *s)
>  {
>      g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 7ea94139b0..5937f91912 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -62,6 +62,17 @@ QTestState *qtest_init(const char *extra_args);
>   */
>  QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>  
> +/**
> + * qtest_init_with_serial:
> + * @extra_args: other arguments to pass to QEMU.  CAUTION: these
> + * arguments are subject to word splitting and shell evaluation.
> + * @sock_fd: pointer to store the socket file descriptor for
> + * connection with serial.
> + *
> + * Returns: #QTestState instance.
> + */
> +QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd);
> +
>  /**
>   * qtest_quit:
>   * @s: #QTestState instance to operate on.
> 

 Thomas

[Qemu-devel] [PATCH v4 2/3] tests/microbit-test: Make test independent of global_qtest

[Julia Suvorova]

Using of global_qtest is not required here. Let's replace functions like
readl() with the corresponding qtest_* counterparts.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 tests/microbit-test.c | 247 ++++++++++++++++++++++--------------------
 1 file changed, 129 insertions(+), 118 deletions(-)

diff --git a/tests/microbit-test.c b/tests/microbit-test.c
index dcdc0cd41a..afeb6b082a 100644
--- a/tests/microbit-test.c
+++ b/tests/microbit-test.c
@@ -24,22 +24,22 @@
 #include "hw/i2c/microbit_i2c.h"
 
 /* Read a byte from I2C device at @addr from register @reg */
-static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
+static uint32_t i2c_read_byte(QTestState *qts, uint32_t addr, uint32_t reg)
 {
     uint32_t val;
 
-    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
-    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
-    writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
-    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
+    val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
     g_assert_cmpuint(val, ==, 1);
-    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
 
-    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
-    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
+    val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
     g_assert_cmpuint(val, ==, 1);
-    val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
-    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
+    val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
 
     return val;
 }
@@ -47,22 +47,25 @@ static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
 static void test_microbit_i2c(void)
 {
     uint32_t val;
+    QTestState *qts = qtest_init("-M microbit");
 
     /* We don't program pins/irqs but at least enable the device */
-    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5);
 
     /* MMA8653 magnetometer detection */
-    val = i2c_read_byte(0x3A, 0x0D);
+    val = i2c_read_byte(qts, 0x3A, 0x0D);
     g_assert_cmpuint(val, ==, 0x5A);
 
-    val = i2c_read_byte(0x3A, 0x0D);
+    val = i2c_read_byte(qts, 0x3A, 0x0D);
     g_assert_cmpuint(val, ==, 0x5A);
 
     /* LSM303 accelerometer detection */
-    val = i2c_read_byte(0x3C, 0x4F);
+    val = i2c_read_byte(qts, 0x3C, 0x4F);
     g_assert_cmpuint(val, ==, 0x40);
 
-    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0);
+    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0);
+
+    qtest_quit(qts);
 }
 
 static void test_nrf51_gpio(void)
@@ -80,220 +83,228 @@ static void test_nrf51_gpio(void)
         {NRF51_GPIO_REG_DIRCLR, 0x00000000}
     };
 
+    QTestState *qts = qtest_init("-M microbit");
+
     /* Check reset state */
     for (i = 0; i < ARRAY_SIZE(reset_state); i++) {
         expected = reset_state[i].expected;
-        actual = readl(NRF51_GPIO_BASE + reset_state[i].addr);
+        actual = qtest_readl(qts, NRF51_GPIO_BASE + reset_state[i].addr);
         g_assert_cmpuint(actual, ==, expected);
     }
 
     for (i = 0; i < NRF51_GPIO_PINS; i++) {
         expected = 0x00000002;
-        actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START + i * 4);
+        actual = qtest_readl(qts, NRF51_GPIO_BASE +
+                                  NRF51_GPIO_REG_CNF_START + i * 4);
         g_assert_cmpuint(actual, ==, expected);
     }
 
     /* Check dir bit consistency between dir and cnf */
     /* Check set via DIRSET */
     expected = 0x80000001;
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRSET, expected);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRSET, expected);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
     g_assert_cmpuint(actual, ==, expected);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01;
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START)
+             & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
 
     /* Check clear via DIRCLR */
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRCLR, 0x80000001);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRCLR, 0x80000001);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
     g_assert_cmpuint(actual, ==, 0x00000000);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01;
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START)
+             & 0x01;
     g_assert_cmpuint(actual, ==, 0x00);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
     g_assert_cmpuint(actual, ==, 0x00);
 
     /* Check set via DIR */
     expected = 0x80000001;
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, expected);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, expected);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
     g_assert_cmpuint(actual, ==, expected);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01;
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START)
+             & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
 
     /* Reset DIR */
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, 0x00000000);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, 0x00000000);
 
     /* Check Input propagates */
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x00);
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x00);
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x00);
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
 
     /* Check pull-up working */
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x00);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b1110);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b1110);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
 
     /* Check pull-down working */
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0110);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0110);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x00);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
 
     /* Check Output propagates */
-    irq_intercept_out("/machine/nrf51");
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0011);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
-    g_assert_true(get_irq(0));
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
-    g_assert_false(get_irq(0));
+    qtest_irq_intercept_out(qts, "/machine/nrf51");
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0011);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
+    g_assert_true(qtest_get_irq(qts, 0));
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
+    g_assert_false(qtest_get_irq(qts, 0));
 
     /* Check self-stimulation */
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x01);
 
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
-    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
+    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
     g_assert_cmpuint(actual, ==, 0x00);
 
     /*
      * Check short-circuit - generates an guest_error which must be checked
      * manually as long as qtest can not scan qemu_log messages
      */
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
-    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
-    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
+    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
+
+    qtest_quit(qts);
 }
 
-static void timer_task(hwaddr task)
+static void timer_task(QTestState *qts, hwaddr task)
 {
-    writel(NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK);
+    qtest_writel(qts, NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK);
 }
 
-static void timer_clear_event(hwaddr event)
+static void timer_clear_event(QTestState *qts, hwaddr event)
 {
-    writel(NRF51_TIMER_BASE + event, NRF51_EVENT_CLEAR);
+    qtest_writel(qts, NRF51_TIMER_BASE + event, NRF51_EVENT_CLEAR);
 }
 
-static void timer_set_bitmode(uint8_t mode)
+static void timer_set_bitmode(QTestState *qts, uint8_t mode)
 {
-    writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_BITMODE, mode);
+    qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_BITMODE, mode);
 }
 
-static void timer_set_prescaler(uint8_t prescaler)
+static void timer_set_prescaler(QTestState *qts, uint8_t prescaler)
 {
-    writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_PRESCALER, prescaler);
+    qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_PRESCALER, prescaler);
 }
 
-static void timer_set_cc(size_t idx, uint32_t value)
+static void timer_set_cc(QTestState *qts, size_t idx, uint32_t value)
 {
-    writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_CC0 + idx * 4, value);
+    qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_CC0 + idx * 4, value);
 }
 
-static void timer_assert_events(uint32_t ev0, uint32_t ev1, uint32_t ev2,
-                                uint32_t ev3)
+static void timer_assert_events(QTestState *qts, uint32_t ev0, uint32_t ev1,
+                                uint32_t ev2, uint32_t ev3)
 {
-    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_0) == ev0);
-    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_1) == ev1);
-    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_2) == ev2);
-    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_3) == ev3);
+    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_0)
+             == ev0);
+    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_1)
+             == ev1);
+    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_2)
+             == ev2);
+    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_3)
+             == ev3);
 }
 
 static void test_nrf51_timer(void)
 {
     uint32_t steps_to_overflow = 408;
+    QTestState *qts = qtest_init("-M microbit");
 
     /* Compare Match */
-    timer_task(NRF51_TIMER_TASK_STOP);
-    timer_task(NRF51_TIMER_TASK_CLEAR);
+    timer_task(qts, NRF51_TIMER_TASK_STOP);
+    timer_task(qts, NRF51_TIMER_TASK_CLEAR);
 
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_0);
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_1);
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_2);
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_3);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_0);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_1);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_2);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_3);
 
-    timer_set_bitmode(NRF51_TIMER_WIDTH_16); /* 16 MHz Timer */
-    timer_set_prescaler(0);
+    timer_set_bitmode(qts, NRF51_TIMER_WIDTH_16); /* 16 MHz Timer */
+    timer_set_prescaler(qts, 0);
     /* Swept over in first step */
-    timer_set_cc(0, 2);
+    timer_set_cc(qts, 0, 2);
     /* Barely miss on first step */
-    timer_set_cc(1, 162);
+    timer_set_cc(qts, 1, 162);
     /* Spot on on third step */
-    timer_set_cc(2, 480);
+    timer_set_cc(qts, 2, 480);
 
-    timer_assert_events(0, 0, 0, 0);
+    timer_assert_events(qts, 0, 0, 0, 0);
 
-    timer_task(NRF51_TIMER_TASK_START);
-    clock_step(10000);
-    timer_assert_events(1, 0, 0, 0);
+    timer_task(qts, NRF51_TIMER_TASK_START);
+    qtest_clock_step(qts, 10000);
+    timer_assert_events(qts, 1, 0, 0, 0);
 
     /* Swept over on first overflow */
-    timer_set_cc(3, 114);
+    timer_set_cc(qts, 3, 114);
 
-    clock_step(10000);
-    timer_assert_events(1, 1, 0, 0);
+    qtest_clock_step(qts, 10000);
+    timer_assert_events(qts, 1, 1, 0, 0);
 
-    clock_step(10000);
-    timer_assert_events(1, 1, 1, 0);
+    qtest_clock_step(qts, 10000);
+    timer_assert_events(qts, 1, 1, 1, 0);
 
     /* Wrap time until internal counter overflows */
     while (steps_to_overflow--) {
-        timer_assert_events(1, 1, 1, 0);
-        clock_step(10000);
+        timer_assert_events(qts, 1, 1, 1, 0);
+        qtest_clock_step(qts, 10000);
     }
 
-    timer_assert_events(1, 1, 1, 1);
+    timer_assert_events(qts, 1, 1, 1, 1);
 
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_0);
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_1);
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_2);
-    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_3);
-    timer_assert_events(0, 0, 0, 0);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_0);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_1);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_2);
+    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_3);
+    timer_assert_events(qts, 0, 0, 0, 0);
 
-    timer_task(NRF51_TIMER_TASK_STOP);
+    timer_task(qts, NRF51_TIMER_TASK_STOP);
 
     /* Test Proposal: Stop/Shutdown */
     /* Test Proposal: Shortcut Compare -> Clear */
     /* Test Proposal: Shortcut Compare -> Stop */
     /* Test Proposal: Counter Mode */
+
+    qtest_quit(qts);
 }
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
 
-    global_qtest = qtest_initf("-machine microbit");
-
     qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
     qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
     qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
 
-    ret = g_test_run();
-
-    qtest_quit(global_qtest);
-    return ret;
+    return g_test_run();
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH v4 2/3] tests/microbit-test: Make test independent of global_qtest

[Philippe Mathieu-Daudé]

On 1/17/19 5:16 PM, Julia Suvorova via Qemu-devel wrote:
> Using of global_qtest is not required here. Let's replace functions like
> readl() with the corresponding qtest_* counterparts.
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/microbit-test.c | 247 ++++++++++++++++++++++--------------------
>  1 file changed, 129 insertions(+), 118 deletions(-)
> 
> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
> index dcdc0cd41a..afeb6b082a 100644
> --- a/tests/microbit-test.c
> +++ b/tests/microbit-test.c
> @@ -24,22 +24,22 @@
>  #include "hw/i2c/microbit_i2c.h"
>  
>  /* Read a byte from I2C device at @addr from register @reg */
> -static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
> +static uint32_t i2c_read_byte(QTestState *qts, uint32_t addr, uint32_t reg)
>  {
>      uint32_t val;
>  
> -    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
> -    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
> -    writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
> -    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
> +    val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
>      g_assert_cmpuint(val, ==, 1);
> -    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
>  
> -    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
> -    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
> +    val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
>      g_assert_cmpuint(val, ==, 1);
> -    val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
> -    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
> +    val = qtest_readl(qts, NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
>  
>      return val;
>  }
> @@ -47,22 +47,25 @@ static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
>  static void test_microbit_i2c(void)
>  {
>      uint32_t val;
> +    QTestState *qts = qtest_init("-M microbit");
>  
>      /* We don't program pins/irqs but at least enable the device */
> -    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5);
>  
>      /* MMA8653 magnetometer detection */
> -    val = i2c_read_byte(0x3A, 0x0D);
> +    val = i2c_read_byte(qts, 0x3A, 0x0D);
>      g_assert_cmpuint(val, ==, 0x5A);
>  
> -    val = i2c_read_byte(0x3A, 0x0D);
> +    val = i2c_read_byte(qts, 0x3A, 0x0D);
>      g_assert_cmpuint(val, ==, 0x5A);
>  
>      /* LSM303 accelerometer detection */
> -    val = i2c_read_byte(0x3C, 0x4F);
> +    val = i2c_read_byte(qts, 0x3C, 0x4F);
>      g_assert_cmpuint(val, ==, 0x40);
>  
> -    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0);
> +    qtest_writel(qts, NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0);
> +
> +    qtest_quit(qts);
>  }
>  
>  static void test_nrf51_gpio(void)
> @@ -80,220 +83,228 @@ static void test_nrf51_gpio(void)
>          {NRF51_GPIO_REG_DIRCLR, 0x00000000}
>      };
>  
> +    QTestState *qts = qtest_init("-M microbit");
> +
>      /* Check reset state */
>      for (i = 0; i < ARRAY_SIZE(reset_state); i++) {
>          expected = reset_state[i].expected;
> -        actual = readl(NRF51_GPIO_BASE + reset_state[i].addr);
> +        actual = qtest_readl(qts, NRF51_GPIO_BASE + reset_state[i].addr);
>          g_assert_cmpuint(actual, ==, expected);
>      }
>  
>      for (i = 0; i < NRF51_GPIO_PINS; i++) {
>          expected = 0x00000002;
> -        actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START + i * 4);
> +        actual = qtest_readl(qts, NRF51_GPIO_BASE +
> +                                  NRF51_GPIO_REG_CNF_START + i * 4);
>          g_assert_cmpuint(actual, ==, expected);
>      }
>  
>      /* Check dir bit consistency between dir and cnf */
>      /* Check set via DIRSET */
>      expected = 0x80000001;
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRSET, expected);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRSET, expected);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
>      g_assert_cmpuint(actual, ==, expected);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01;
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START)
> +             & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
>  
>      /* Check clear via DIRCLR */
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRCLR, 0x80000001);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIRCLR, 0x80000001);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
>      g_assert_cmpuint(actual, ==, 0x00000000);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01;
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START)
> +             & 0x01;
>      g_assert_cmpuint(actual, ==, 0x00);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x00);
>  
>      /* Check set via DIR */
>      expected = 0x80000001;
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, expected);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, expected);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR);
>      g_assert_cmpuint(actual, ==, expected);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START) & 0x01;
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START)
> +             & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
>  
>      /* Reset DIR */
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, 0x00000000);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_DIR, 0x00000000);
>  
>      /* Check Input propagates */
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x00);
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x00);
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x00);
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
>  
>      /* Check pull-up working */
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x00);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b1110);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b1110);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
>  
>      /* Check pull-down working */
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0000);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0110);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0110);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x00);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0x02);
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, -1);
>  
>      /* Check Output propagates */
> -    irq_intercept_out("/machine/nrf51");
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0011);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
> -    g_assert_true(get_irq(0));
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
> -    g_assert_false(get_irq(0));
> +    qtest_irq_intercept_out(qts, "/machine/nrf51");
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b0011);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
> +    g_assert_true(qtest_get_irq(qts, 0));
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
> +    g_assert_false(qtest_get_irq(qts, 0));
>  
>      /* Check self-stimulation */
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x01);
>  
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
> -    actual = readl(NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTCLR, 0x01);
> +    actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN) & 0x01;
>      g_assert_cmpuint(actual, ==, 0x00);
>  
>      /*
>       * Check short-circuit - generates an guest_error which must be checked
>       * manually as long as qtest can not scan qemu_log messages
>       */
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
> -    writel(NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
> -    qtest_set_irq_in(global_qtest, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START, 0b01);
> +    qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_OUTSET, 0x01);
> +    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
> +
> +    qtest_quit(qts);
>  }
>  
> -static void timer_task(hwaddr task)
> +static void timer_task(QTestState *qts, hwaddr task)
>  {
> -    writel(NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK);
> +    qtest_writel(qts, NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK);
>  }
>  
> -static void timer_clear_event(hwaddr event)
> +static void timer_clear_event(QTestState *qts, hwaddr event)
>  {
> -    writel(NRF51_TIMER_BASE + event, NRF51_EVENT_CLEAR);
> +    qtest_writel(qts, NRF51_TIMER_BASE + event, NRF51_EVENT_CLEAR);
>  }
>  
> -static void timer_set_bitmode(uint8_t mode)
> +static void timer_set_bitmode(QTestState *qts, uint8_t mode)
>  {
> -    writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_BITMODE, mode);
> +    qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_BITMODE, mode);
>  }
>  
> -static void timer_set_prescaler(uint8_t prescaler)
> +static void timer_set_prescaler(QTestState *qts, uint8_t prescaler)
>  {
> -    writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_PRESCALER, prescaler);
> +    qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_PRESCALER, prescaler);
>  }
>  
> -static void timer_set_cc(size_t idx, uint32_t value)
> +static void timer_set_cc(QTestState *qts, size_t idx, uint32_t value)
>  {
> -    writel(NRF51_TIMER_BASE + NRF51_TIMER_REG_CC0 + idx * 4, value);
> +    qtest_writel(qts, NRF51_TIMER_BASE + NRF51_TIMER_REG_CC0 + idx * 4, value);
>  }
>  
> -static void timer_assert_events(uint32_t ev0, uint32_t ev1, uint32_t ev2,
> -                                uint32_t ev3)
> +static void timer_assert_events(QTestState *qts, uint32_t ev0, uint32_t ev1,
> +                                uint32_t ev2, uint32_t ev3)
>  {
> -    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_0) == ev0);
> -    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_1) == ev1);
> -    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_2) == ev2);
> -    g_assert(readl(NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_3) == ev3);
> +    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_0)
> +             == ev0);
> +    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_1)
> +             == ev1);
> +    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_2)
> +             == ev2);
> +    g_assert(qtest_readl(qts, NRF51_TIMER_BASE + NRF51_TIMER_EVENT_COMPARE_3)
> +             == ev3);
>  }
>  
>  static void test_nrf51_timer(void)
>  {
>      uint32_t steps_to_overflow = 408;
> +    QTestState *qts = qtest_init("-M microbit");
>  
>      /* Compare Match */
> -    timer_task(NRF51_TIMER_TASK_STOP);
> -    timer_task(NRF51_TIMER_TASK_CLEAR);
> +    timer_task(qts, NRF51_TIMER_TASK_STOP);
> +    timer_task(qts, NRF51_TIMER_TASK_CLEAR);
>  
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_0);
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_1);
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_2);
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_3);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_0);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_1);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_2);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_3);
>  
> -    timer_set_bitmode(NRF51_TIMER_WIDTH_16); /* 16 MHz Timer */
> -    timer_set_prescaler(0);
> +    timer_set_bitmode(qts, NRF51_TIMER_WIDTH_16); /* 16 MHz Timer */
> +    timer_set_prescaler(qts, 0);
>      /* Swept over in first step */
> -    timer_set_cc(0, 2);
> +    timer_set_cc(qts, 0, 2);
>      /* Barely miss on first step */
> -    timer_set_cc(1, 162);
> +    timer_set_cc(qts, 1, 162);
>      /* Spot on on third step */
> -    timer_set_cc(2, 480);
> +    timer_set_cc(qts, 2, 480);
>  
> -    timer_assert_events(0, 0, 0, 0);
> +    timer_assert_events(qts, 0, 0, 0, 0);
>  
> -    timer_task(NRF51_TIMER_TASK_START);
> -    clock_step(10000);
> -    timer_assert_events(1, 0, 0, 0);
> +    timer_task(qts, NRF51_TIMER_TASK_START);
> +    qtest_clock_step(qts, 10000);
> +    timer_assert_events(qts, 1, 0, 0, 0);
>  
>      /* Swept over on first overflow */
> -    timer_set_cc(3, 114);
> +    timer_set_cc(qts, 3, 114);
>  
> -    clock_step(10000);
> -    timer_assert_events(1, 1, 0, 0);
> +    qtest_clock_step(qts, 10000);
> +    timer_assert_events(qts, 1, 1, 0, 0);
>  
> -    clock_step(10000);
> -    timer_assert_events(1, 1, 1, 0);
> +    qtest_clock_step(qts, 10000);
> +    timer_assert_events(qts, 1, 1, 1, 0);
>  
>      /* Wrap time until internal counter overflows */
>      while (steps_to_overflow--) {
> -        timer_assert_events(1, 1, 1, 0);
> -        clock_step(10000);
> +        timer_assert_events(qts, 1, 1, 1, 0);
> +        qtest_clock_step(qts, 10000);
>      }
>  
> -    timer_assert_events(1, 1, 1, 1);
> +    timer_assert_events(qts, 1, 1, 1, 1);
>  
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_0);
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_1);
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_2);
> -    timer_clear_event(NRF51_TIMER_EVENT_COMPARE_3);
> -    timer_assert_events(0, 0, 0, 0);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_0);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_1);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_2);
> +    timer_clear_event(qts, NRF51_TIMER_EVENT_COMPARE_3);
> +    timer_assert_events(qts, 0, 0, 0, 0);
>  
> -    timer_task(NRF51_TIMER_TASK_STOP);
> +    timer_task(qts, NRF51_TIMER_TASK_STOP);
>  
>      /* Test Proposal: Stop/Shutdown */
>      /* Test Proposal: Shortcut Compare -> Clear */
>      /* Test Proposal: Shortcut Compare -> Stop */
>      /* Test Proposal: Counter Mode */
> +
> +    qtest_quit(qts);
>  }
>  
>  int main(int argc, char **argv)
>  {
> -    int ret;
> -
>      g_test_init(&argc, &argv, NULL);
>  
> -    global_qtest = qtest_initf("-machine microbit");
> -
>      qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
>      qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
>      qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
>  
> -    ret = g_test_run();
> -
> -    qtest_quit(global_qtest);
> -    return ret;
> +    return g_test_run();
>  }
> 

Re: [Qemu-devel] [PATCH v4 2/3] tests/microbit-test: Make test independent of global_qtest

[Thomas Huth]

On 2019-01-17 17:16, Julia Suvorova wrote:
> Using of global_qtest is not required here. Let's replace functions like
> readl() with the corresponding qtest_* counterparts.
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  tests/microbit-test.c | 247 ++++++++++++++++++++++--------------------
>  1 file changed, 129 insertions(+), 118 deletions(-)

Great, thank you!

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

[Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality

[Julia Suvorova]

Some functional tests for:
    Basic reception/transmittion
    Suspending
    INTEN* registers

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 tests/microbit-test.c | 84 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/tests/microbit-test.c b/tests/microbit-test.c
index afeb6b082a..3da6d9529f 100644
--- a/tests/microbit-test.c
+++ b/tests/microbit-test.c
@@ -19,10 +19,93 @@
 #include "libqtest.h"
 
 #include "hw/arm/nrf51.h"
+#include "hw/char/nrf51_uart.h"
 #include "hw/gpio/nrf51_gpio.h"
 #include "hw/timer/nrf51_timer.h"
 #include "hw/i2c/microbit_i2c.h"
 
+#include <sys/socket.h>
+#include <sys/un.h>
+
+static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr)
+{
+    int i;
+
+    for (i = 0; i < 1000; i++) {
+        if (qtest_readl(qts, event_addr) == 1) {
+            qtest_writel(qts, event_addr, 0x00);
+            return true;
+        }
+        g_usleep(10000);
+    }
+
+    return false;
+}
+
+static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in,
+                           char *out)
+{
+    int i, in_len = strlen(in);
+
+    g_assert(write(sock_fd, in, in_len) == in_len);
+    for (i = 0; i < in_len; i++) {
+        g_assert(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_RXDRDY));
+        out[i] = qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD);
+    }
+    out[i] = '\0';
+}
+
+static void uart_w_to_txd(QTestState *qts, const char *in)
+{
+    int i, in_len = strlen(in);
+
+    for (i = 0; i < in_len; i++) {
+        qtest_writel(qts, NRF51_UART_BASE + A_UART_TXD, in[i]);
+        g_assert(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_TXDRDY));
+    }
+}
+
+static void test_nrf51_uart(void)
+{
+    int sock_fd;
+    char s[10];
+    QTestState *qts = qtest_init_with_serial("-M microbit", &sock_fd);
+
+    g_assert(write(sock_fd, "c", 1) == 1);
+    g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD) == 0);
+
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_ENABLE, 0x04);
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTRX, 0x01);
+
+    g_assert(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_RXDRDY));
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_RXDRDY, 0x00);
+    g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD) == 'c');
+
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_INTENSET, 0x04);
+    g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_INTEN) == 0x04);
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_INTENCLR, 0x04);
+    g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_INTEN) == 0x00);
+
+    uart_rw_to_rxd(qts, sock_fd, "hello", s);
+    g_assert(memcmp(s, "hello", 5) == 0);
+
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTTX, 0x01);
+    uart_w_to_txd(qts, "d");
+    g_assert(read(sock_fd, s, 10) == 1);
+    g_assert(s[0] == 'd');
+
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_SUSPEND, 0x01);
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_TXD, 'h');
+    qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTTX, 0x01);
+    uart_w_to_txd(qts, "world");
+    g_assert(read(sock_fd, s, 10) == 5);
+    g_assert(memcmp(s, "world", 5) == 0);
+
+    close(sock_fd);
+
+    qtest_quit(qts);
+}
+
 /* Read a byte from I2C device at @addr from register @reg */
 static uint32_t i2c_read_byte(QTestState *qts, uint32_t addr, uint32_t reg)
 {
@@ -302,6 +385,7 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
+    qtest_add_func("/microbit/nrf51/uart", test_nrf51_uart);
     qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
     qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
     qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality

[Stefan Hajnoczi]

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

On Thu, Jan 17, 2019 at 07:16:40PM +0300, Julia Suvorova wrote:
> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
> index afeb6b082a..3da6d9529f 100644
> --- a/tests/microbit-test.c
> +++ b/tests/microbit-test.c
> @@ -19,10 +19,93 @@
>  #include "libqtest.h"
>  
>  #include "hw/arm/nrf51.h"
> +#include "hw/char/nrf51_uart.h"
>  #include "hw/gpio/nrf51_gpio.h"
>  #include "hw/timer/nrf51_timer.h"
>  #include "hw/i2c/microbit_i2c.h"
>  
> +#include <sys/socket.h>
> +#include <sys/un.h>

./HACKING "1.2. Include directives" requires putting system header
includes (<>) right after #include "qemu/osdep.h".

But are these includes really needed?  This code doesn't use Sockets API
calls like send(2)/recv(2)/shutdown(2) or UNIX Domain Sockets specifics
like struct sockaddr_un.

I suggest dropping these includes (plus they are pulled in implicitly by
osdep.h anyway).

Peter: If there are no other issues with this patch series, could you
remove these two includes when merging?  Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality

[Thomas Huth]

On 2019-01-17 17:16, Julia Suvorova via Qemu-devel wrote:
> Some functional tests for:
>     Basic reception/transmittion
>     Suspending
>     INTEN* registers
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  tests/microbit-test.c | 84 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
> index afeb6b082a..3da6d9529f 100644
> --- a/tests/microbit-test.c
> +++ b/tests/microbit-test.c
> @@ -19,10 +19,93 @@
>  #include "libqtest.h"
>  
>  #include "hw/arm/nrf51.h"
> +#include "hw/char/nrf51_uart.h"
>  #include "hw/gpio/nrf51_gpio.h"
>  #include "hw/timer/nrf51_timer.h"
>  #include "hw/i2c/microbit_i2c.h"
>  
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +
> +static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr)
> +{
> +    int i;
> +
> +    for (i = 0; i < 1000; i++) {
> +        if (qtest_readl(qts, event_addr) == 1) {
> +            qtest_writel(qts, event_addr, 0x00);
> +            return true;
> +        }
> +        g_usleep(10000);
> +    }

So this times out after 10 seconds? ... this is likely plenty on a
normal host, but we run the tests on overloaded CI systems sometimes,
where 10 seconds are not that much...

I'd suggest to replace the condition in the for-loop with "i < 30000" or
even "i < 60000", just to be sure.

> +    return false;
> +}
> +
> +static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in,
> +                           char *out)
> +{
> +    int i, in_len = strlen(in);
> +
> +    g_assert(write(sock_fd, in, in_len) == in_len);

Sorry for being pedantic, but I'd really prefer to write it without
side-effects in g_assert() please. (same for the other occurrences in
this patch)

 Thomas

Re: [Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 2019-01-17 17:16, Julia Suvorova via Qemu-devel wrote:
>> Some functional tests for:
>>     Basic reception/transmittion
>>     Suspending
>>     INTEN* registers
>>
>> Signed-off-by: Julia Suvorova <jusual@mail.ru>
>> ---
>>  tests/microbit-test.c | 84 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 84 insertions(+)
>>
>> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
>> index afeb6b082a..3da6d9529f 100644
>> --- a/tests/microbit-test.c
>> +++ b/tests/microbit-test.c
>> @@ -19,10 +19,93 @@
>>  #include "libqtest.h"
>>
>>  #include "hw/arm/nrf51.h"
>> +#include "hw/char/nrf51_uart.h"
>>  #include "hw/gpio/nrf51_gpio.h"
>>  #include "hw/timer/nrf51_timer.h"
>>  #include "hw/i2c/microbit_i2c.h"
>>
>> +#include <sys/socket.h>
>> +#include <sys/un.h>
>> +
>> +static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < 1000; i++) {
>> +        if (qtest_readl(qts, event_addr) == 1) {
>> +            qtest_writel(qts, event_addr, 0x00);
>> +            return true;
>> +        }
>> +        g_usleep(10000);
>> +    }
>
> So this times out after 10 seconds? ... this is likely plenty on a
> normal host, but we run the tests on overloaded CI systems sometimes,
> where 10 seconds are not that much...
>
> I'd suggest to replace the condition in the for-loop with "i < 30000" or
> even "i < 60000", just to be sure.

Or use a g_timer and look at elapsed time rather than having open coded loops?

>
>> +    return false;
>> +}
>> +
>> +static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in,
>> +                           char *out)
>> +{
>> +    int i, in_len = strlen(in);
>> +
>> +    g_assert(write(sock_fd, in, in_len) == in_len);
>
> Sorry for being pedantic, but I'd really prefer to write it without
> side-effects in g_assert() please. (same for the other occurrences in
> this patch)

+1

--
Alex Bennée

Re: [Qemu-devel] [PATCH v4 0/3] tests/microbit-test: Add UART device test

[Stefan Hajnoczi]

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

On Thu, Jan 17, 2019 at 07:16:37PM +0300, Julia Suvorova wrote:
> v4:
>     * Replace sprintf with g_strdup_printf [Peter]
>     * Move socket connection to qtest library [Peter]
>     * Use memcmp instead of strcmp [Stefan]
>     * Remove using global_qtest [Thomas]
> v3:
>     * Fix directory leak [Stefan]
> 
> Based-on: <20190110094020.18354-1-stefanha@redhat.com>
> 
> Julia Suvorova (3):
>   tests/libqtest: Introduce qtest_init_with_serial()
>   tests/microbit-test: Make test independent of global_qtest
>   tests/microbit-test: Check nRF51 UART functionality
> 
>  tests/libqtest.c      |  26 ++++
>  tests/libqtest.h      |  11 ++
>  tests/microbit-test.c | 331 +++++++++++++++++++++++++++---------------
>  3 files changed, 250 insertions(+), 118 deletions(-)

I posted a minor comment which can be touched up when merging.

Thanks for doing the global_qtest removal!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]