[PATCH v3 0/2] rng: Provide a RNG based on the RISC-V Zkr ISA extension
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.
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
If CSRs like seed are readable by S-mode, may not be determinable by S-mode. For safe driver probing allow to resume via a longjmp after an exception.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v3: Add API documentation v2: New patch --- arch/riscv/lib/interrupts.c | 13 +++++++++++ doc/api/index.rst | 1 + include/interrupt.h | 45 +++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 include/interrupt.h
diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c index 02dbcfd423..a26ccc721f 100644 --- a/arch/riscv/lib/interrupts.c +++ b/arch/riscv/lib/interrupts.c @@ -12,6 +12,7 @@ #include <linux/compat.h> #include <efi_loader.h> #include <hang.h> +#include <interrupt.h> #include <irq_func.h> #include <asm/global_data.h> #include <asm/ptrace.h> @@ -21,6 +22,13 @@
DECLARE_GLOBAL_DATA_PTR;
+static struct resume_data *resume; + +void set_resume(struct resume_data *data) +{ + resume = data; +} + static void show_efi_loaded_images(uintptr_t epc) { efi_print_image_infos((void *)epc); @@ -105,6 +113,11 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs) "Store/AMO page fault", };
+ if (resume) { + resume->code = code; + longjmp(resume->jump, 1); + } + if (code < ARRAY_SIZE(exception_code)) printf("Unhandled exception: %s\n", exception_code[code]); else diff --git a/doc/api/index.rst b/doc/api/index.rst index 2f0218c47a..51b2013af3 100644 --- a/doc/api/index.rst +++ b/doc/api/index.rst @@ -12,6 +12,7 @@ U-Boot API documentation efi event getopt + interrupt linker_lists lmb logging diff --git a/include/interrupt.h b/include/interrupt.h new file mode 100644 index 0000000000..46ef2e196d --- /dev/null +++ b/include/interrupt.h @@ -0,0 +1,45 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <asm/setjmp.h> + +/** + * struct resume_data - data for resume after interrupt + */ +struct resume_data { + /** @jump: longjmp buffer */ + jmp_buf jump; + /** @code: exception code */ + ulong code; +}; + +/** + * set_resume() - set longjmp buffer for resuming after exception + * + * By calling this function it is possible to use a long jump to catch an + * exception. The caller sets the long jump buffer with set_resume() and then + * executes setjmp(). If an exception occurs, the code will return to the + * setjmp caller(). The exception code will be returned in @data->code. + * + * After the critical operation call set_resume(NULL) so that an exception in + * another part of the code will not accidently invoke the long jump. + * + * .. code-block:: c + * + * // This example shows how to use set_resume(). + * + * struct resume_data resume; + * int ret; + * + * set_resume(&resume); + * ret = setjmp(resume.jump); + * if (ret) { + * printf("An exception %ld occurred\n", resume.code); + * } else { + * // Do what might raise an exception here. + * } + * set_resume(NULL); + * + * @data: pointer to structure with longjmp address + * Return: 0 before an exception, 1 after an exception occurred + */ +void set_resume(struct resume_data *data);
On Tue, Oct 31, 2023 at 02:55:51PM +0200, Heinrich Schuchardt wrote:
If CSRs like seed are readable by S-mode, may not be determinable by S-mode. For safe driver probing allow to resume via a longjmp after an exception.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v3: Add API documentation v2: New patch
arch/riscv/lib/interrupts.c | 13 +++++++++++ doc/api/index.rst | 1 + include/interrupt.h | 45 +++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 include/interrupt.h
Reviewed-by: Leo Yu-Chi Liang ycliang@andestech.com
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.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Leo Yu-Chi Liang ycliang@andestech.com --- v3: no change v2: catch exceptions --- drivers/rng/Kconfig | 8 +++ drivers/rng/Makefile | 1 + drivers/rng/riscv_zkr_rng.c | 116 ++++++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+) create mode 100644 drivers/rng/riscv_zkr_rng.c
diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig index 994cc35b27..4f6e367169 100644 --- a/drivers/rng/Kconfig +++ b/drivers/rng/Kconfig @@ -48,6 +48,14 @@ config RNG_OPTEE accessible to normal world but reserved and used by the OP-TEE to avoid the weakness of a software PRNG.
+config RNG_RISCV_ZKR + bool "RISC-V Zkr random number generator" + depends on RISCV_SMODE + help + This driver provides a Random Number Generator based on the + Zkr RISC-V ISA extension which provides an interface to an + NIST SP 800-90B or BSI AIS-31 compliant physical entropy source. + config RNG_STM32 bool "Enable random number generator for STM32" depends on ARCH_STM32 || ARCH_STM32MP diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile index 47b323e61e..a5d3ca4130 100644 --- a/drivers/rng/Makefile +++ b/drivers/rng/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o obj-$(CONFIG_RNG_NPCM) += npcm_rng.o obj-$(CONFIG_RNG_OPTEE) += optee_rng.o obj-$(CONFIG_RNG_STM32) += stm32_rng.o +obj-$(CONFIG_RNG_RISCV_ZKR) += riscv_zkr_rng.o obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o obj-$(CONFIG_RNG_SMCCC_TRNG) += smccc_trng.o diff --git a/drivers/rng/riscv_zkr_rng.c b/drivers/rng/riscv_zkr_rng.c new file mode 100644 index 0000000000..8c9e111e2e --- /dev/null +++ b/drivers/rng/riscv_zkr_rng.c @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * The RISC-V Zkr extension provides CSR seed which provides access to a + * random number generator. + */ + +#define LOG_CATEGORY UCLASS_RNG + +#include <dm.h> +#include <interrupt.h> +#include <log.h> +#include <rng.h> + +#define DRIVER_NAME "riscv_zkr" + +enum opst { + /** @BIST: built in self test running */ + BIST = 0b00, + /** @WAIT: sufficient amount of entropy is not yet available */ + WAIT = 0b01, + /** @ES16: 16bits of entropy available */ + ES16 = 0b10, + /** @DEAD: unrecoverable self-test error */ + DEAD = 0b11, +}; + +static unsigned long read_seed(void) +{ + unsigned long ret; + + __asm__ __volatile__("csrrw %0, seed, x0" : "=r" (ret) : : "memory"); + + return ret; +} + +static int riscv_zkr_read(struct udevice *dev, void *data, size_t len) +{ + u8 *ptr = data; + + while (len) { + u32 val; + + val = read_seed(); + + switch (val >> 30) { + case BIST: + continue; + case WAIT: + continue; + case ES16: + *ptr++ = val & 0xff; + if (--len) { + *ptr++ = val >> 8; + --len; + } + break; + case DEAD: + return -ENODEV; + } + } + + return 0; +} + +/** + * riscv_zkr_probe() - check if the seed register is available + * + * If the SBI software has not set mseccfg.sseed=1 or the Zkr + * extension is not available this probe function will result + * in an exception. Currently we cannot recover from this. + * + * @dev: RNG device + * Return: 0 if successfully probed + */ +static int riscv_zkr_probe(struct udevice *dev) +{ + struct resume_data resume; + int ret; + u32 val; + + /* Check if reading seed leads to interrupt */ + set_resume(&resume); + ret = setjmp(resume.jump); + if (ret) + log_debug("Exception %ld reading seed CSR\n", resume.code); + else + val = read_seed(); + set_resume(NULL); + if (ret) + return -ENODEV; + + do { + val = read_seed(); + val >>= 30; + } while (val == BIST || val == WAIT); + + if (val == DEAD) + return -ENODEV; + + return 0; +} + +static const struct dm_rng_ops riscv_zkr_ops = { + .read = riscv_zkr_read, +}; + +U_BOOT_DRIVER(riscv_zkr) = { + .name = DRIVER_NAME, + .id = UCLASS_RNG, + .ops = &riscv_zkr_ops, + .probe = riscv_zkr_probe, +}; + +U_BOOT_DRVINFO(cpu_riscv_zkr) = { + .name = DRIVER_NAME, +};
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.c...
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.
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
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.c...
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()?
--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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
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.c...
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.
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
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.c...
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
On 11/1/23 19: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.c...
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_...
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...
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?
Best regards
Heinrich
....
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.
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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. Use the devicetree.
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...
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.
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.
Best regards
Heinrich
....
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.
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20230830113230.3925868-1-an...
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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.
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...
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?
Cheers, Andre
Best regards
Heinrich
....
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.
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-... [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG] https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schuchard... [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sameo@rivosinc....
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
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20230830113230.3925868-1-an...
Hi Andre,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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.
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...
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-an...
On 11/4/23 21:45, Simon Glass wrote:
Hi Andre,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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...
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-an...
Hi Heinrich,
On Sat, 4 Nov 2023 at 20:36, Heinrich Schuchardt heinrich.schuchardt@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@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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...
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-an...
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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*. 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.
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...
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-an...
On Mon, Nov 06, 2023 at 05:26:01PM +0000, Andre Przywara wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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*. 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.
In general, yes, this. And we keep banging against this too. If we can figure it out at run time, without needing device tree, we should be doing that, not adding a device tree node/property.
A device tree check is not our only run-time "does this exist" check.
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.
In specifics, yes, some sort of split like this sounds good and we may or may not also want to have some option for talking with op-tee in the case where that, rather than instructions are how we do it. And a similar approach for timers with a possible need for something slightly more complex for bootstage usage?
Hi Andre,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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.
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.
Let's just stop this discussion and instead talk about the binding we need.
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...
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-an...
Regards, Simon
On Mon, Nov 06, 2023 at 01:38:39PM -0700, Simon Glass wrote:
Hi Andre,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c... > > 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.
It is limited to that if we're going to keep using the device trees that Linux uses. Full stop. There's not really wiggle room there either.
Hi Tom,
On Mon, 6 Nov 2023 at 13:46, Tom Rini trini@konsulko.com wrote:
On Mon, Nov 06, 2023 at 01:38:39PM -0700, Simon Glass wrote:
Hi Andre,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c... > > > > 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.
It is limited to that if we're going to keep using the device trees that Linux uses. Full stop. There's not really wiggle room there either.
That is really the problem, I agree.
But I would be happy with a u-boot.dtsi file to resolve this, while we wait. I believe a binding makes sense in this case.
Regards, Simon
On Tue, Nov 07, 2023 at 01:10:44AM +0000, Simon Glass wrote:
Hi Tom,
On Mon, 6 Nov 2023 at 13:46, Tom Rini trini@konsulko.com wrote:
On Mon, Nov 06, 2023 at 01:38:39PM -0700, Simon Glass wrote:
Hi Andre,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
> Hi Heinrich, > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > heinrich.schuchardt@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@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.c... > > > > > > 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.
It is limited to that if we're going to keep using the device trees that Linux uses. Full stop. There's not really wiggle room there either.
That is really the problem, I agree.
And we need to accept that, and what is/isn't something that we can expect every board developer to have to tweak on top of this.
Heck, maybe part of the issue here is that devicetree-the-spec and devicetree-the-linux-kernel-input need a little differentiation and some official statement along the lines of "just because X can be in the device tree does not mean that X will be defined in the device tree, if it can be detected in some other reliable manner" for the latter.
But I would be happy with a u-boot.dtsi file to resolve this, while we wait. I believe a binding makes sense in this case.
We don't need a binding, we can easily check at run-time. We only barely have to worry about run-time failing (yes, QEMU could be fired up with a model that lacks it, or some future change, and it's cheap to check). I would say we could use the cpu compatible as a binding, but I don't want to then have to add a bootph property to that. And the RISC-V example makes it even clearer that this is not a binding thing.
I do not want a binding here that we just don't upstream because it will make life harder for everyone else that's adding new platforms to get the feature to work. Or people won't get it to work and instead add new code that they most likely didn't need to, per drivers/timer/ today.
I'll let security people argue on what level of RNG (and so perhaps RNG choice on systems that have more than one source available) devices need to be present. But I think drivers/timer/ is the better example of needing to just have the source present, with minimal run-time checking (since it's a feature of the CPU).
On Tue, Nov 7, 2023 at 1:30 PM Tom Rini trini@konsulko.com wrote:
On Tue, Nov 07, 2023 at 01:10:44AM +0000, Simon Glass wrote:
Hi Tom,
On Mon, 6 Nov 2023 at 13:46, Tom Rini trini@konsulko.com wrote:
On Mon, Nov 06, 2023 at 01:38:39PM -0700, Simon Glass wrote:
Hi Andre,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote: > > On Fri, 3 Nov 2023 13:38:58 -0600 > Simon Glass sjg@chromium.org wrote: > > Hi Simon, > > > Hi Heinrich, > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > heinrich.schuchardt@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@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.c... > > > > > > > > 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.
It is limited to that if we're going to keep using the device trees that Linux uses. Full stop. There's not really wiggle room there either.
That is really the problem, I agree.
And we need to accept that, and what is/isn't something that we can expect every board developer to have to tweak on top of this.
Heck, maybe part of the issue here is that devicetree-the-spec and devicetree-the-linux-kernel-input need a little differentiation and some official statement along the lines of "just because X can be in the device tree does not mean that X will be defined in the device tree, if it can be detected in some other reliable manner" for the latter.
I think Andre's statement is just missing 1 word: required
A devicetree is only *required* for peripherals *that cannot be located by probing*.
But really I'd phrase it in terms of what's needed for discoverable devices.
I'm somewhat surprised at this point in time we need a statement, but happy to add something to the DT spec. DT has been optional for PCI/USB since the advent of FDT and only used there when there's extra resources which are not discoverable. It only seems to be a question when it's a not $sig bus.
But I would be happy with a u-boot.dtsi file to resolve this, while we wait. I believe a binding makes sense in this case.
We don't need a binding, we can easily check at run-time. We only barely have to worry about run-time failing (yes, QEMU could be fired up with a model that lacks it, or some future change, and it's cheap to check). I would say we could use the cpu compatible as a binding, but I don't want to then have to add a bootph property to that. And the RISC-V example makes it even clearer that this is not a binding thing.
I do not want a binding here that we just don't upstream because it will make life harder for everyone else that's adding new platforms to get the feature to work. Or people won't get it to work and instead add new code that they most likely didn't need to, per drivers/timer/ today.
I'll let security people argue on what level of RNG (and so perhaps RNG choice on systems that have more than one source available) devices need to be present.
There's the stance that you don't trust any of them, so use them all and mix them together.
But I think drivers/timer/ is the better example of needing to just have the source present, with minimal run-time checking (since it's a feature of the CPU).
Timers come up frequently (well, less so with Arm arch timer now) with various ways to assign timers to Linux clocksource and clockevent. My response there is what's the difference in the instances that you care about assignment? If they are all the same, then you shouldn't. If they aren't the same, describe the difference. Sometimes it's just one instance has an interrupt, so it's the clockevent. Or you need the one that's always on. The OS/client can figure that out if you describe those properties. That's better than creating arbitrary indices.
Rob
On Tue, Nov 07, 2023 at 03:52:36PM -0600, Rob Herring wrote:
On Tue, Nov 7, 2023 at 1:30 PM Tom Rini trini@konsulko.com wrote:
On Tue, Nov 07, 2023 at 01:10:44AM +0000, Simon Glass wrote:
Hi Tom,
On Mon, 6 Nov 2023 at 13:46, Tom Rini trini@konsulko.com wrote:
On Mon, Nov 06, 2023 at 01:38:39PM -0700, Simon Glass wrote:
Hi Andre,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
> On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote: > > > > On Fri, 3 Nov 2023 13:38:58 -0600 > > Simon Glass sjg@chromium.org wrote: > > > > Hi Simon, > > > > > Hi Heinrich, > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > > heinrich.schuchardt@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@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.c... > > > > > > > > > > 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.
It is limited to that if we're going to keep using the device trees that Linux uses. Full stop. There's not really wiggle room there either.
That is really the problem, I agree.
And we need to accept that, and what is/isn't something that we can expect every board developer to have to tweak on top of this.
Heck, maybe part of the issue here is that devicetree-the-spec and devicetree-the-linux-kernel-input need a little differentiation and some official statement along the lines of "just because X can be in the device tree does not mean that X will be defined in the device tree, if it can be detected in some other reliable manner" for the latter.
I think Andre's statement is just missing 1 word: required
A devicetree is only *required* for peripherals *that cannot be located by probing*.
But really I'd phrase it in terms of what's needed for discoverable devices.
I'm somewhat surprised at this point in time we need a statement, but happy to add something to the DT spec. DT has been optional for PCI/USB since the advent of FDT and only used there when there's extra resources which are not discoverable. It only seems to be a question when it's a not $sig bus.
I think the need for a statement about this will help with less long time and/or Linux kernel centric communities. So really do think putting something in the spec will be helpful, and maybe further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
[snip]
But I think drivers/timer/ is the better example of needing to just have the source present, with minimal run-time checking (since it's a feature of the CPU).
Timers come up frequently (well, less so with Arm arch timer now) with various ways to assign timers to Linux clocksource and clockevent. My response there is what's the difference in the instances that you care about assignment? If they are all the same, then you shouldn't. If they aren't the same, describe the difference. Sometimes it's just one instance has an interrupt, so it's the clockevent. Or you need the one that's always on. The OS/client can figure that out if you describe those properties. That's better than creating arbitrary indices.
In details, this is about the U-Boot API for "how long has elapsed in the system" and not clocks for peripherals, which gets confusing in that I _think_ in Linux it's all drivers/clocksource/ but in U-Boot we have drivers/clk for peripherals and too-many-things for "elapsed time" and so for example the ARMv8 generic timer isn't in the DM framework but nor do most platforms using it trigger the "no DM migration!" warning because the "driver" is so simple.
I introduced the "timer" part to the "rng" thread because when I saw this going on Friday it reminded me that I'd seen another case where "CPU lets us do X" and "this is not happy in current DM" was true, sorry for the confusion.
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions and so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
Hi,
On Tue, 7 Nov 2023 at 15:38, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions and so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
Some more details (sorry) from your friendly devicetree and driver model maintainer:
- U-Boot models hardware (and other things) as devices in driver model [1] - U-Boot requires devices to be in the devicetree, with very limited exceptions [2] - Where multiple devices exist in a uclass, it is desirable to be able to number them [3] - Similarly it is useful to be able select a particular device, e.g. with a phandle [4] - U-Boot uses devicetree for configuration as it has no userspace
Regards, Simon
[1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/index.html [2] https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#pla... [3] https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#dev... [4] https://u-boot.readthedocs.io/en/latest/api/dm.html?highlight=phandle#c.of_f...
On Tue, Nov 07, 2023 at 03:51:21PM -0700, Simon Glass wrote:
Hi,
On Tue, 7 Nov 2023 at 15:38, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions and so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
Some more details (sorry) from your friendly devicetree and driver model maintainer:
- U-Boot models hardware (and other things) as devices in driver model [1]
- U-Boot requires devices to be in the devicetree, with very limited
exceptions [2]
- Where multiple devices exist in a uclass, it is desirable to be able
to number them [3]
- Similarly it is useful to be able select a particular device, e.g.
with a phandle [4]
- U-Boot uses devicetree for configuration as it has no userspace
And I'm very specifically trying to NOT dive in to these particular details and just find out how the accepted bindings for RISC-V are intended to solve the question U-Boot has and in turn figure out what U-Boot should do here. I'm not asking Conor how it should all fit in to what U-Boot does, just how what's defined now can be used in this case.
+CC Palmer
On Tue, Nov 07, 2023 at 05:38:37PM -0500, Tom Rini wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions
For this one, it's pretty straightforward IMO - if riscv,isa-extensions contains "zbb", then you are safe to use those instructions. My understanding is that relying on getting illegal instruction traps is not a sufficient test for usability of standard extensions, as a vendor extension could be using the same opcodes as a standard extension.
so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
The ordinary extensions that are instructions - like Zbkb that provides bit manipulation instructions for cryptography you will be able to rely on riscv,isa-extensions also. Zkr is actually a CSR acting as an entropy source and is a bit more complicated. RISC-V Cryptography Extensions Volume I, Chapter Four [0] is the relevant thing for use of the CSR provided by Zkr, and it says "The seed CSR is also access controlled by execution mode, and attempted read or write access will raise an illegal instruction exception outside M mode unless access is explicitly granted." My take is that either the SBI implementation needs to provide S-Mode U-Boot with an accurate devicetree (including what extensions are valid for use in S-mode) or if the devicetree is provided as part of the U-Boot binary then it needs to match what is available at that privilege level on the platform. In this case, you would also be able to rely on riscv,isa-extensions for that detection. There is an existing dt-binding patch https://lore.kernel.org/linux-riscv/20231107105556.517187-6-cleger@rivosinc.com/ that adds Zkr, and my proposal would be to document that the presence of Zkr explicitly in riscv,isa-extensions means that the bit in mseccfg.[s,u]seed has been set so it can be used at the current privilege level.
If that's not acceptable, and people think that having Zkr in the devicetree means that the hardware has the extension, regardless of usability at the present privilege level, then IMO we need an SBI ecall defined to request entablement of the CSR & report as to whether or not that was possible.
I'm not sure how any of the above lines up with the ARMv8.5 RNG feature unfortunately.
Cheers, Conor.
0 - https://github.com/riscv/riscv-crypto/releases/tag/v1.0.1-scalar
On Tue, Nov 07, 2023 at 11:12:16PM +0000, Conor Dooley wrote:
+CC Palmer
On Tue, Nov 07, 2023 at 05:38:37PM -0500, Tom Rini wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions
For this one, it's pretty straightforward IMO - if riscv,isa-extensions contains "zbb", then you are safe to use those instructions. My understanding is that relying on getting illegal instruction traps is not a sufficient test for usability of standard extensions, as a vendor extension could be using the same opcodes as a standard extension.
so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
The ordinary extensions that are instructions - like Zbkb that provides bit manipulation instructions for cryptography you will be able to rely on riscv,isa-extensions also. Zkr is actually a CSR acting as an entropy source and is a bit more complicated. RISC-V Cryptography Extensions Volume I, Chapter Four [0] is the relevant thing for use of the CSR provided by Zkr, and it says "The seed CSR is also access controlled by execution mode, and attempted read or write access will raise an illegal instruction exception outside M mode unless access is explicitly granted." My take is that either the SBI implementation needs to provide S-Mode U-Boot with an accurate devicetree (including what extensions are valid for use in S-mode) or if the devicetree is provided as part of the U-Boot binary then it needs to match what is available at that privilege level on the platform. In this case, you would also be able to rely on riscv,isa-extensions for that detection. There is an existing dt-binding patch https://lore.kernel.org/linux-riscv/20231107105556.517187-6-cleger@rivosinc.com/ that adds Zkr, and my proposal would be to document that the presence of Zkr explicitly in riscv,isa-extensions means that the bit in mseccfg.[s,u]seed has been set so it can be used at the current privilege level.
If that's not acceptable, and people think that having Zkr in the devicetree means that the hardware has the extension, regardless of usability at the present privilege level, then IMO we need an SBI ecall defined to request entablement of the CSR & report as to whether or not that was possible.
I'm not sure how any of the above lines up with the ARMv8.5 RNG feature unfortunately.
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
On Tue, Nov 07, 2023 at 11:12:16PM +0000, Conor Dooley wrote:
+CC Palmer
On Tue, Nov 07, 2023 at 05:38:37PM -0500, Tom Rini wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions
For this one, it's pretty straightforward IMO - if riscv,isa-extensions contains "zbb", then you are safe to use those instructions. My understanding is that relying on getting illegal instruction traps is not a sufficient test for usability of standard extensions, as a vendor extension could be using the same opcodes as a standard extension.
so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
The ordinary extensions that are instructions - like Zbkb that provides bit manipulation instructions for cryptography you will be able to rely on riscv,isa-extensions also. Zkr is actually a CSR acting as an entropy source and is a bit more complicated. RISC-V Cryptography Extensions Volume I, Chapter Four [0] is the relevant thing for use of the CSR provided by Zkr, and it says "The seed CSR is also access controlled by execution mode, and attempted read or write access will raise an illegal instruction exception outside M mode unless access is explicitly granted." My take is that either the SBI implementation needs to provide S-Mode U-Boot with an accurate devicetree (including what extensions are valid for use in S-mode) or if the devicetree is provided as part of the U-Boot binary then it needs to match what is available at that privilege level on the platform. In this case, you would also be able to rely on riscv,isa-extensions for that detection. There is an existing dt-binding patch https://lore.kernel.org/linux-riscv/20231107105556.517187-6-cleger@rivosinc.com/ that adds Zkr, and my proposal would be to document that the presence of Zkr explicitly in riscv,isa-extensions means that the bit in mseccfg.[s,u]seed has been set so it can be used at the current privilege level.
If that's not acceptable, and people think that having Zkr in the devicetree means that the hardware has the extension, regardless of usability at the present privilege level, then IMO we need an SBI ecall defined to request entablement of the CSR & report as to whether or not that was possible.
I'm not sure how any of the above lines up with the ARMv8.5 RNG feature unfortunately.
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node? I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility? https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L767
| - U-Boot requires devices to be in the devicetree, with very limited | exceptions [2]
| - Where multiple devices exist in a uclass, it is desirable to be able | to number them [3]
I'm not sure really how this one ties in. Do you need a number for each CPU that supports Zkr, since a system may be heterogeneous? I think that how you treat things like that is beyond communicating support via DT though, IMO the job of the DT is just to tell U-Boot on which CPUs it can access the seed CSR.
| - Similarly it is useful to be able select a particular device, e.g. | with a phandle [4]
I suppose a phandle to the CPU would work in this case.
| - U-Boot uses devicetree for configuration as it has no userspace
Cheers, Conor.
On Wed, Nov 08, 2023 at 12:29:03AM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
[snip]
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node? I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility? https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L767
| - U-Boot requires devices to be in the devicetree, with very limited | exceptions [2]
| - Where multiple devices exist in a uclass, it is desirable to be able | to number them [3]
I'm not sure really how this one ties in. Do you need a number for each CPU that supports Zkr, since a system may be heterogeneous? I think that how you treat things like that is beyond communicating support via DT though, IMO the job of the DT is just to tell U-Boot on which CPUs it can access the seed CSR.
| - Similarly it is useful to be able select a particular device, e.g. | with a phandle [4]
I suppose a phandle to the CPU would work in this case.
| - U-Boot uses devicetree for configuration as it has no userspace
I mean, the reason I was setting aside Simon's question is that in my mind, we (U-Boot) need to think about what we're declaring as a MUST because the constant feedback that we get is "No, why does that need to get added to DT? Can't you just use ... ?". So I do find your answers above enlightening in that regard.
On 11/7/23 16:34, Tom Rini wrote:
On Wed, Nov 08, 2023 at 12:29:03AM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
[snip]
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node?
Devices in U-Boot are bound on the basis of a compatible string. All RISC-V CPU nodes have a compatible string 'riscv' but that does not provide any information about the existence of the Zkr extension. That information is in the 'riscv,isa-extensions' property of the cpu nodes (see Documentation/devicetree/bindings/riscv/cpus.yaml).
I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility?
This is what the U_BOOT_DRVINFO() macro in my driver does and which Simon discourages.
The discussion boils down to whether U-Boot can force every M-mode firmware invoking it to provide a compatible string for Zkr if the extension is provided (or to be more restrictive if mseccfg.sseed is set to 1).
I would prefer to avoid duplicate encoding of RISC-V extensions in the device-tree.
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L767
| - U-Boot requires devices to be in the devicetree, with very limited | exceptions [2]
| - Where multiple devices exist in a uclass, it is desirable to be able | to number them [3]
I'm not sure really how this one ties in. Do you need a number for each CPU that supports Zkr, since a system may be heterogeneous? I think that how you treat things like that is beyond communicating support via DT though, IMO the job of the DT is just to tell U-Boot on which CPUs it can access the seed CSR.
U-Boot only uses a single hart. It is for this specific hart that we need to find out if the seed register provided by the Zkr extension is readable. The register is only readable if additionally the mseccfg.sseed bit is set by machine mode firmware.
Thus any additional device-tree node related to the Zkr extension would have to be provided per Zkr enabled 'cpu' device-tree node.
Best regards
Heinrich
| - Similarly it is useful to be able select a particular device, e.g. | with a phandle [4]
I suppose a phandle to the CPU would work in this case.
| - U-Boot uses devicetree for configuration as it has no userspace
I mean, the reason I was setting aside Simon's question is that in my mind, we (U-Boot) need to think about what we're declaring as a MUST because the constant feedback that we get is "No, why does that need to get added to DT? Can't you just use ... ?". So I do find your answers above enlightening in that regard.
On Wed, Nov 08, 2023 at 06:23:37AM -0800, Heinrich Schuchardt wrote:
On 11/7/23 16:34, Tom Rini wrote:
On Wed, Nov 08, 2023 at 12:29:03AM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
[snip]
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node?
Devices in U-Boot are bound on the basis of a compatible string. All RISC-V CPU nodes have a compatible string 'riscv' but that does not provide any information about the existence of the Zkr extension. That information is in the 'riscv,isa-extensions' property of the cpu nodes (see Documentation/devicetree/bindings/riscv/cpus.yaml).
I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility?
This is what the U_BOOT_DRVINFO() macro in my driver does and which Simon discourages.
My current thoughts are that in this case we could use U_BOOT_DRVINFO() like today and then have riscv_zkr_probe() be what checks the riscv,isa-extensions property for an appropriate match? This would mean we don't need any new nodes/compatibles/etc, and possibly not need any bootph- properties added either? I assume we don't need the RNG so early as for that to be an issue.
On 11/8/23 06:37, Tom Rini wrote:
On Wed, Nov 08, 2023 at 06:23:37AM -0800, Heinrich Schuchardt wrote:
On 11/7/23 16:34, Tom Rini wrote:
On Wed, Nov 08, 2023 at 12:29:03AM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
[snip]
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node?
Devices in U-Boot are bound on the basis of a compatible string. All RISC-V CPU nodes have a compatible string 'riscv' but that does not provide any information about the existence of the Zkr extension. That information is in the 'riscv,isa-extensions' property of the cpu nodes (see Documentation/devicetree/bindings/riscv/cpus.yaml).
I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility?
This is what the U_BOOT_DRVINFO() macro in my driver does and which Simon discourages.
My current thoughts are that in this case we could use U_BOOT_DRVINFO() like today and then have riscv_zkr_probe() be what checks the riscv,isa-extensions property for an appropriate match? This would mean we don't need any new nodes/compatibles/etc, and possibly not need any bootph- properties added either? I assume we don't need the RNG so early as for that to be an issue.
The presence of the Zkr extension in the device-tree 'riscv,isa-extensions' property does not indicate if the machine mode firmware has enabled access in supervisor mode via the mseccfg.sseed flag. This is why my driver tries to read the seed register and checks if an exception occurs.
Additionally checking the device-tree would increase code size. Is it really needed?
We may have multiple RNG drivers enabled on the same device, e.g. TPM, Zkr, virtio-rng. In several code locations we try to use the first RNG device (not the first successfully probed RNG device). This is why
[PATCH 1/1] rng: detect RISC-V Zkr RNG device in bind method https://lore.kernel.org/u-boot/20231104065107.23623-1-heinrich.schuchardt@ca...
moves the detection from probe() to bind(). The patch further corrects the return code if the RNG device is not available.
Best regards
Heinrich
On Wed, Nov 08, 2023 at 07:25:22AM -0800, Heinrich Schuchardt wrote:
On 11/8/23 06:37, Tom Rini wrote:
On Wed, Nov 08, 2023 at 06:23:37AM -0800, Heinrich Schuchardt wrote:
On 11/7/23 16:34, Tom Rini wrote:
On Wed, Nov 08, 2023 at 12:29:03AM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
[snip]
Thanks. Setting aside Simon's follow-up, this is what I was looking for. We might have to wait for Heinrich to return from the conference to have time to look at how to utilize the above and see what we can do from there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node?
Devices in U-Boot are bound on the basis of a compatible string. All RISC-V CPU nodes have a compatible string 'riscv' but that does not provide any information about the existence of the Zkr extension. That information is in the 'riscv,isa-extensions' property of the cpu nodes (see Documentation/devicetree/bindings/riscv/cpus.yaml).
I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility?
This is what the U_BOOT_DRVINFO() macro in my driver does and which Simon discourages.
My current thoughts are that in this case we could use U_BOOT_DRVINFO() like today and then have riscv_zkr_probe() be what checks the riscv,isa-extensions property for an appropriate match? This would mean we don't need any new nodes/compatibles/etc, and possibly not need any bootph- properties added either? I assume we don't need the RNG so early as for that to be an issue.
The presence of the Zkr extension in the device-tree 'riscv,isa-extensions' property does not indicate if the machine mode firmware has enabled access in supervisor mode via the mseccfg.sseed flag. This is why my driver tries to read the seed register and checks if an exception occurs.
This I think is a question for Cody or someone else in the RISC-V community? If just checking the property isn't sufficient, what is? Or, what's the best / most reliable way? I don't know and I'll let the RISC-V community sort that out instead of making incorrect assumptions myself.
Additionally checking the device-tree would increase code size. Is it really needed?
I mean, it depends? My biggest issue right now is that in-tree I don't see any RISC-V targets that select any of the RISCV_ISA options so I can't evaluate what platforms grow by how much where. We shouldn't be talking about kilobytes of change, so no, I don't think somehow checking the device tree for this is out of bounds. But it comes back to what I just asked above, first. We need the correct and expected way to check for this feature to be known, then we decide how to handle that.
We may have multiple RNG drivers enabled on the same device, e.g. TPM, Zkr, virtio-rng. In several code locations we try to use the first RNG device (not the first successfully probed RNG device). This is why
[PATCH 1/1] rng: detect RISC-V Zkr RNG device in bind method https://lore.kernel.org/u-boot/20231104065107.23623-1-heinrich.schuchardt@ca...
moves the detection from probe() to bind(). The patch further corrects the return code if the RNG device is not available.
Sounds like we have some other general issues to sort out too then. Filing an issue on https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/ would be good so it doesn't get forgotten.
On 11/8/23 08:44, Tom Rini wrote:
On Wed, Nov 08, 2023 at 07:25:22AM -0800, Heinrich Schuchardt wrote:
On 11/8/23 06:37, Tom Rini wrote:
On Wed, Nov 08, 2023 at 06:23:37AM -0800, Heinrich Schuchardt wrote:
On 11/7/23 16:34, Tom Rini wrote:
On Wed, Nov 08, 2023 at 12:29:03AM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
[snip]
> Thanks. Setting aside Simon's follow-up, this is what I was looking for. > We might have to wait for Heinrich to return from the conference to have > time to look at how to utilize the above and see what we can do from > there.
I did read that, but I don't think most of it is relevant to the binding itself. His five things were: | - U-Boot models hardware (and other things) as devices in driver model [1]
This I think should be satisfied. The Zkr CSR is a property of the CPU, and shouldn't have its own DT node IMO. Is it problematic for U-Boot to populate multiple devices for its driver model based on one DT node?
Devices in U-Boot are bound on the basis of a compatible string. All RISC-V CPU nodes have a compatible string 'riscv' but that does not provide any information about the existence of the Zkr extension. That information is in the 'riscv,isa-extensions' property of the cpu nodes (see Documentation/devicetree/bindings/riscv/cpus.yaml).
I know in Linux that I can create devices using something like platform_device_register(), does U-Boot have a similar facility?
This is what the U_BOOT_DRVINFO() macro in my driver does and which Simon discourages.
My current thoughts are that in this case we could use U_BOOT_DRVINFO() like today and then have riscv_zkr_probe() be what checks the riscv,isa-extensions property for an appropriate match? This would mean we don't need any new nodes/compatibles/etc, and possibly not need any bootph- properties added either? I assume we don't need the RNG so early as for that to be an issue.
The presence of the Zkr extension in the device-tree 'riscv,isa-extensions' property does not indicate if the machine mode firmware has enabled access in supervisor mode via the mseccfg.sseed flag. This is why my driver tries to read the seed register and checks if an exception occurs.
This I think is a question for Cody or someone else in the RISC-V community? If just checking the property isn't sufficient, what is? Or, what's the best / most reliable way? I don't know and I'll let the RISC-V community sort that out instead of making incorrect assumptions myself.
Additionally checking the device-tree would increase code size. Is it really needed?
I mean, it depends? My biggest issue right now is that in-tree I don't see any RISC-V targets that select any of the RISCV_ISA options so I can't evaluate what platforms grow by how much where. We shouldn't be talking about kilobytes of change, so no, I don't think somehow checking the device tree for this is out of bounds. But it comes back to what I just asked above, first. We need the correct and expected way to check for this feature to be known, then we decide how to handle that.
Hardware with the Zkr extension is yet to hit the market. Please, run upstream QEMU (commit 2f32dcabc2f0 or later) with -cpu rv64,zkr=on to see the device-tree entry. A patch for OpenSBI to enable mseccfg.sseed is pending.
We may have multiple RNG drivers enabled on the same device, e.g. TPM, Zkr, virtio-rng. In several code locations we try to use the first RNG device (not the first successfully probed RNG device). This is why
[PATCH 1/1] rng: detect RISC-V Zkr RNG device in bind method https://lore.kernel.org/u-boot/20231104065107.23623-1-heinrich.schuchardt@ca...
moves the detection from probe() to bind(). The patch further corrects the return code if the RNG device is not available.
Sounds like we have some other general issues to sort out too then. Filing an issue on https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/ would be good so it doesn't get forgotten.
Here is the issue:
Missing function to find first successfully probed device for a uclass https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/8
Best regards
Heinrich
On Tue, 07 Nov 2023 15:12:16 PST (-0800), Conor Dooley wrote:
+CC Palmer
On Tue, Nov 07, 2023 at 05:38:37PM -0500, Tom Rini wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being
Also just kind of jumping in: I don't usually follow u-boot stuff, but a few of us ended up talking abot this.
asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions
For this one, it's pretty straightforward IMO - if riscv,isa-extensions contains "zbb", then you are safe to use those instructions. My understanding is that relying on getting illegal instruction traps is not a sufficient test for usability of standard extensions, as a vendor extension could be using the same opcodes as a standard extension.
Not just could, but we've got systems that actually overlay vendor-specific behavior onto the standard encoding space. There's a lot of small offenders for things like errata, but there's also stuff like T-Head where huge chunks of space reserved by the ISA for standard stuff gets reused.
so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
The ordinary extensions that are instructions - like Zbkb that provides bit manipulation instructions for cryptography you will be able to rely on riscv,isa-extensions also. Zkr is actually a CSR acting as an entropy source and is a bit more complicated. RISC-V Cryptography Extensions Volume I, Chapter Four [0] is the relevant thing for use of the CSR provided by Zkr, and it says "The seed CSR is also access controlled by execution mode, and attempted read or write access will raise an illegal instruction exception outside M mode unless access is explicitly granted." My take is that either the SBI implementation needs to provide S-Mode U-Boot with an accurate devicetree (including what extensions are valid for use in S-mode) or if the devicetree is provided as part of the U-Boot binary then it needs to match what is available at that privilege level on the platform. In this case, you would also be able to rely on riscv,isa-extensions for that detection. There is an existing dt-binding patch https://lore.kernel.org/linux-riscv/20231107105556.517187-6-cleger@rivosinc.com/ that adds Zkr, and my proposal would be to document that the presence of Zkr explicitly in riscv,isa-extensions means that the bit in mseccfg.[s,u]seed has been set so it can be used at the current privilege level.
FWIW, that seems generally viable to me.
If that's not acceptable, and people think that having Zkr in the devicetree means that the hardware has the extension, regardless of usability at the present privilege level, then IMO we need an SBI ecall defined to request entablement of the CSR & report as to whether or not that was possible.
I think we can start without the SBI interface, but I'm not 100% sure. I was worried about writes to "seed" somehow resulting in an information leak, but the spec says "The write value (in rs1 or uimm) must be ignored by implementations." so I think we're safe.
I'm not sure how any of the above lines up with the ARMv8.5 RNG feature unfortunately.
All I know is what's in this patch set https://patchwork.kernel.org/project/linux-arm-kernel/patch/20191019022048.28065-2-richard.henderson@linaro.org/. It looks generalyl to me like the RNDR bits in "cpu-features-registers.rst" would coorespond to "Zkr" being set in "riscv,isa-extensions" -- we don't have ISA-defined feature registers, hence why all this ends up shimed in via DT.
Cheers, Conor.
0 - https://github.com/riscv/riscv-crypto/releases/tag/v1.0.1-scalar
Hi,
On Wed, 8 Nov 2023 at 10:38, Palmer Dabbelt palmer@dabbelt.com wrote:
On Tue, 07 Nov 2023 15:12:16 PST (-0800), Conor Dooley wrote:
+CC Palmer
On Tue, Nov 07, 2023 at 05:38:37PM -0500, Tom Rini wrote:
On Tue, Nov 07, 2023 at 10:27:50PM +0000, Conor Dooley wrote:
On Tue, Nov 07, 2023 at 05:10:23PM -0500, Tom Rini wrote:
further clarify or not the RISC-V ISA thing that's elsewhere in this thread (and part of the kernel, not a U-Boot thing).
TBH, this a bit fragmented across threads, and as someone that hasn't been following it it's a bit difficult to tell exactly what is being
Also just kind of jumping in: I don't usually follow u-boot stuff, but a few of us ended up talking abot this.
asked for. Would someone be able to ask it as a direct question?
Sorry for being unclear, and thanks for asking. What I think the U-Boot community would like to know is, what is the device-tree based way to know if a RISC-V platform has the Zbb extensions
For this one, it's pretty straightforward IMO - if riscv,isa-extensions contains "zbb", then you are safe to use those instructions. My understanding is that relying on getting illegal instruction traps is not a sufficient test for usability of standard extensions, as a vendor extension could be using the same opcodes as a standard extension.
Not just could, but we've got systems that actually overlay vendor-specific behavior onto the standard encoding space. There's a lot of small offenders for things like errata, but there's also stuff like T-Head where huge chunks of space reserved by the ISA for standard stuff gets reused.
so the RNG opcodes, similar (in concept at least?) to the ARMv8.5 RNG feature.
The ordinary extensions that are instructions - like Zbkb that provides bit manipulation instructions for cryptography you will be able to rely on riscv,isa-extensions also. Zkr is actually a CSR acting as an entropy source and is a bit more complicated. RISC-V Cryptography Extensions Volume I, Chapter Four [0] is the relevant thing for use of the CSR provided by Zkr, and it says "The seed CSR is also access controlled by execution mode, and attempted read or write access will raise an illegal instruction exception outside M mode unless access is explicitly granted." My take is that either the SBI implementation needs to provide S-Mode U-Boot with an accurate devicetree (including what extensions are valid for use in S-mode) or if the devicetree is provided as part of the U-Boot binary then it needs to match what is available at that privilege level on the platform. In this case, you would also be able to rely on riscv,isa-extensions for that detection. There is an existing dt-binding patch https://lore.kernel.org/linux-riscv/20231107105556.517187-6-cleger@rivosinc.com/ that adds Zkr, and my proposal would be to document that the presence of Zkr explicitly in riscv,isa-extensions means that the bit in mseccfg.[s,u]seed has been set so it can be used at the current privilege level.
FWIW, that seems generally viable to me.
If that's not acceptable, and people think that having Zkr in the devicetree means that the hardware has the extension, regardless of usability at the present privilege level, then IMO we need an SBI ecall defined to request entablement of the CSR & report as to whether or not that was possible.
I think we can start without the SBI interface, but I'm not 100% sure. I was worried about writes to "seed" somehow resulting in an information leak, but the spec says "The write value (in rs1 or uimm) must be ignored by implementations." so I think we're safe.
I'm not sure how any of the above lines up with the ARMv8.5 RNG feature unfortunately.
All I know is what's in this patch set https://patchwork.kernel.org/project/linux-arm-kernel/patch/20191019022048.28065-2-richard.henderson@linaro.org/. It looks generalyl to me like the RNDR bits in "cpu-features-registers.rst" would coorespond to "Zkr" being set in "riscv,isa-extensions" -- we don't have ISA-defined feature registers, hence why all this ends up shimed in via DT.
Thanks for the info.
What is the hardware architecture of the RNG? Is there a single RNG in the SoC or does each CPU have its own? Is it configurable in any way?
Cheers, Conor.
0 - https://github.com/riscv/riscv-crypto/releases/tag/v1.0.1-scalar
Regards, Simon
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c... > > 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...
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-an...
Regards, Simon
Hi Andre,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c... > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
+ /* Check if reading seed leads to interrupt */ + set_resume(&resume); + ret = setjmp(resume.jump); + if (ret) + log_debug("Exception %ld reading seed CSR\n", resume.code); + else + val = read_seed(); + set_resume(NULL); + if (ret) + return -ENODEV;
I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
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.
I don't know what that is.
In the other email I proposed a binding for this, so I hope that can make progress.
Regards, SImon
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... > > 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-an...
Regards, SImon
On Tue, 7 Nov 2023 01:08:15 +0000 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
> Hi Heinrich, > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > heinrich.schuchardt@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@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.c... > > > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
It seems to be common agreement between devicetree folks, and I find it in one of Frank Roward's slidedeck about devicetree in the early days (2015ish). But indeed this should be added to official documents. I poked some people to get this sorted.
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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
Ah, sorry, I sense some misunderstanding: I was arguing about the ARM RNDR driver. The Arm architecture manual describes the FEAT_RNG feature as perfectly discoverable, in a clean way, without any risk or further knowledge about the platform.
This thread here was originally about the RISC-V driver (written by Heinrich), where the situation is slightly different: while there seem to be CSRs to discover CPU features, this is apparently not the case for every instruction. So Heinrich did some probing, testing for an illegal instruction, which honestly still sounds better than a DT node to me.
/* Check if reading seed leads to interrupt */set_resume(&resume);ret = setjmp(resume.jump);if (ret)log_debug("Exception %ld reading seed CSR\n", resume.code);elseval = read_seed();set_resume(NULL);if (ret)return -ENODEV;I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
For the RISC-V case: maybe. But there is already a (newish) binding to list CPU features in the DT: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... It's just not a normal device node binding, with a compatible string, instead a string list inside each CPU's node.
So one possibility would be some connector code that parses that list and looks for drivers having registered? Like a CPU bus, I think Sean proposed something like this earlier. Or we ditch the idea of this being a regular driver in the first place, instead go with a "CPU entropy instruction abstraction".
But for Arm it's a different story.
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.
I don't know what that is.
That's what Tom and I were talking about earlier: ... "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."
In the other email I proposed a binding for this, so I hope that can make progress.
I don't think we need a new DT binding for RISC-V, instead lean on riscv,isa-extensions. And I am pretty sure any attempt at a binding for ARM will be NAKed immediately.
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... > > > > 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-an...
Regards, SImon
Hi Andre,
On Tue, 7 Nov 2023 at 04:27, Andre Przywara andre.przywara@arm.com wrote:
On Tue, 7 Nov 2023 01:08:15 +0000 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote: > > On Fri, 3 Nov 2023 13:38:58 -0600 > Simon Glass sjg@chromium.org wrote: > > Hi Simon, > > > Hi Heinrich, > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > heinrich.schuchardt@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@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.c... > > > > > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
It seems to be common agreement between devicetree folks, and I find it in one of Frank Roward's slidedeck about devicetree in the early days (2015ish). But indeed this should be added to official documents. I poked some people to get this sorted.
Yes I recall those dark days but it is not actually correct. That sort of restriction would be very destructive, in fact.
Even if you look at all the PCI stuff you can see that specifying probe-able stuff in the DT is fine.
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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
Ah, sorry, I sense some misunderstanding: I was arguing about the ARM RNDR driver. The Arm architecture manual describes the FEAT_RNG feature as perfectly discoverable, in a clean way, without any risk or further knowledge about the platform.
This thread here was originally about the RISC-V driver (written by Heinrich), where the situation is slightly different: while there seem to be CSRs to discover CPU features, this is apparently not the case for every instruction. So Heinrich did some probing, testing for an illegal instruction, which honestly still sounds better than a DT node to me.
/* Check if reading seed leads to interrupt */set_resume(&resume);ret = setjmp(resume.jump);if (ret)log_debug("Exception %ld reading seed CSR\n", resume.code);elseval = read_seed();set_resume(NULL);if (ret)return -ENODEV;I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
For the RISC-V case: maybe. But there is already a (newish) binding to list CPU features in the DT: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... It's just not a normal device node binding, with a compatible string, instead a string list inside each CPU's node.
Hmm so each CPU has its own random-number generator?
So one possibility would be some connector code that parses that list and looks for drivers having registered? Like a CPU bus, I think Sean proposed something like this earlier. Or we ditch the idea of this being a regular driver in the first place, instead go with a "CPU entropy instruction abstraction".
But for Arm it's a different story.
The key thing here is that U-Boot (mostly) needs to have a DT node for each device it creates. In the case of a random-number generator, there can be several devices in the system. We want to control which one is used for a particular feature. This is normally done with aliases, or with a phandle from the feature that uses it.
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.
I don't know what that is.
That's what Tom and I were talking about earlier: ... "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."
That doesn't solve the problem, though. The TPM may provide random numbers. There may be some other crypto thing, or even a remoteproc interface.
We already have a perfectly good way of selecting between multiple devices. It is used all over U-Boot. We should not be inventing a hard-coded hack just because we are confused about whether something is a device. Just make it a device.
In the other email I proposed a binding for this, so I hope that can make progress.
I don't think we need a new DT binding for RISC-V, instead lean on riscv,isa-extensions.
IMO we do need a new DT binding for the reasons given above.
And I am pretty sure any attempt at a binding for ARM will be NAKed immediately.
Well perhaps you can help resolve that, which seems to be the core issue here. I hope you can understand my frustration at this sort of tactic. It is quite destructive. U-Boot has suffered for years from an inability to upstream bindings. It has been a significant drag on the project and its contributors. We need to change the conversation here and permit non-Linux projects to contribute to bindings for firmware-specific reasons, even ones which Linux doesn't care about. A clear statement to that effect would put my mind at ease. It just shouldn't be this hard.
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... > > > > > > 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-an... >
Regards, SImon
On Tue, 7 Nov 2023 05:22:58 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Andre,
On Tue, 7 Nov 2023 at 04:27, Andre Przywara andre.przywara@arm.com wrote:
On Tue, 7 Nov 2023 01:08:15 +0000 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
> On Sat, 4 Nov 2023 at 17:13, Andre Przywara > andre.przywara@arm.com wrote: > > > > On Fri, 3 Nov 2023 13:38:58 -0600 > > Simon Glass sjg@chromium.org wrote: > > > > Hi Simon, > > > > > Hi Heinrich, > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > > heinrich.schuchardt@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@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.c... > > > > > > > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
It seems to be common agreement between devicetree folks, and I find it in one of Frank Roward's slidedeck about devicetree in the early days (2015ish). But indeed this should be added to official documents. I poked some people to get this sorted.
Yes I recall those dark days but it is not actually correct. That sort of restriction would be very destructive, in fact.
Even if you look at all the PCI stuff you can see that specifying probe-able stuff in the DT is fine.
In the recent PCI case this is exactly about non-probe-able stuff: "This is the groundwork for applying overlays to PCI devices containing non-discoverable downstream devices." And if I understand this correctly, this is for generating DT nodes in the live OF tree only, during kernel runtime?
But yes, there are exceptions from that rule, be it for quirks, or if you need further integration information, like an interrupt, as used for the Arm Generic Timer (aka arch timer) or the Arm PMU. But in those cases there is always a good reason, and then a binding is accepted. But in general: no, if we can safely probe it, we don't want a DT node.
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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
Ah, sorry, I sense some misunderstanding: I was arguing about the ARM RNDR driver. The Arm architecture manual describes the FEAT_RNG feature as perfectly discoverable, in a clean way, without any risk or further knowledge about the platform.
This thread here was originally about the RISC-V driver (written by Heinrich), where the situation is slightly different: while there seem to be CSRs to discover CPU features, this is apparently not the case for every instruction. So Heinrich did some probing, testing for an illegal instruction, which honestly still sounds better than a DT node to me.
/* Check if reading seed leads to interrupt */set_resume(&resume);ret = setjmp(resume.jump);if (ret)log_debug("Exception %ld reading seed CSR\n",resume.code);
elseval = read_seed();set_resume(NULL);if (ret)return -ENODEV;I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
For the RISC-V case: maybe. But there is already a (newish) binding to list CPU features in the DT: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... It's just not a normal device node binding, with a compatible string, instead a string list inside each CPU's node.
Hmm so each CPU has its own random-number generator?
Don't know what you mean? How those CPU instructions typically work is that there are generic *architected* instructions or system registers, described by the official ARMv8.5 FEAT_RNG, in this case, while the actual entropy source is provided by the integrator. So it's a common interface to whatever TRNG the SoC might already have anyway. And one reason for this feature was to exactly do away with the device specific interfaces and discovery, and allow easy access for any user.
So one possibility would be some connector code that parses that list and looks for drivers having registered? Like a CPU bus, I think Sean proposed something like this earlier. Or we ditch the idea of this being a regular driver in the first place, instead go with a "CPU entropy instruction abstraction".
But for Arm it's a different story.
The key thing here is that U-Boot (mostly) needs to have a DT node for each device it creates.
That's a (current?) U-Boot design decision, unrelated to the hardware it describes. Hence nothing that really justifies a hardware DTB description.
In the case of a random-number generator, there can be several devices in the system.
Yes, and others (Linux, for instance) can apparently just cope fine with this.
We want to control which one is used for a particular feature.
So that means it's a policy decision, which might even differ between different users with different needs. So again not hardware related.
This is normally done with aliases, or with a phandle from the feature that uses it.
Well, aliases and phandles are DT features, so naturally apply to devices in the DT. But that doesn't mean we need to shoehorn everything into some DT. And part of that is policy again, so doesn't really belong in the *hardware* DT.
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.
I don't know what that is.
That's what Tom and I were talking about earlier: ... "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."
That doesn't solve the problem, though. The TPM may provide random numbers. There may be some other crypto thing, or even a remoteproc interface.
Yes, or a PCI based entropy source, which would have a normal driver, but no DT as well. And anyway you might expose this via a driver, but I don't see why every device must be described in the DT.
We already have a perfectly good way of selecting between multiple devices. It is used all over U-Boot. We should not be inventing a hard-coded hack just because we are confused about whether something is a device. Just make it a device.
And this is exactly what my ARM RNDR driver was doing: using a UCLASS_RNG driver interface to expose entropy. I just don't understand why we desperately need a DT node for that? I understand that U_BOOT_DRVINFO should not be used, and this is fine. So if you don't like the Linux style approach of treating CPU instructions separately, that's fair enough, but I also don't see why a U-Boot DM driver must be DT only.
In the other email I proposed a binding for this, so I hope that can make progress.
I don't think we need a new DT binding for RISC-V, instead lean on riscv,isa-extensions.
IMO we do need a new DT binding for the reasons given above.
I don't agree, the current binding gives you perfect discoverability, just not via the usual compatible string search. But that's some discussion for the DT maintainers anyway ...
And I am pretty sure any attempt at a binding for ARM will be NAKed immediately.
Well perhaps you can help resolve that, which seems to be the core issue here. I hope you can understand my frustration at this sort of tactic. It is quite destructive. U-Boot has suffered for years from an inability to upstream bindings. It has been a significant drag on the project and its contributors. We need to change the conversation here and permit non-Linux projects to contribute to bindings for firmware-specific reasons, even ones which Linux doesn't care about.
The Linux kernel repo does accept bindings for devices that Linux doesn't have or need a driver for (DRAM controllers, for instance). If you look at DT binding patch submissions, maintainers routinely object the word "driver" or even the mentioning of "Linux" in there, because the binding is user agnostic and not bound to a Linux driver at all. And the DT maintainers are very clear that those are not the the "Linux DT bindings", but that the Linux kernel community is just the place that provides the review experience and the infrastructure to host the binding files.
So in those cases I don't think it's about the DT maintainers being hostile or unreasonable, it's just a general problem of some requests being out of scope of a *hardware* DTB.
A clear statement to that effect would put my mind at ease. It just shouldn't be this hard.
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. 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.
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... > > > > > > > > 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-an... > > > > >
Regards, SImon
On Tue, Nov 07, 2023 at 03:12:48PM +0000, Andre Przywara wrote:
On Tue, 7 Nov 2023 05:22:58 -0700 Simon Glass sjg@chromium.org wrote:
[snip]
We already have a perfectly good way of selecting between multiple devices. It is used all over U-Boot. We should not be inventing a hard-coded hack just because we are confused about whether something is a device. Just make it a device.
And this is exactly what my ARM RNDR driver was doing: using a UCLASS_RNG driver interface to expose entropy. I just don't understand why we desperately need a DT node for that? I understand that U_BOOT_DRVINFO should not be used, and this is fine.
I think part of the problem is that U_BOOT_DRVINFO probably is the right answer for more things than it might have been intended for at first. Because yes, looking at the driver right now, whatever semantic-wise was mentioned earlier in the thread is applicable, but it otherwise looks like the easy path to "one device has many functions" without us having to develop some special case bus to put things on.
Hi Andre,
On Tue, 7 Nov 2023 at 08:12, Andre Przywara andre.przywara@arm.com wrote:
On Tue, 7 Nov 2023 05:22:58 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Andre,
On Tue, 7 Nov 2023 at 04:27, Andre Przywara andre.przywara@arm.com wrote:
On Tue, 7 Nov 2023 01:08:15 +0000 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote: > > On Sat, 4 Nov 2023 19:45:06 +0000 > Simon Glass sjg@chromium.org wrote: > > Hi, > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara > > andre.przywara@arm.com wrote: > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600 > > > Simon Glass sjg@chromium.org wrote: > > > > > > Hi Simon, > > > > > > > Hi Heinrich, > > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > > > heinrich.schuchardt@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@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.c... > > > > > > > > > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
It seems to be common agreement between devicetree folks, and I find it in one of Frank Roward's slidedeck about devicetree in the early days (2015ish). But indeed this should be added to official documents. I poked some people to get this sorted.
Yes I recall those dark days but it is not actually correct. That sort of restriction would be very destructive, in fact.
Even if you look at all the PCI stuff you can see that specifying probe-able stuff in the DT is fine.
In the recent PCI case this is exactly about non-probe-able stuff: "This is the groundwork for applying overlays to PCI devices containing non-discoverable downstream devices." And if I understand this correctly, this is for generating DT nodes in the live OF tree only, during kernel runtime?
But yes, there are exceptions from that rule, be it for quirks, or if you need further integration information, like an interrupt, as used for the Arm Generic Timer (aka arch timer) or the Arm PMU. But in those cases there is always a good reason, and then a binding is accepted. But in general: no, if we can safely probe it, we don't want a DT node.
> 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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
Ah, sorry, I sense some misunderstanding: I was arguing about the ARM RNDR driver. The Arm architecture manual describes the FEAT_RNG feature as perfectly discoverable, in a clean way, without any risk or further knowledge about the platform.
This thread here was originally about the RISC-V driver (written by Heinrich), where the situation is slightly different: while there seem to be CSRs to discover CPU features, this is apparently not the case for every instruction. So Heinrich did some probing, testing for an illegal instruction, which honestly still sounds better than a DT node to me.
/* Check if reading seed leads to interrupt */set_resume(&resume);ret = setjmp(resume.jump);if (ret)log_debug("Exception %ld reading seed CSR\n",resume.code);
elseval = read_seed();set_resume(NULL);if (ret)return -ENODEV;I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
For the RISC-V case: maybe. But there is already a (newish) binding to list CPU features in the DT: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... It's just not a normal device node binding, with a compatible string, instead a string list inside each CPU's node.
Hmm so each CPU has its own random-number generator?
Don't know what you mean? How those CPU instructions typically work is that there are generic *architected* instructions or system registers, described by the official ARMv8.5 FEAT_RNG, in this case, while the actual entropy source is provided by the integrator. So it's a common interface to whatever TRNG the SoC might already have anyway. And one reason for this feature was to exactly do away with the device specific interfaces and discovery, and allow easy access for any user.
So one possibility would be some connector code that parses that list and looks for drivers having registered? Like a CPU bus, I think Sean proposed something like this earlier. Or we ditch the idea of this being a regular driver in the first place, instead go with a "CPU entropy instruction abstraction".
But for Arm it's a different story.
The key thing here is that U-Boot (mostly) needs to have a DT node for each device it creates.
That's a (current?) U-Boot design decision, unrelated to the hardware it describes. Hence nothing that really justifies a hardware DTB description.
In the case of a random-number generator, there can be several devices in the system.
Yes, and others (Linux, for instance) can apparently just cope fine with this.
We want to control which one is used for a particular feature.
So that means it's a policy decision, which might even differ between different users with different needs. So again not hardware related.
This is normally done with aliases, or with a phandle from the feature that uses it.
Well, aliases and phandles are DT features, so naturally apply to devices in the DT. But that doesn't mean we need to shoehorn everything into some DT. And part of that is policy again, so doesn't really belong in the *hardware* DT.
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.
I don't know what that is.
That's what Tom and I were talking about earlier: ... "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."
That doesn't solve the problem, though. The TPM may provide random numbers. There may be some other crypto thing, or even a remoteproc interface.
Yes, or a PCI based entropy source, which would have a normal driver, but no DT as well. And anyway you might expose this via a driver, but I don't see why every device must be described in the DT.
We already have a perfectly good way of selecting between multiple devices. It is used all over U-Boot. We should not be inventing a hard-coded hack just because we are confused about whether something is a device. Just make it a device.
And this is exactly what my ARM RNDR driver was doing: using a UCLASS_RNG driver interface to expose entropy. I just don't understand why we desperately need a DT node for that? I understand that U_BOOT_DRVINFO should not be used, and this is fine. So if you don't like the Linux style approach of treating CPU instructions separately, that's fair enough, but I also don't see why a U-Boot DM driver must be DT only.
In the other email I proposed a binding for this, so I hope that can make progress.
I don't think we need a new DT binding for RISC-V, instead lean on riscv,isa-extensions.
IMO we do need a new DT binding for the reasons given above.
I don't agree, the current binding gives you perfect discoverability, just not via the usual compatible string search. But that's some discussion for the DT maintainers anyway ...
And I am pretty sure any attempt at a binding for ARM will be NAKed immediately.
Well perhaps you can help resolve that, which seems to be the core issue here. I hope you can understand my frustration at this sort of tactic. It is quite destructive. U-Boot has suffered for years from an inability to upstream bindings. It has been a significant drag on the project and its contributors. We need to change the conversation here and permit non-Linux projects to contribute to bindings for firmware-specific reasons, even ones which Linux doesn't care about.
The Linux kernel repo does accept bindings for devices that Linux doesn't have or need a driver for (DRAM controllers, for instance). If you look at DT binding patch submissions, maintainers routinely object the word "driver" or even the mentioning of "Linux" in there, because the binding is user agnostic and not bound to a Linux driver at all. And the DT maintainers are very clear that those are not the the "Linux DT bindings", but that the Linux kernel community is just the place that provides the review experience and the infrastructure to host the binding files.
So in those cases I don't think it's about the DT maintainers being hostile or unreasonable, it's just a general problem of some requests being out of scope of a *hardware* DTB.
A clear statement to that effect would put my mind at ease. It just shouldn't be this hard.
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. 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... > > > > > > > > > > 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-an... > > > > > > >
Regards, SImon
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... > > > > > > > > > > > > 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-an... > > > > > > > > >
Regards, SImon
On Tue, Nov 07, 2023 at 05:22:58AM -0700, Simon Glass wrote:
Hi Andre,
On Tue, 7 Nov 2023 at 04:27, Andre Przywara andre.przywara@arm.com wrote:
On Tue, 7 Nov 2023 01:08:15 +0000 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
> On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote: > > > > On Fri, 3 Nov 2023 13:38:58 -0600 > > Simon Glass sjg@chromium.org wrote: > > > > Hi Simon, > > > > > Hi Heinrich, > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > > heinrich.schuchardt@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@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.c... > > > > > > > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
It seems to be common agreement between devicetree folks, and I find it in one of Frank Roward's slidedeck about devicetree in the early days (2015ish). But indeed this should be added to official documents. I poked some people to get this sorted.
Yes I recall those dark days but it is not actually correct. That sort of restriction would be very destructive, in fact.
Even if you look at all the PCI stuff you can see that specifying probe-able stuff in the DT is fine.
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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
Ah, sorry, I sense some misunderstanding: I was arguing about the ARM RNDR driver. The Arm architecture manual describes the FEAT_RNG feature as perfectly discoverable, in a clean way, without any risk or further knowledge about the platform.
This thread here was originally about the RISC-V driver (written by Heinrich), where the situation is slightly different: while there seem to be CSRs to discover CPU features, this is apparently not the case for every instruction. So Heinrich did some probing, testing for an illegal instruction, which honestly still sounds better than a DT node to me.
/* Check if reading seed leads to interrupt */set_resume(&resume);ret = setjmp(resume.jump);if (ret)log_debug("Exception %ld reading seed CSR\n", resume.code);elseval = read_seed();set_resume(NULL);if (ret)return -ENODEV;I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
For the RISC-V case: maybe. But there is already a (newish) binding to list CPU features in the DT: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... It's just not a normal device node binding, with a compatible string, instead a string list inside each CPU's node.
Hmm so each CPU has its own random-number generator?
So one possibility would be some connector code that parses that list and looks for drivers having registered? Like a CPU bus, I think Sean proposed something like this earlier. Or we ditch the idea of this being a regular driver in the first place, instead go with a "CPU entropy instruction abstraction".
But for Arm it's a different story.
The key thing here is that U-Boot (mostly) needs to have a DT node for each device it creates. In the case of a random-number generator,
Well, "mostly" is not "only". And I certainly never agreed that we would only and ever find devices based on device tree. Especially in the cases where we can cleanly run-time probe for a feature. Which in the ARM case we're talking here is exactly how it was implemented.
there can be several devices in the system. We want to control which one is used for a particular feature. This is normally done with aliases, or with a phandle from the feature that uses it.
Then this is what we need to think about. Even then, I wonder what exactly we're doing for priority and perhaps should be thinking about that differently? Let the specific RNG driver tell the uclass just how Good (or trustworthy or whatever) it is. Or maybe they're all equally Good/Trustworthy/Valid, and we only need one of them really.
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.
I don't know what that is.
That's what Tom and I were talking about earlier: ... "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."
That doesn't solve the problem, though. The TPM may provide random numbers. There may be some other crypto thing, or even a remoteproc interface.
We already have a perfectly good way of selecting between multiple devices. It is used all over U-Boot. We should not be inventing a hard-coded hack just because we are confused about whether something is a device. Just make it a device.
I think you bringing up TPM is a really good example of why we need to think about what perhaps U-Boot is doing wrong and needs to figure out how to handle better. If I recall the long thread about TPM and RNG correctly, a spec compliant TPM provides an RNG. There is no other "device" here, nor is it optional. RNG is a function the TPM provides, by definition and specification. What we're talking about here, in both the RISC-V and ARM cases is the same thing. The CPU device provides RNG as a function. It's not another "device", it's a function. Perhaps the problem is that U-Boot needs to better handle the case where a device can provide many functions.
In the other email I proposed a binding for this, so I hope that can make progress.
I don't think we need a new DT binding for RISC-V, instead lean on riscv,isa-extensions.
IMO we do need a new DT binding for the reasons given above.
Maybe there's an argument that the RISC-V ISA binding should be compatibles, or maybe we could use that _if_ it's relevant to us because we don't just know at run-time.
And further, it's possible to argue I think that "arm,armv8" is an insufficient compatible and platforms should have been doing "arm,armv8.5" or something too. But first it's far too late for that (maybe not for arm,armv9 but....) however I suspect the reason is that everything that matters from one rev to the next and is a mandated feature of the core can be determined at run-time and so no, there's not a further more detailed compatible, and no interest in one being added and used.
And I am pretty sure any attempt at a binding for ARM will be NAKed immediately.
Well perhaps you can help resolve that, which seems to be the core issue here. I hope you can understand my frustration at this sort of tactic. It is quite destructive. U-Boot has suffered for years from an inability to upstream bindings. It has been a significant drag on the project and its contributors. We need to change the conversation here and permit non-Linux projects to contribute to bindings for firmware-specific reasons, even ones which Linux doesn't care about. A clear statement to that effect would put my mind at ease. It just shouldn't be this hard.
And as I also keep saying in these threads, we also need to have a good reason for the bindings we're submitting.
And maybe also the device tree specification needs to clarify its stance on bindings about parts of the hardware that can be determined safely at run time.
In the end, part of the problem is going to be that for any new binding it will need to be populated by others. And in this thread we do _not_ have a good response to "Why can't you just determine it at run time?" and then "OK, but why do you need a device? Sounds like a U-Boot problem, everyone else is fine with run time."
On Tue, Nov 07, 2023 at 11:27:08AM +0000, Andre Przywara wrote:
On Tue, 7 Nov 2023 01:08:15 +0000 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 21:55, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 6 Nov 2023 13:38:39 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Mon, 6 Nov 2023 at 10:26, Andre Przywara andre.przywara@arm.com wrote:
On Sat, 4 Nov 2023 19:45:06 +0000 Simon Glass sjg@chromium.org wrote:
Hi,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara andre.przywara@arm.com wrote: > > On Fri, 3 Nov 2023 13:38:58 -0600 > Simon Glass sjg@chromium.org wrote: > > Hi Simon, > > > Hi Heinrich, > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt > > heinrich.schuchardt@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@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.c... > > > > > > > > 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.
Really? Where is that even coming from? Certainly not the DT spec.
It seems to be common agreement between devicetree folks, and I find it in one of Frank Roward's slidedeck about devicetree in the early days (2015ish). But indeed this should be added to official documents. I poked some people to get this sorted.
And it both dates back further still than that, less has always been more is a phrase that can apply to Linux Kernel device trees for forever. And like I put in another thread today, yes, an official declaration that "device trees for the Linux Kernel" are not the same as "semantically valid and conceptually strictly the hardware". As they aren't, and that's fine, and will make life clearer moving forward.
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.
That statement just mystifies me. Why not just send a binding? Even the people that complain that DT should only describe hardware will be happy with it.
The code you sent should have been a clue that you need to know whether the feature is present:
Ah, sorry, I sense some misunderstanding: I was arguing about the ARM RNDR driver. The Arm architecture manual describes the FEAT_RNG feature as perfectly discoverable, in a clean way, without any risk or further knowledge about the platform.
This thread here was originally about the RISC-V driver (written by Heinrich), where the situation is slightly different: while there seem to be CSRs to discover CPU features, this is apparently not the case for every instruction. So Heinrich did some probing, testing for an illegal instruction, which honestly still sounds better than a DT node to me.
/* Check if reading seed leads to interrupt */set_resume(&resume);ret = setjmp(resume.jump);if (ret)log_debug("Exception %ld reading seed CSR\n", resume.code);elseval = read_seed();set_resume(NULL);if (ret)return -ENODEV;I have never seen code like that in a driver. Please let's just have the binding discussion with the Linux people and hopefully they will see reason.
For the RISC-V case: maybe. But there is already a (newish) binding to list CPU features in the DT: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... It's just not a normal device node binding, with a compatible string, instead a string list inside each CPU's node.
And Simon, you may find it interesting to get involved there and see what is / isn't supposed to work or intended to work, in that case.
So one possibility would be some connector code that parses that list and looks for drivers having registered? Like a CPU bus, I think Sean proposed something like this earlier. Or we ditch the idea of this being a regular driver in the first place, instead go with a "CPU entropy instruction abstraction".
But for Arm it's a different story.
In terms of U-Boot design, yes, I'm not entirely sure what's best as another part of the design needs to be that we make abstractions only when we need them. Maybe RNG needs a bit more as maybe user (device designers) need better strength in some cases rather than others. But even then we should still leverage what we know can and cannot be true at build time.
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.
I don't know what that is.
That's what Tom and I were talking about earlier: ... "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."
Seeing what's right for exposing to UEFI is another question, disjoint from just having an RNG available.
In the other email I proposed a binding for this, so I hope that can make progress.
I don't think we need a new DT binding for RISC-V, instead lean on riscv,isa-extensions.
Perhaps even then we don't _need_ to be doing that because we're making binaries that I suspect won't run on platforms that lack the ISA. And so this is partly a question for the U-Boot riscv people (who are at a conference this week I believe). But if we just built a binary that will only run on a core with the Zbb ISA it'd be pretty silly for us to then run-time check if we have Zbb, since if we don't we wouldn't be there to start with.
On Sat, Nov 04, 2023 at 05:12:12PM +0000, Andre Przywara wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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 think we have a similar problem with how we're doing with DM_TIMER and armv7-a/armv7-m/armv8[1]. We shouldn't need the drivers in drivers/timer/ to cover platforms where SYS_ARCH_TIMER is (or should be!) enabled. But in turn, the code under arch/arm/cpu/*/*timer.c doesn't implement the uclass side of things, only the regular API. This is because there's nothing to probe even because we don't support the kind of multi-arch binary where it'd be possible to not have the feature.
Hi Tom,
On Mon, 6 Nov 2023 at 09:46, Tom Rini trini@konsulko.com wrote:
On Sat, Nov 04, 2023 at 05:12:12PM +0000, Andre Przywara wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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 think we have a similar problem with how we're doing with DM_TIMER and armv7-a/armv7-m/armv8[1]. We shouldn't need the drivers in drivers/timer/ to cover platforms where SYS_ARCH_TIMER is (or should be!) enabled. But in turn, the code under arch/arm/cpu/*/*timer.c doesn't implement the uclass side of things, only the regular API. This is because there's nothing to probe even because we don't support the kind of multi-arch binary where it'd be possible to not have the feature.
The difference here is that there is only one timer device, at least in hardware I have used.
I am leaning towards NAKing this and any future use of U_BOOT_DRVINFO(), in favour of a proper DT binding. It's time we stopped making this so hard. I'll reply on the other thread.
Regards, Simon
-- Tom
[1]: We do have the problem of armv7-r not having this feature so things like say TI K3 platforms need the platform driver on the Cortex-R host. A similar issue is the pre-ARMv7 i.MX platforms.
On Mon, 6 Nov 2023 11:46:27 -0500 Tom Rini trini@konsulko.com wrote:
Hi Tom,
On Sat, Nov 04, 2023 at 05:12:12PM +0000, Andre Przywara wrote:
On Fri, 3 Nov 2023 13:38:58 -0600 Simon Glass sjg@chromium.org wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt heinrich.schuchardt@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@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.c...
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 think we have a similar problem with how we're doing with DM_TIMER and armv7-a/armv7-m/armv8[1]. We shouldn't need the drivers in drivers/timer/ to cover platforms where SYS_ARCH_TIMER is (or should be!) enabled. But in turn, the code under arch/arm/cpu/*/*timer.c doesn't implement the uclass side of things, only the regular API. This is because there's nothing to probe even because we don't support the kind of multi-arch binary where it'd be possible to not have the feature.
Well, normally we indeed build a U-Boot image for one particular board, and chips typically don't gain new hardware features over night while sitting on the shelf. But then we also support software models (QEMU, Arm FVP) and "more flexible" hardware (like FPGA platforms), where the CPU features are indeed in flux. In QEMU it's as easy as "-cpu max", and people use that to test new architecture features.
For the arch timer it's a slightly different story, since every halfway recent chip has it, but still we try to detect it at places, as it's easy enough to do.
So I do believe there is some place for auto-detection, even in U-Boot.
Cheers, Andre.
participants (10)
-
Andre Przywara -
Conor Dooley -
Heinrich Schuchardt -
Ilias Apalodimas -
Leo Liang -
Palmer Dabbelt -
Rob Herring -
Sean Anderson -
Simon Glass -
Tom Rini