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

[Simon Glass]

Hi Heinrich,

On Sat, 4 Nov 2023 at 20:36, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 11/4/23 21:45, Simon Glass wrote:
> > Hi Andre,
> >
> > 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.
>
> The Zkr extension isn't a device. It is an instruction set architecture
> extension. It is represented by the ISA extension strings of the
> respective harts in the device-tree.
>
> If the seed register of the hart can be accessed by U-Boot, is
> determined by bit sseed in the mseccfg register.
>
> I don't expect the encoding of RISC-V ISA extensions in device-trees to
> be changed.

Please just give up with the 'not a device' thing. Let's just say that
a device is a software concept and how we model hardware.

This 'not a device' thing is creating all sorts of problems, to no end.

Nothing is changed until someone sends a patch.

Regards,
Simon



>
> Best regards
>
> Heinrich
>
> >
> >>
> >>>>> 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/
> >>
>