[PATCH] virtio: rng: gracefully handle 0 byte returns

[Andre Przywara]

On Fri, 10 Nov 2023 05:53:59 -0700
Simon Glass <sjg at chromium.org> wrote:

Hi Simon,

> On Tue, 7 Nov 2023 at 09:09, Andre Przywara <andre.przywara at arm.com> wrote:
> >
> > According to the virtio v1.x "entropy device" specification, a virtio-rng
> > device is supposed to always return at least one byte of entropy.
> > However the virtio v0.9 spec does not mention such a requirement.
> >
> > The Arm Fixed Virtual Platform (FVP) implementation of virtio-rng always
> > returns 8 bytes less of entropy than requested. If 8 bytes or less are
> > requested, it will return 0 bytes.
> > This behaviour makes U-Boot's virtio_rng_read() implementation go into an
> > endless loop, hanging the system.
> >
> > Work around this problem by always requesting 8 bytes more than needed,
> > but only if a previous call to virtqueue_get_buf() returned 0 bytes.
> >
> > This should never trigger on a v1.x spec compliant implementation, but
> > fixes the hang on the Arm FVP.
> >
> > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > Reported-by: Peter Hoyes <peter.hoyes at arm.com>
> > ---
> >  drivers/virtio/virtio_rng.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)  
> 
> Unrelated to this patch, but do you know the hardware architecture of
> the ARM RNG? Is there one RNG unit per CPU or one for the whole SoC?

Architecturally and from a software perspective the ARMv8.5 FEAT_RNG
feature is a system register, so per-core. Theoretically the availability
could differ between cores, but the CPU ID feature registers are also
per-core, so as long as we run on a single core, or always at least
read from the same core, it's all good.

Now the architecture only describes the CPU instruction aspect of the
feature, and establishes rules for the quality and spec conformance, but
leaves the actual source of the entropy open to the integrator.

The manual in the Neoverse V1 core[1] (still not a SoC!) states that the
actual entropy source is a memory mapped peripheral, its address being
held in an internal, per-core register. So you can have one shared entropy
source per SoC, or a private instance for each core, that's up to the
actual integrator to design.

From the software perspective this shouldn't matter, though: the feature
is "per-core", how this is backed is an implementation detail.

Cheers,
Andre

[1]
https://developer.arm.com/documentation/101427/0102/Functional-description/Random-number-support/About-the-random-number-support


> > diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> > index b85545c2ee5..786359a6e36 100644
> > --- a/drivers/virtio/virtio_rng.c
> > +++ b/drivers/virtio/virtio_rng.c
> > @@ -20,7 +20,7 @@ struct virtio_rng_priv {
> >  static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
> >  {
> >         int ret;
> > -       unsigned int rsize;
> > +       unsigned int rsize = 1;
> >         unsigned char buf[BUFFER_SIZE] __aligned(4);
> >         unsigned char *ptr = data;
> >         struct virtio_sg sg;
> > @@ -29,7 +29,12 @@ static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
> >
> >         while (len) {
> >                 sg.addr = buf;
> > -               sg.length = min(len, sizeof(buf));
> > +               /*
> > +                * Work around implementations which always return 8 bytes
> > +                * less than requested, down to 0 bytes, which would
> > +                * cause an endless loop otherwise.
> > +                */
> > +               sg.length = min(rsize ? len : len + 8, sizeof(buf));
> >                 sgs[0] = &sg;
> >
> >                 ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
> > --
> > 2.25.1
> >  
> 
> Regards,
> Simon