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

[Andre Przywara]

On Mon, 6 Nov 2023 13:38:39 -0700
Simon Glass <sjg at chromium.org> wrote:

Hi Simon,

> On Mon, 6 Nov 2023 at 10:26, Andre Przywara <andre.przywara at arm.com> wrote:
> >
> > On Sat, 4 Nov 2023 19:45:06 +0000
> > Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi,
> >  
> > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara <andre.przywara at arm.com> wrote:  
> > > >
> > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Simon,
> > > >  
> > > > > Hi Heinrich,
> > > > >
> > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > <heinrich.schuchardt at canonical.com> wrote:  
> > > > > >
> > > > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > Heinrich Schuchardt <heinrich.schuchardt at canonical.com> wrote:
> > > > > > >
> > > > > > > Hi Heinrich,
> > > > > > >  
> > > > > > >> The Zkr ISA extension (ratified Nov 2021) introduced the seed CSR. It
> > > > > > >> provides an interface to a physical entropy source.
> > > > > > >>
> > > > > > >> A RNG driver based on the seed CSR is provided. It depends on
> > > > > > >> mseccfg.sseed being set in the SBI firmware.  
> > > > > > >
> > > > > > > As you might have seen, I added a similar driver for the respective Arm
> > > > > > > functionality:
> > > > > > > https://lore.kernel.org/u-boot/20230830113230.3925868-1-andre.przywara@arm.com/
> > > > > > >
> > > > > > > And I see that you seem to use the same mechanism to probe and init the
> > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > > implemented.
> > > > > > > One downside of this approach is that the driver is always loaded (and
> > > > > > > visible in the DM tree), even with the feature not being available.
> > > > > > > That doesn't seem too much of a problem on the first glance, but it
> > > > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > VExpress64# rng 0
> > > > > > > No RNG device  
> > > > >
> > > > > Why do we get this? If the device is not there, the bind() function
> > > > > can return -ENODEV
> > > > >
> > > > > I see this in U-Boot:
> > > > >
> > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > >
> > > > > We should not use this.  
> > > >
> > > > Agreed.
> > > >  
> > > > > Use the devicetree.  
> > > >
> > > > No, this is definitely not something for the DT, at least not on ARM.
> > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > Similar to PCI and USB devices, which we don't probe via the DT as well.
> > > >
> > > > It's arguably not proper "driver" material per se, as I've argued before, but
> > > > it's the simplest solution and fits in nicely otherwise.
> > > >
> > > > I was wondering if it might be something for UCLASS_CPU, something like
> > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > features (instead of compatible strings), then only bind() those
> > > > drivers it the respective bit is set.
> > > >
> > > > Does that make sense? Would that be doable without boiling the ocean?
> > > > As I don't know if we see many users apart from this.  
> > >
> > > I have seen this so many times, where people want to avoid putting
> > > things in the DT and then are surprised that everything is difficult,
> > > broken and confusing. Why not just follow the rules? It is not just
> > > about whether we can avoid it, etc. It is about how devices fit
> > > together cohesively in the system, and how U-Boot operates.  
> >
> > A devicetree is only for peripherals *that cannot be located by probing*.  
> 
> I have to stop you there. It absolutely is not limited to that.

I am very sorry, but I - (and seemingly everyone else in the kernel DT
community?) - seem to disagree here.

> > Which are traditionally most peripherals in non-server Arm SoCs. While I
> > do love the DT, the best DT node is the one you don't need.  
> 
> We need it in U-Boot, at least.
> 
> I'll send a patch with a warning on U_BOOT_DRVINFO() as it seems that
> some people did not see the header-file comment.

Fair enough.

> Let's just stop this discussion and instead talk about the binding we need.

Alright, if that is your decision, I will send a patch to revert
that "driver". There will never be a binding for a CPU instruction
discoverable by the architected CPU ID register.
I had some gripes with that "driver" in the first place, but it was so
temptingly simple and fit in so nicely, for instance into the UEFI
entropy service without even touching that code, that I couldn't resist
to just try it. And it actually solved a nasty problem for us, where
the kernel boot was stuck for minutes waiting for enough entropy to ...
let a script create a random filename ;-)
But we also have virtio-rng, so are not limited to the instructions.

But well, I guess I will just bite the bullet and go along the proper
route and create some RNG instruction abstraction, as sketched in that
other email.

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