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

[Andre Przywara <andre.przywara@arm.com>]

On Sat, 11 Nov 2023 10:39:40 +0100
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

Hi Heinrich,

thanks for having a look!

> On 11/10/23 15:16, Andre Przywara wrote:
> > On Fri, 10 Nov 2023 05:53:59 -0700
> > Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Simon,
> >  
> >> On Tue, 7 Nov 2023 at 09:09, Andre Przywara <andre.przywara@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.  
> 
> v0.9 was a draft. So nothing we have to care about.
> 
> >>>
> >>> 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@arm.com>
> >>> Reported-by: Peter Hoyes <peter.hoyes@arm.com>  
> 
> The bug should be fixed in the Arm Fixed Virtual Platform instead of
> working around it in U-Boot.

... and it is already fixed in the internal releases, but external users
have to wait for the next public release in a few weeks, and then
everyone would need to update as well. So I was hoping that we could
just have this simple software fix?
After all non-spec compliant hardware is quite common, and we have tons
of quirks and workarounds for every kind of erratum or hardware problem.
I don't think this one here is particularly intrusive. If you want, I
can hide it behind a Kconfig option?

> Our driver should return -EIO if the virtio host is not compliant.

That sounds a bit over the top to detect this and deliberately deny
service, when we could just be robust and carry on anyways?

Cheers,
Andre

> 
> Best regards
> 
> Heinrich
> 
> >>> ---
> >>>   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  
> >  
>