Re: [PATCH v4 3/3] tests/qtest: Introduce tests for AMD/Xilinx Versal TRNG device

[Peter Maydell]

On Tue, 17 Oct 2023 at 20:32, Tong Ho <tong.ho@amd.com> wrote:
>
> Signed-off-by: Tong Ho <tong.ho@amd.com>
> ---
>  tests/qtest/meson.build             |   2 +-
>  tests/qtest/xlnx-versal-trng-test.c | 486 ++++++++++++++++++++++++++++
>  2 files changed, 487 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qtest/xlnx-versal-trng-test.c


> +static void trng_wait(uint32_t wait_mask, bool on, const char *act)
> +{
> +    time_t tmo = time(NULL) + 2; /* at most 2 seconds */

Don't put timeouts this short into tests, please; ideally,
avoid them entirely. Sometimes the CI machines are heavily
loaded and the test might not be able to complete in a
short time like this. If you must have a timeout, it should
be at least a minute. (And see below on whether we need one.)

> +    uint32_t event_mask = 0;
> +    uint32_t clear_mask = 0;
> +
> +    /*
> +     * Only selected bits are events in R_TRNG_STATUS, and
> +     * clear them needs to go through R_INT_CTRL.
> +     */
> +    if (wait_mask & TRNG_STATUS_CERTF_MASK) {
> +        event_mask |= TRNG_STATUS_CERTF_MASK;
> +        clear_mask |= TRNG_INT_CTRL_CERTF_RST_MASK;
> +    }
> +    if (wait_mask & TRNG_STATUS_DTF_MASK) {
> +        event_mask |= TRNG_STATUS_DTF_MASK;
> +        clear_mask |= TRNG_INT_CTRL_DTF_RST_MASK;
> +    }
> +    if (wait_mask & TRNG_STATUS_DONE_MASK) {
> +        event_mask |= TRNG_STATUS_DONE_MASK;
> +        clear_mask |= TRNG_INT_CTRL_DONE_RST_MASK;
> +    }
> +
> +    for (;;) {
> +        bool sta = !!(trng_status() & event_mask);
> +
> +        if ((on ^ sta) == 0) {
> +            break;
> +        }
> +
> +        if (time(NULL) >= tmo) {
> +            FAILED("%s: Timed out waiting for event 0x%x to be %d%s",
> +                   act, event_mask, (int)on, trng_info());
> +        }
> +
> +        g_usleep(10000);

Why does this test need to use sleeps and timeouts?
A qtest test controls the guest 'clock' directly, so
usually they're completely deterministic. Is there some
behaviour in the TRNG device which is asynchronous (in
a way not driven from the QEMU guest clock) that I've missed ?

> +    }
> +
> +    /* Remove event */
> +    trng_bit_set(R_TRNG_INT_CTRL, clear_mask);
> +
> +    if (!!(trng_read(R_TRNG_STATUS) & event_mask)) {
> +        FAILED("%s: Event 0x%0x stuck at 1 after clear: %s",
> +               act, event_mask, trng_info());
> +    }
> +}

thanks
-- PMM