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

[Sean Anderson <seanga2@gmail.com>]

On 11/1/23 13:49, Andre Przywara wrote:
> On Wed, 1 Nov 2023 13:16:24 -0400
> Sean Anderson <seanga2@gmail.com> wrote:
> 
> Hi Sean,
> 
>> On 11/1/23 13:05, Andre Przywara wrote:
>>> On Tue, 31 Oct 2023 14:55:50 +0200
>>> Heinrich Schuchardt <heinrich.schuchardt@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
>>> VExpress64# rng 1
>>> 00000000: f3 88 b6 d4 24 da 49 ca 49 f7 9e 66 5f 12 07 b2  ....$.I.I..f_...
>>> ....
>>>
>>> Now the EFI code always picks RNG device 0, which means we don't get
>>> entropy in this case.
>>>
>>> Do you have any idea how to solve this?
>>> Maybe EFI tries to probe further - but that sounds arbitrary.
>>> Or we find another way for probing the device, maybe via some artificial
>>> CPU feature "bus"? There is UCLASS_CPU, but that doesn't look helpful?
>>>
>>> If anyone has any idea, I'd be grateful.
>>
>> Wouldn't the right way be to detect the hardware in bind()?
> 
> Yes, that's what I thought as well and tried, but the problem is that for
> those "fixed drivers" (the ones using U_BOOT_DRVINFO) returning a
> failure in bind() is fatal to the boot sequence:
> ============
> Model: FVP Base
> DRAM:  2 GiB (effective 4 GiB)
> No match for driver 'arm-rndr'
> initcall failed at call 00000000fef3d744 (err=-19)
> ### ERROR ### Please RESET the board ###
> 
> That is what a proper "CPU bus" would probably solve, as I agree that
> failing bind() should be the proper solution.

Hm, so maybe this should go in riscv_cpu_bind?

--Sean

> Cheers,
> Andre
> 
>> --Sean
>>
>>> Cheers,
>>> Andre
>>>    
>>>> If the seed CSR readable, is not determinable by S-mode without risking
>>>> an exception. For safe driver probing allow to resume via a longjmp
>>>> after an exception.
>>>>
>>>> As the driver depends on mseccfg.sseed=1 we should wait with merging the
>>>> driver until a decision has been taken in the RISC-V PRS TG on prescribing
>>>> this.
>>>>
>>>> Setting mseccfg.sseed=1 is queued for OpenSBI [1]. This has been discussed
>>>> in the RISC-V Boot & Runtime Services TG. Standardization has to be pursued
>>>> via the upcoming platform specification.
>>>>
>>>> A bug fix for QEMU relating to the Zkr extension is available in [2].
>>>>
>>>> A similar Linux driver has been proposed in [3].
>>>>
>>>> [1] lib: sbi: Configure seed bits when MSECCFG is readable
>>>>       https://patchwork.ozlabs.org/project/opensbi/patch/20230712083254.1585244-1-sameo@rivosinc.com/
>>>> [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
>>>>       https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchardt@canonical.com/
>>>> [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
>>>>       https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc.com/
>>>>
>>>> v3:
>>>> 	Add API documentation.
>>>> v2:
>>>> 	Catch exception if mseccfg.sseed=0.
>>>>
>>>> Heinrich Schuchardt (2):
>>>>     riscv: allow resume after exception
>>>>     rng: Provide a RNG based on the RISC-V Zkr ISA extension
>>>>
>>>>    arch/riscv/lib/interrupts.c |  13 ++++
>>>>    doc/api/index.rst           |   1 +
>>>>    drivers/rng/Kconfig         |   8 +++
>>>>    drivers/rng/Makefile        |   1 +
>>>>    drivers/rng/riscv_zkr_rng.c | 116 ++++++++++++++++++++++++++++++++++++
>>>>    include/interrupt.h         |  45 ++++++++++++++
>>>>    6 files changed, 184 insertions(+)
>>>>    create mode 100644 drivers/rng/riscv_zkr_rng.c
>>>>    create mode 100644 include/interrupt.h
>>>>   
>>>    
>>
>