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

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

[Andre Przywara]

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@arm.com>
Reported-by: Peter Hoyes <peter.hoyes@arm.com>
---
 drivers/virtio/virtio_rng.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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

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

[Simon Glass]

Hi Andre,

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

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

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

[Andre Przywara]

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

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

[Heinrich Schuchardt]

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.

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

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
>

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

[Andre Przywara]

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

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

[Simon Glass]

Hi Andre,

On Fri, 10 Nov 2023 at 07:16, Andre Przywara <andre.przywara@arm.com> 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.
> > >
> > > 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>
> > > ---
> > >  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?)


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

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

[Andre Przywara]

On Sat, 11 Nov 2023 20:08:36 -0700
Simon Glass <sjg@chromium.org> wrote:

Hi,

> On Fri, 10 Nov 2023 at 07:16, Andre Przywara <andre.przywara@arm.com> 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.
> > > >
> > > > 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>
> > > > ---
> > > >  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.

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

Re: 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@chromium.org> wrote:
>
> Hi,
>
>> On Fri, 10 Nov 2023 at 07:16, Andre Przywara <andre.przywara@arm.com> 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.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>   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
>

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]

On Tue, Nov 07, 2023 at 04:09:00PM +0000, Andre Przywara 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@arm.com>
> Reported-by: Peter Hoyes <peter.hoyes@arm.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]