Re: [PATCH v3 4/6] hw/misc: Add npcm7xx random number generator

[Havard Skinnemoen]

On Fri, Oct 30, 2020 at 6:34 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 23 Oct 2020 at 22:06, Havard Skinnemoen <hskinnemoen@google.com> 
> wrote:
> >
> > The RNG module returns a byte of randomness when the Data Valid bit is
> > set.
> >
> > This implementation ignores the prescaler setting, and loads a new value
> > into RNGD every time RNGCS is read while the RNG is enabled and random
> > data is available.
> >
> > A qtest featuring some simple randomness tests is included.
> >
> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
>
> I've just seen some intermittent failures on the rng tests
> in this patch:
>
> PASS 1 qtest-arm/npcm7xx_rng-test /arm/npcm7xx_rng/enable_disable
> PASS 2 qtest-arm/npcm7xx_rng-test /arm/npcm7xx_rng/rosel
> **
> ERROR:../../tests/qtest/npcm7xx_rng-test.c:190:test_continuous_monobit:
> assertion failed (calc_monobit_p(buf, sizeof(buf)) > 0.01):
> (0.00800994233 > 0.01)
>
> (on OSX)
>
> and (on x86-64 Linux):
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/home/petmay01/li
> naro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-arm tests/qtest/npcm7xx_rng-test --tap
> -k
> PASS 1 qtest-arm/npcm7xx_rng-test /arm/npcm7xx_rng/enable_disable
> PASS 2 qtest-arm/npcm7xx_rng-test /arm/npcm7xx_rng/rosel
> PASS 3 qtest-arm/npcm7xx_rng-test /arm/npcm7xx_rng/continuous/monobit
> **
> ERROR:../../tests/qtest/npcm7xx_rng-test.c:211:test_continuous_runs:
> assertion failed (calc_runs_p(buf.l, sizeof(buf) * BITS_PER_BYTE) >
> 0.01): (0.00198053202 > 0.01)
>
>
> Are these just "we got a bit unlucky in the values we got from
> the RNG" ?

Yes, it looks like the randomness is a little low.

> If so, we probably need to disable these tests from the set
> that we run in "make check" -- we can't really have "there's
> a non-zero chance that the test fails" tests in our CI loop...

Agree. Is there a way to disable the test by default, or should I send
a patch to remove it?

Havard