Re: [PATCH] qemu/bitops.h: Locate changed bits

[Ho, Tong]

Point taken, and I am withdrawing this patch.

I will post a new implementation following the arbitrary-length array pattern
in a later date, and only as part of a series including the consuming code.

Thanks,
Tong Ho


From: Peter Maydell 
Sent: Wednesday, May 29, 2024 6:44 AM
To: Ho, Tong 
Cc: qemu-devel@nongnu.org 
Subject: Re: [PATCH] qemu/bitops.h: Locate changed bits

On Wed, 29 May 2024 at 06:05, Tong Ho  wrote:
>
> Add inlined functions to obtain a mask of changed bits.  3 flavors
> are added: toggled, changed to 1, changed to 0.
>
> These newly added utilities aid common device behaviors where
> actions are taken only when a register's bit(s) are changed.

Generally we would expect this kind of "add new utility functions"
patch to appear in a series together with some patches which
actually use the new functions. Otherwise this is all dead code.

More generally:
 * the other bit operations in this file work on bit arrays
   which are arbitrary-length arrays of unsigned long, so
   these new functions don't fit the pattern
 * we have the bitops functions partly because they're inherited
   from the Linux kernel. The use of unsigned long works quite
   badly in QEMU, because for us 'long' is a type that is almost
   always wrong. QEMU devices usually want a type of a known
   length, which is either 'uint32_t' or 'uint64_t'. So I'm
   dubious about adding more functions that work on unsigned long.

thanks
-- PMM

Re: [PATCH] hw/char/pl011: Add support for loopback

[Ho, Tong]

On Thu, Feb 8, 2024 at 3:36 AM, Peter Maydell  wrote:

> This implementation will send the transmitted characters
> to the QEMU chardev and also loop them back into the UART
> when loopback is enabled. Similarly if we receive a character
> from the real input we will put it into the FIFO still, so
> the FIFO will get both looped-back and real input together.

> I think we only have one other UART where loopback is implemented,
> and that is hw/char/serial.c. In that device we make loopback not
> send transmitted characters out when in loopback mode, because
> the 16550 datasheet explicitly says that's how its loopback
> mode works. The PL011 datasheet is unfortunately silent on
> this question. Do you have a real hardware PL011 that you
> can check to see whether when it is in loopback mode
> transmitted data is also sent to the output port as well
> as looped back? Similarly for input: we should check whether
> the UART continues to accept real input or if the real input
> is completely disconnected while in loopback mode.

Hi Peter,

Here is what I found using hardware I have access to.

When loopback is enabled:

1. Receive is disconnected from the real input and
only accepts transmit from loopback.

2. Transmitted characters is sent to both physical
output and loopback to receive.

#2 is also collaborated by commit message for
   https://github.com/torvalds/linux/commit/734745ca

However, the same message also suggested that
#2 may not be the case in other implementations of pl011.

I will work on v2 to address you other comments
as well, with a property for customizing whether
transmit will send to both in loopback mode.

Regards,
Tong Ho

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

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

Re: [PATCH] xlnx-bbram: hw/nvram: Remove deprecated device reset

[Ho, Tong]

Hi Philippe,

Thanks for the review.  The rest is in the pipeline.

Regards,
Tong Ho


From: Philippe Mathieu-Daudé 
Sent: Monday, October 2, 2023 11:23 PM
To: Ho, Tong ; qemu-...@nongnu.org 
Cc: qemu-devel@nongnu.org ; alist...@alistair23.me 
; edgar.igles...@gmail.com ; 
peter.mayd...@linaro.org 
Subject: Re: [PATCH] xlnx-bbram: hw/nvram: Remove deprecated device reset

Hi Tong,

On 3/10/23 07:23, Tong Ho wrote:
> This change implements the ResettableClass interface for the device.
>
> Signed-off-by: Tong Ho 
> ---
>   hw/nvram/xlnx-bbram.c | 8 +---
>   1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Since you did this one, do you mind updating the other Xilinx devices?

