On Sat, 11 Nov 2023 10:39:40 +0100 Heinrich Schuchardt <xypron.g...@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 <s...@chromium.org> wrote: > > > > Hi Simon, > > > >> On Tue, 7 Nov 2023 at 09:09, Andre Przywara <andre.przyw...@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.przyw...@arm.com> > >>> Reported-by: Peter Hoyes <peter.ho...@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 > > >