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

[Heinrich Schuchardt]

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.

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