$ git grep -F -- '->reset = ' hw/*/*xlnx*
hw/display/xlnx_dp.c:1399:dc->reset = xlnx_dp_reset;
hw/dma/xlnx-zdma.c:827:dc->reset = zdma_reset;
hw/dma/xlnx-zynq-devcfg.c:387:dc->reset = xlnx_zynq_devcfg_reset;
hw/dma/xlnx_csu_dma.c:714:dc->reset = xlnx_csu_dma_reset;
hw/dma/xlnx_dpdma.c:601:dc->reset = xlnx_dpdma_reset;
hw/intc/xlnx-pmu-iomod-intc.c:539:dc->reset = xlnx_pmu_io_intc_reset;
hw/intc/xlnx-zynqmp-ipi.c:362:dc->reset = xlnx_zynqmp_ipi_reset;
hw/misc/xlnx-versal-cfu.c:498:dc->reset = cfu_apb_reset;
hw/nvram/xlnx-bbram.c:526:dc->reset = bbram_ctrl_reset;
hw/nvram/xlnx-versal-efuse-ctrl.c:753:dc->reset = efuse_ctrl_reset;
hw/nvram/xlnx-zynqmp-efuse.c:841:dc->reset = zynqmp_efuse_reset;
hw/rtc/xlnx-zynqmp-rtc.c:258:dc->reset = rtc_reset;
hw/ssi/xlnx-versal-ospi.c:1833:dc->reset = xlnx_versal_ospi_reset;

RE: [PATCH 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

[Ho, Tong]

Hi Peter,

Your recommendation is noted.  Thanks for your valuable input.  I will present 
V2 with better alignment.

Regards,
Tong Ho

-Original Message-
From: Peter Maydell  
Sent: Friday, September 8, 2023 11:35 AM
To: Ho, Tong 
Cc: Richard Henderson ; qemu-...@nongnu.org; 
qemu-devel@nongnu.org; alist...@alistair23.me; edgar.igles...@gmail.com
Subject: Re: [PATCH 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

On Fri, 8 Sept 2023 at 18:56, Ho, Tong  wrote:
>
> Hi Peter,
>
> The Versal TRNG device is required to support both TRNG mode and PRNG 
> mode, and target/guest software selects and changes the mode at will during 
> runtime.
>
> I do agree that, in the TRNG mode, the model using qemu_guest_getrandom() 
> will work without any issues.
>
> When software selects the PRNG mode, the Versal TRNG device is 
> expected to output a reproducible and deterministic sequence of values for a 
> given seed.  This is part of the hardware spec.
>
> I fail to see how qemu_guest_getrandom() can be used to model such 
> requirement correctly.

If the hardware documents a specific RNG that it must use, then yes, we should 
model that, and the comments need to make it clear that we're modelling a very 
specific thing, not merely "here is an arbitrary PRNG".

-- PMM

RE: [PATCH 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

[Ho, Tong]

Hi Peter,

The Versal TRNG device is required to support both TRNG mode and PRNG mode, and 
target/guest
software selects and changes the mode at will during runtime.

I do agree that, in the TRNG mode, the model using qemu_guest_getrandom() will 
work without any issues.

When software selects the PRNG mode, the Versal TRNG device is expected to 
output a reproducible
and deterministic sequence of values for a given seed.  This is part of the 
hardware spec.

I fail to see how qemu_guest_getrandom() can be used to model such requirement 
correctly.

Regards,
Tong Ho

-Original Message-
From: Peter Maydell  
Sent: Friday, September 8, 2023 6:50 AM
To: Ho, Tong 
Cc: Richard Henderson ; qemu-...@nongnu.org; 
qemu-devel@nongnu.org; alist...@alistair23.me; edgar.igles...@gmail.com
Subject: Re: [PATCH 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

On Fri, 1 Sept 2023 at 05:16, Ho, Tong  wrote:
>
> Hi Richard,
>
> Thanks for your input.
>
> I have questions regarding using qemu/guest-random.h for QEMU device models.
>
> Using qemu/guest-random.h, how can this TRNG model ensure its 
> independence from other uses of the same qemu_guest_getrandom() and 
> qemu_guest_random_seed_*()?
>
> By "other uses", I mean components and/or devices using qemu/guest-random.h 
> but unrelated to this Xilinx Versal TRNG device.
>
> By "independent", I mean the Xilinx Versal TRNG device is:
>
> 1. Not impacted by other uses that may or may not need to set the 
> '-seed' option, and
>
> 2. Not impacting other uses just because a Xilinx Versal machine user decides 
> to use deterministic mode *only" for this TRNG device.
>
> Also, I am at a loss in how unrelated QEMU devices can remain independent 
> when:
>
> 3. qemu/guest-random.h uses '__thread' variable for GRand context, but
>
> 4. QEMU devices run mostly as co-routines and not as separate threads.

You shouldn't need to care about any of this. Just assume you can get decent 
quality random numbers from qemu_guest_getrandom() or 
qemu_guest_getrandom_nofail(). The -seed option is for the entire simulation, 
not specific to individual RNG devices.

> I suppose the Versal TRNG implementation could use g_rand_*() 
> directly, having a GRand object in the device state and seeding through 
> g_rand_set_seed_array().

Don't do something non-standard. Write this RNG device the same way we do all 
other RNG devices in QEMU.

thanks
-- PMM

RE: [PATCH 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

[Ho, Tong]

Hi Richard,

Thanks for your input.

I have questions regarding using qemu/guest-random.h for QEMU device models.

Using qemu/guest-random.h, how can this TRNG model ensure its independence from
other uses of the same qemu_guest_getrandom() and qemu_guest_random_seed_*()?

By "other uses", I mean components and/or devices using qemu/guest-random.h but 
unrelated to this Xilinx Versal TRNG device.

By "independent", I mean the Xilinx Versal TRNG device is:

1. Not impacted by other uses that may or may not need to set the '-seed' 
option, and

2. Not impacting other uses just because a Xilinx Versal machine user decides 
to use deterministic mode *only" for this TRNG device.

Also, I am at a loss in how unrelated QEMU devices can remain independent when:

3. qemu/guest-random.h uses '__thread' variable for GRand context, but

4. QEMU devices run mostly as co-routines and not as separate threads.

I suppose the Versal TRNG implementation could use g_rand_*() directly,
having a GRand object in the device state and seeding through 
g_rand_set_seed_array().

Best regards,
Tong Ho

-Original Message-
From: Richard Henderson  
Sent: Thursday, August 31, 2023 4:45 PM
To: Ho, Tong ; qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org; alist...@alistair23.me; edgar.igles...@gmail.com; 
peter.mayd...@linaro.org
Subject: Re: [PATCH 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

On 8/31/23 10:18, Tong Ho wrote:
> This adds a non-cryptographic grade implementation of the model for 
> the True Random Number Generator (TRNG) component in AMD/Xilinx Versal 
> device family.
> 
> This model is only intended for non-real world testing of guest 
> software, where cryptographically strong TRNG is not needed.
> 
> This model supports versions 1 & 2 of the Versal TRNG, with default to 
> be version 2; the 'hw-version' uint32 property can be set to 0x0100 to 
> override the default.
> 
> Other implemented properties:
> - 'forced-prng', uint64
>When set to non-zero, "true random reseed" is replaced by
>deterministic reseed based on the given value and other
>deterministic parameters, even when guest software has
>configured the TRNG as "true random reseed".  This option
>allows guest software to reproduce data-dependent defects.
> 
> - 'fips-fault-events', uint32, bit-mask
>bit 3: Triggers the SP800-90B entropy health test fault irq
>bit 1: Triggers the FIPS 140-2 continuous test fault irq
> 
> Signed-off-by: Tong Ho
> ---
>   hw/misc/Kconfig|   3 +
>   hw/misc/meson.build|   3 +
>   hw/misc/xlnx-versal-trng.c | 725 +
>   include/hw/misc/xlnx-versal-trng.h |  58 +++
>   4 files changed, 789 insertions(+)
>   create mode 100644 hw/misc/xlnx-versal-trng.c
>   create mode 100644 include/hw/misc/xlnx-versal-trng.h

I don't think you should be inventing another PRNG, or related properties.

We already have qemu/guest-random.h, and the -seed command-line parameter to 
force the use of a deterministic PRNG with a given seed value.


r~