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

[Ho, Tong]

Hi Peter,

This is in regard to your comment on the use of g_usleep() in waiting for

an event-bit update from the device under test.

The TRNG device model presented in patch #1 does not have any asynchronous behavior.

So, do you mean that, although the qtest process and the qemu-system-* process

are running as 2 separate processes, qtest infrastructure already has the necessary

mechanism to ensure that:

1. After qtest test sets a register that has the correct deterministic behavior to update an event bit,

2. The same qtest test subsequently issuing a register read will be guaranteed to observe the updated bit?

If that is the case, then there would be no need for any timeout. Instead,

a qtest test can declare fail when the expected bit update is not observed by the test.

I would like to know.

Thanks,

Tong Ho

---

From: Peter Maydell <peter.maydell@linaro.org>
Sent: Friday, October 27, 2023 6:03 AM
To: Ho, Tong <tong.ho@amd.com>
Cc: qemu-arm@nongnu.org <qemu-arm@nongnu.org>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>; alistair@alistair23.me <alistair@alistair23.me>; edgar.iglesias@gmail.com <edgar.iglesias@gmail.com>; richard.henderson@linaro.org <richard.henderson@linaro.org>; frasse.iglesias@gmail.com <frasse.iglesias@gmail.com>
Subject: Re: [PATCH v4 3/3] tests/qtest: Introduce tests for AMD/Xilinx Versal TRNG device

 

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