[PATCH] driver: rng: Add DM_RNG interface for ARMv8.5 RNDR registers

[Andre Przywara]

On Wed, 30 Aug 2023 20:49:18 -0600
Simon Glass <sjg at chromium.org> wrote:

Hi Simon,

> On Wed, 30 Aug 2023 at 05:32, Andre Przywara <andre.przywara at arm.com> wrote:
> >
> > The ARMv8.5 architecture extension defines architectural RNDR/RNDRRS
> > system registers, that provide 64 bits worth of randomness on every
> > read. Since it's an extension, and implementing it is optional, there is
> > a field in the ID_AA64ISAR0_EL1 ID register to query the availability
> > of those registers.
> >
> > Add a UCLASS_RNG driver that returns entropy via repeated reads from
> > those system registers, if the extension is implemented.
> > The driver always binds, but checks the availability in the probe()
> > routine.
> >
> > This helps systems which suffer from low boot entropy, since U-Boot can
> > provide entropy via the generic UEFI entropy gathering protocol to the OS,
> > at an early stage.
> >
> > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > ---
> > Hi Simon,
> >
> > not sure if this is the expected way to model a driver which autodetects
> > its device at runtime. It somewhat works, but always lists the bound
> > device, even if the registers are not supported. If I do the check in bind
> > instead, it fails booting when this returns -ENODEV, since it expects
> > the fixed device to always bind successfully, I guess?
> > Is there any other way of modeling this? And before you say your
> > favourite two letters: this is not a DT job, since it can be safely
> > auto-detected in an architectural way.
> >
> > Thanks,
> > Andre
> >
> >  arch/arm/include/asm/system.h |  1 +
> >  drivers/rng/Kconfig           |  6 +++
> >  drivers/rng/Makefile          |  1 +
> >  drivers/rng/arm_rndr.c        | 82 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 90 insertions(+)
> >  create mode 100644 drivers/rng/arm_rndr.c  
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>

thanks!

> nit below
> 
> >
> > diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
> > index 87d1c77e8b1..0eae857e73a 100644
> > --- a/arch/arm/include/asm/system.h
> > +++ b/arch/arm/include/asm/system.h
> > @@ -84,6 +84,7 @@
> >  #define HCR_EL2_HCD_DIS                (1 << 29) /* Hypervisor Call disabled         */
> >  #define HCR_EL2_AMO_EL2                (1 <<  5) /* Route SErrors to EL2             */
> >
> > +#define ID_AA64ISAR0_EL1_RNDR  (0xFUL << 60) /* RNDR random registers */
> >  /*
> >   * ID_AA64ISAR1_EL1 bits definitions
> >   */
> > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> > index 5deb5db5b71..72788d479cc 100644
> > --- a/drivers/rng/Kconfig
> > +++ b/drivers/rng/Kconfig
> > @@ -76,6 +76,12 @@ config RNG_SMCCC_TRNG
> >           Enable random number generator for platforms that support Arm
> >           SMCCC TRNG interface.
> >
> > +config RNG_ARM_RNDR
> > +       bool "Generic ARMv8.5 RNDR register"
> > +       depends on DM_RNG && ARM64
> > +       help
> > +         Use the ARMv8.5 RNDR register to provide random numbers.
> > +
> >  config TPM_RNG
> >         bool "Enable random number generator on TPM device"
> >         depends on TPM
> > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > index 78f61051acf..572fa7a0c66 100644
> > --- a/drivers/rng/Makefile
> > +++ b/drivers/rng/Makefile
> > @@ -13,4 +13,5 @@ obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> >  obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
> >  obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> >  obj-$(CONFIG_RNG_SMCCC_TRNG) += smccc_trng.o
> > +obj-$(CONFIG_RNG_ARM_RNDR) += arm_rndr.o
> >  obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> > diff --git a/drivers/rng/arm_rndr.c b/drivers/rng/arm_rndr.c
> > new file mode 100644
> > index 00000000000..55989743eae
> > --- /dev/null
> > +++ b/drivers/rng/arm_rndr.c
> > @@ -0,0 +1,82 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2023, Arm Ltd.
> > + *
> > + * Use the (optional) ARMv8.5 RNDR register to provide random numbers to
> > + * U-Boot's UCLASS_RNG users.
> > + * Detection is done at runtime using the CPU ID registers.
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_RNG
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <linux/kernel.h>
> > +#include <rng.h>
> > +#include <asm/system.h>
> > +
> > +#define DRIVER_NAME    "arm-rndr"
> > +
> > +static bool cpu_has_rndr(void)
> > +{
> > +       uint64_t reg;
> > +
> > +       __asm__ volatile("mrs %0, ID_AA64ISAR0_EL1\n" : "=r" (reg));
> > +       return !!(reg & ID_AA64ISAR0_EL1_RNDR);
> > +}
> > +
> > +/*
> > + * The system register name is RNDR, but this isn't widely known among older
> > + * toolchains, and also triggers errors because of it being an architecture
> > + * extension. Since we check the availability of the register before, it's
> > + * fine to use here, though.
> > + */
> > +#define RNDR   "S3_3_C2_C4_0"
> > +
> > +static uint64_t read_rndr(void)
> > +{
> > +       uint64_t reg;
> > +
> > +       __asm__ volatile("mrs %0, " RNDR "\n" : "=r" (reg));
> > +
> > +       return reg;
> > +}
> > +
> > +static int arm_rndr_read(struct udevice *dev, void *data, size_t len)
> > +{
> > +       uint64_t random;
> > +
> > +       while (len) {
> > +               int tocopy = min(sizeof(uint64_t), len);
> > +
> > +               random = read_rndr();
> > +               memcpy(data, &random, tocopy);
> > +               len -= tocopy;
> > +               data += tocopy;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_rng_ops arm_rndr_ops = {
> > +       .read = arm_rndr_read,
> > +};
> > +
> > +static int arm_rndr_probe(struct udevice *dev)
> > +{
> > +       if (!cpu_has_rndr())
> > +               return -ENODEV;  
> 
> Can you put this in a bind() function instead, if not too expensive?

That's the point I was wondering about (see my message to you above, after
the S-o-B): it seems more natural there, but stops the boot.
Apparently DM expects those fixed devices (U_BOOT_DRVINFO) to always
succeed their bind() calls? Because if I put it in bind, I get:
================
No match for driver 'arm-rndr'
initcall sequence 00000000fefc4528 failed at call 000000008801d104 (err=-19)
### ERROR ### Please RESET the board ###
================
if the feature is not available.
Any suggestions how to use bind and avoid that crash are highly welcome.

And regarding "expensive": This is reading a fixed, read-only system
register. There is probably nothing cheaper than that on a modern chip.

Cheers,
Andre

> > +
> > +       return 0;
> > +}
> > +
> > +U_BOOT_DRIVER(arm_rndr) = {
> > +       .name = DRIVER_NAME,
> > +       .id = UCLASS_RNG,
> > +       .ops = &arm_rndr_ops,
> > +       .probe = arm_rndr_probe,
> > +};
> > +
> > +U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > +       .name = DRIVER_NAME,
> > +};
> > --
> > 2.25.1
> >  
> 
> Regards,
> Simon