ARMv8.5 RNG driver (was :Re: [PATCH] virtio: rng: gracefully handle 0 byte returns)

[Heinrich Schuchardt]

On 11/12/23 12:53, Andre Przywara wrote:
> On Sat, 11 Nov 2023 20:08:36 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
>> On Fri, 10 Nov 2023 at 07:16, Andre Przywara <andre.przywara at arm.com> wrote:
>>>
>>> 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.
>>
>> Would it make sense to model this as a memory-mapped peripheral under
>> /soc (perhaps one without an address?)
>
> No, absolutely not. What I mentioned above is a somewhat hidden
> implementation detail of that *particular core*. One big reason for
> having those architected *system registers* is to do away with all
> those implementation specific ways to access an entropy source, and
> make this dead easy for software (including userland!) to use it:
> Check the CPU ID register, read the sysreg. No prior knowledge required.
>
> I now deeply regret sending this Armv8.5 RNG driver. I have an
> alternative solution, just got distracted later this week to finish
> this up.
> Let's have a discussion there, or we find a way to probe UCLASS_RNG
> drivers other than through devicetree nodes. If U-Boot really insists on
> matching drivers to DT nodes 1:1, that's a really limiting design
> decision, and we should not proliferate this by shoehorning everyone
> and their dog into devicetree.

For the RISC-V Zkr driver Tom said we cannot expect QEMU and Linux to
change how the device-tree is set up to model CPU registers and the
usage of U_BOOT_DRVINFO() for these architecturally defined registers is
fine.

I would assume the same in the ARM case.

Best regards

Heinrich

>
> 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
>