Re: [PATCH v2 6/6] tests/qtest: Add ivshmem-flat test

[Thomas Huth]

On 16/02/2024 15.44, Philippe Mathieu-Daudé wrote:
> From: Gustavo Romero <gustavo.romero@linaro.org>
>
> Add qtest for the ivshmem-flat device.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> Message-ID: <20231127052024.435743-4-gustavo.romero@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/ivshmem-flat-test.c | 320 ++++++++++++++++++++++++++++++++
>   tests/qtest/meson.build         |   2 +
>   2 files changed, 322 insertions(+)
>   create mode 100644 tests/qtest/ivshmem-flat-test.c
>
> diff --git a/tests/qtest/ivshmem-flat-test.c b/tests/qtest/ivshmem-flat-test.c
> new file mode 100644
> index 0000000000..5489a0d915
> --- /dev/null
> +++ b/tests/qtest/ivshmem-flat-test.c
> @@ -0,0 +1,320 @@
> +/*
> + * Inter-VM Shared Memory Flat Device qtests
> + *
> + * SPDX-FileCopyrightText: 2023 Linaro Ltd.
> + * SPDX-FileContributor: Gustavo Romero <gustavo.romero@linaro.org>
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + */
> +
> +#include "ivshmem-utils.h"
> +
> +#define IVSHMEM_FLAT_MMR_ADDR 0x400FF000
> +#define IVSHMEM_FLAT_SHM_ADDR 0x40100000
> +#define SHM_SIZE 131072 /* 128k */
> +
> +static ServerThread thread;
> +
> +uint32_t *shm_ptr;
> +char *shm_rel_path;
> +char *server_socket_path;
> +
> +static void cleanup(void)
> +{
> +    if (shm_ptr) {
> +        munmap(shm_ptr, SHM_SIZE);
> +        shm_ptr = NULL;
> +    }
> +
> +    if (shm_rel_path) {
> +        shm_unlink(shm_rel_path);
> +        shm_rel_path = NULL;
> +    }
> +
> +    if (server_socket_path) {
> +        unlink(server_socket_path);
> +        server_socket_path = NULL;
> +    }
> +}
> +
> +static void abort_handler(void *data)
> +{
> +    test_ivshmem_server_stop(&thread);
> +    cleanup();
> +}
> +
> +/*
> + * Check if exactly 1 positive pulse (low->high->low) on 'irq' IRQ line happens
> + * in 'timeout' second(s). 'irq' must be intercepted using 
> qtest_irq_intercept_*
> + * before this function can be used on it. It returns 0 when pulse is detected,
> + * otherwise 1.
> + */
> +static int test_ivshmem_flat_irq_positive_pulse(QTestState *qts, int irq,
> +                                                int timeout)
> +{
> +    uint64_t num_raises = 0;
> +    uint64_t num_lows = 0;
> +    uint64_t end_time;
> +
> +    end_time = g_get_monotonic_time() + timeout * G_TIME_SPAN_SECOND;
> +    do {
> +        num_raises = qtest_get_irq_raised_counter(qts, 0);
> +        if (num_raises) {
> +            num_lows = qtest_get_irq_lowered_counter(qts, 0);
> +            /* Check for 1 raise and 1 low IRQ event. */
> +            if (num_raises == num_lows && num_lows == 1) {
> +                return 0;
> +            } else {
> +                g_message("%s: Timeout expired", __func__);
> +                return 1;
> +            }
> +        }
> +        qtest_clock_step(qts, 10000);
> +    } while (g_get_monotonic_time() < end_time);
> +
> +    return 1;
> +}

The timeout stuff looks like it will be quite sensitive to slow CI runners. 
Timeout is set to 2 seconds in most callers - but on very loaded systems, it 
can easily happen that a job gets interrupted for two seconds.

I think you should either increase the 2 seconds to something bigger (maybe 
use a #define for it), or instead of using g_get_monotonic_time(), rather 
loop a certain number of iterations and use a g_sleep(10000) in the loop 
body - that will also relax the host CPU while the test waits for QEMU to 
progress?

> +static inline uint32_t read_reg(QTestState *qts, enum Reg reg)
> +{
> +    uint32_t v;
> +
> +    qtest_memread(qts, IVSHMEM_FLAT_MMR_ADDR + reg, &v, sizeof(v));
> +
> +    return v;
> +}
> +
> +static inline void write_reg(QTestState *qts, enum Reg reg, uint32_t v)
> +{
> +    qtest_memwrite(qts, IVSHMEM_FLAT_MMR_ADDR + reg, &v, sizeof(v));
> +}
> +
> +/*
> + * Setup a test VM with ivshmem-flat device attached, IRQ properly set, and
> + * connected to the ivshmem-server.
> + */
> +static QTestState *setup_vm(void)
> +{
> +    QTestState *qts;
> +    const char *cmd_line;
> +
> +    cmd_line = g_strdup_printf("-machine lm3s6965evb "

Could you please add a check, either in meson.build or in main(), to make 
sure that the lm3s6965evb machine is really available when running this 
test? ... it might have been disabled in the build ...

> +                               "-chardev socket,path=%s,id=ivshm "
> +                               "-device ivshmem-flat,chardev=ivshm,"
> +                               
> "x-irq-qompath='/machine/unattached/device[1]/nvic/unnamed-gpio-in[0]',"
> +                               "x-bus-qompath='/sysbus',shmem-size=%d",
> +                               server_socket_path, SHM_SIZE);
> +    qts = qtest_init(cmd_line);
> +
> +    return qts;
> +}

 Thomas