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

[Havard Skinnemoen]

On Tue, Oct 20, 2020 at 6:02 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 9 Oct 2020 at 00:22, 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>
> > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
>
>
> > +static const VMStateDescription vmstate_npcm7xx_rng = {
> > +    .name = "npcm7xx-rng",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(rngcs, NPCM7xxRNGState),
> > +        VMSTATE_UINT8(rngd, NPCM7xxRNGState),
> > +        VMSTATE_UINT8(rngmode, NPCM7xxRNGState),
> > +    },
>
> This is missing the VMSTATE_END_OF_LIST() terminator.
>
> > +};
> > +
>
> > --- /dev/null
> > +++ b/tests/qtest/npcm7xx_rng-test.c
> > @@ -0,0 +1,278 @@
> > +/*
> > + * QTest testcase for the Nuvoton NPCM7xx Random Number Generator
> > + *
> > + * Copyright 2020 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but 
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > + * for more details.
> > + */
> > +
> > +#include <math.h>
> > +
> > +#include "qemu/osdep.h"
>
> osdep.h must always be the first #include line, before anything
> else, including system includes.
>
> > + * Verifies that the first data byte collected after enabling the RNG 
> > satisfies
> > + * a monobit test.
>
> Some of this "is this really a random number" testing seems a bit
> overkill given that the RNG device is just plumbing through the
> qemu_guest_getrandom() result, but since you've written the code
> we may as well keep it :-)

You may be right, though there were a couple of reasons why I added it.

One is that rngd was complaining about /dev/hwrng failing randomness
tests, so I wanted to make sure my device wasn't doing anything to
ruin the randomness. In the end, it turned out to be a kernel bug:
https://lkml.org/lkml/2020/9/23/1021

Another reason is that the value from qemu_guest_getrandom() isn't
consumed directly when reading the data register; it's latched when
reading the status register and returned on the next read from the
data register. So it's possible to imagine a bug causing the same
value to be returned over and over, which should cause the randomness
test to fail.

> If you fix the #include order and the missing terminator then

Will do, thanks!

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM