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.mayd...@linaro.org>
Sent: Friday, October 27, 2023 6:03 AM
To: Ho, Tong <tong...@amd.com>
Cc: qemu-...@nongnu.org <qemu-...@nongnu.org>; qemu-devel@nongnu.org
<qemu-devel@nongnu.org>; alist...@alistair23.me <alist...@alistair23.me>;
edgar.igles...@gmail.com <edgar.igles...@gmail.com>;
richard.hender...@linaro.org <richard.hender...@linaro.org>;
frasse.igles...@gmail.com <frasse.igles...@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...@amd.com> wrote:
>
> Signed-off-by: Tong Ho <tong...@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
