[PATCH v3 0/2] rng: Provide a RNG based on the RISC-V Zkr ISA extension

[Ilias Apalodimas]

Hi
I am late to the party but

[...]

> > I can't help to think that you like the FDT as a well understood and
> > flexible general purpose data structure. And it can indeed be used as a
> > configuration file, especially since you have the parser in your code
> > already - the FIT image is a good example. But this is distinct from the
> > Devicetree as a hardware description.
>
> Indeed, although I don't see such a bright line between the
> hardware/software concepts. In any case, the RNG is a hardware
> feature, surely?
>
> > Would it help to separate the two use cases: to go with one DTB that is
> > strictly for hardware configuration, as described by the DT bindings in
> > the Linux kernel tree, and a *separate* DT blob that carries configuration
> > information, U-Boot specific data (like memory layout, SRAM usage, device
> > priorities, packaging information)? TF-A chose this approach: there is
> > hw-config, which is the hardware DTB, and there is fw-config, which is a
> > FDT blob as well, but describes the firmware layout and other configuration
> > information.
>
> Or we could just not do that and have a single DT :-)
>
> I wondered where that idea came from and it has been mentioned before.

Yes, that was me.  I still think this would solve a *ton* of problems
and I am willing to explore that.
The only thing I haven't thought through is DTs and limited SRAM in
the SPL, but I don't think this should be a showstopper.

Thanks
/Ilias
> TF-A should be a C library called by bootloaders, BTW.
>
> I haven't replied to everything above...I believe I have had this same
> discussion about a dozen times over the last 6 years. I did try to
> send a patch to state my POV at some point, but I think it got NAKed
> :-)
>
> Regards,
> Simon
>
>
> >
> > Cheers,
> > Andre
> >
> > > > > > > > But as Heinrich also said: those instructions are not
> > > > > > > > peripherals, they are part of an instruction set extensions,
> > > > > > > > the same story as with x86's RDRAND instruction. We don't have
> > > > > > > > those in ACPI or so as well, because CPUID has you covered.
> > > > > > > > The same on ARM, ID_AA64ISAR0_EL1 is readable on every chip
> > > > > > > > (outside of EL0), and tells you whether you have the RNDR
> > > > > > > > register or not. IIUC RISC-V is slightly different here, since
> > > > > > > > not all ISA extensions are covered by CSRs, hence some of them
> > > > > > > > indeed listed in the DT.
> > > > > > > >
> > > > > > > > So a proper solution(TM) would be to split this up in
> > > > > > > > architectural *instructions* and proper TRNG *devices*, maybe
> > > > > > > > wrapping this up in some function that tests both. This is
> > > > > > > > roughly what the kernel does, somewhat abstracted by the
> > > > > > > > concept of "entropy sources", which could be TRNG devices, CPU
> > > > > > > > instructions, interrupt jitter or even "instruction execution
> > > > > > > > jitter"[1], with the latter two definitely not being devices
> > > > > > > > really at all.
> > > > > > > >
> > > > > > > > But I don't know if U-Boot wants to go through the hassle of
> > > > > > > > this whole framework, as we tend to implement things much
> > > > > > > > easier. But a simple get_cpu_random() function, implemented
> > > > > > > > per architecture, and with some kind of success flag, should
> > > > > > > > be easy enough to do. Then either the users (UEFI?) explicitly
> > > > > > > > call this before trying UCLASS_RNG, or we wrap this for every
> > > > > > > > RNG user.
> > > > > > > >
> > > > > > > > Cheers,
> > > > > > > > Andre
> > > > > > > >
> > > > > > > > > > > > > VExpress64# rng 1
> > > > > > > > > > > > > 00000000: f3 88 b6 d4 24 da 49 ca 49 f7 9e 66 5f 12
> > > > > > > > > > > > > 07 b2  ....$.I.I..f_...
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Essentially in any case were you have multiple drivers
> > > > > > > > > > > > for the same device using uclass_get_device(, 0, ) and
> > > > > > > > > > > > uclass_find_first_device() will only give you the
> > > > > > > > > > > > first bound device and not the first successfully
> > > > > > > > > > > > probed device. Furthermore neither of this functions
> > > > > > > > > > > > causes probing. This is not restricted to the RNG
> > > > > > > > > > > > drivers but could also happen with multiple TPM
> > > > > > > > > > > > drivers or multiple watchdogs.
> > > > > > > > > > > >
> > > > > > > > > > > > This patch is related to the problem:
> > > > > > > > > > > >
> > > > > > > > > > > > [PATCH v1] rng: add dm_rng_read_default() helper
> > > > > > > > > > > > https://lore.kernel.org/u-boot/4e28a388-f5b1-4cf7-b0e3-b12a876d0567@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
> > > > > > > > > > > >
> > > > > > > > > > > > We have weak function platform_get_rng_device() which
> > > > > > > > > > > > should be moved to drivers/rng/rng-uclass.c.
> > > > > > > > > > > >
> > > > > > > > > > > > We could add a function to drivers/core/uclass.c to
> > > > > > > > > > > > retrieve the first successfully probed device. Another
> > > > > > > > > > > > approach would be to implement
> > > > > > > > > > > > uclass_driver.post_probe() in the RNG uclass to take
> > > > > > > > > > > > note of the first successfully probed device.
> > > > > > > > > > > >
> > > > > > > > > > > > @Simon:
> > > > > > > > > > > > What would make most sense from a DM design
> > > > > > > > > > > > standpoint?
> > > > > > > > > > >
> > > > > > > > > > > I am sure I provided feedback on this at the time, but I
> > > > > > > > > > > don't remember. OK I just found it here [1]. So the
> > > > > > > > > > > problem is entirely because my feedback was not
> > > > > > > > > > > addressed. Please just address it and avoid this sort of
> > > > > > > > > > > mess.
> > > > > > > > > >
> > > > > > > > > > Yeah, Tom just merged it, but that's not Heinrich's fault
> > > > > > > > > > ;-)
> > > > > > > > > > > So arm_rndr should have a devicetree compatible string
> > > > > > > > > > > and be bound like anything else. If for some reason the
> > > > > > > > > > > device doesn't exist in the hardware, it can return
> > > > > > > > > > > -ENODEV from its bind() method.
> > > > > > > > > > >
> > > > > > > > > > > If you want to control which RNG device is used for
> > > > > > > > > > > booting, you could add a property to /bootstd with a
> > > > > > > > > > > phandle to the device. We are trying to provide a
> > > > > > > > > > > standard approach to booting in U-Boot, used by all
> > > > > > > > > > > methods. Doing one-off things for particular cases is
> > > > > > > > > > > best avoided.
> > > > > > > > > >
> > > > > > > > > > Picking the first usable device doesn't sound much like a
> > > > > > > > > > one-off to me. After all the caller (be it UEFI or the rng
> > > > > > > > > > command) later detect that this is not usable. So there
> > > > > > > > > > might be some merit to cover this more automatically,
> > > > > > > > > > either in the caller, or by providing a suitable wrapper
> > > > > > > > > > function?
> > > > > > > > >
> > > > > > > > > Or just follow the existing mechanisms which have been in
> > > > > > > > > U-Boot for years. Please...!
> > > > > > > > >
> > > > > > > > > [..]
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Simon
> > > > > > > > >  >
> > > > > > > > > > > [1]
> > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230830113230.3925868-1-andre.przywara@arm.com/
> > > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > > > >
> > > > > Regards,
> > > > > SImon
> > > >
> >