| Message ID | 20231104065107.23623-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Commit | 1351cd3b4b1b18cafa4893a44378ca6b1d091c8e |
| Delegated to: | Andes |
| Headers | show |
| Series | [1/1] rng: detect RISC-V Zkr RNG device in bind method | expand |
Hi Heinrich, On Sat, 4 Nov 2023 at 06:51, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > The existence of devices should be checked in the bind method and not in > the probe method. Adjust the RISC-V Zkr RNG driver accordingly. > > Use ENOENT (and not ENODEV) to signal that the device is not available. > > Fixes: ceec977ba1a9 ("rng: Provide a RNG based on the RISC-V Zkr ISA extension") > Reported-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > drivers/rng/riscv_zkr_rng.c | 34 ++++++++++++++++++++++++++-------- > 1 file changed, 26 insertions(+), 8 deletions(-) This device should be in the DT, so you have some control over which RNG is used. Regards, Simon
On 11/4/23 21:42, Simon Glass wrote: > Hi Heinrich, > > On Sat, 4 Nov 2023 at 06:51, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> The existence of devices should be checked in the bind method and not in >> the probe method. Adjust the RISC-V Zkr RNG driver accordingly. >> >> Use ENOENT (and not ENODEV) to signal that the device is not available. >> >> Fixes: ceec977ba1a9 ("rng: Provide a RNG based on the RISC-V Zkr ISA extension") >> Reported-by: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> drivers/rng/riscv_zkr_rng.c | 34 ++++++++++++++++++++++++++-------- >> 1 file changed, 26 insertions(+), 8 deletions(-) > > This device should be in the DT, so you have some control over which > RNG is used. The device-tree provided by QEMU does not contain such a device as Zkr is an ISA extension and not a device. This device-tree is not under the control of the U-Boot project. Best regards Heinrich
Hi Heinrich, On Sun, 5 Nov 2023 at 03:47, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 11/4/23 21:42, Simon Glass wrote: > > Hi Heinrich, > > > > On Sat, 4 Nov 2023 at 06:51, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> The existence of devices should be checked in the bind method and not in > >> the probe method. Adjust the RISC-V Zkr RNG driver accordingly. > >> > >> Use ENOENT (and not ENODEV) to signal that the device is not available. > >> > >> Fixes: ceec977ba1a9 ("rng: Provide a RNG based on the RISC-V Zkr ISA extension") > >> Reported-by: Andre Przywara <andre.przywara@arm.com> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> drivers/rng/riscv_zkr_rng.c | 34 ++++++++++++++++++++++++++-------- > >> 1 file changed, 26 insertions(+), 8 deletions(-) > > > > This device should be in the DT, so you have some control over which > > RNG is used. > > The device-tree provided by QEMU does not contain such a device as Zkr > is an ISA extension and not a device. This device-tree is not under the > control of the U-Boot project. Why do you bring up QEMU? I would expect that it follows the norma dt bindings, so it should not be any different from real hardware? Anyway, you could add it. It is just a binding. I believe RISC-V has all sorts of isa options which result in different machine features. What are you going to do when you want to choose between the ISA RNG and a TPM one? Regards, Simon
On 11/5/23 18:25, Simon Glass wrote: > Hi Heinrich, > > On Sun, 5 Nov 2023 at 03:47, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> On 11/4/23 21:42, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Sat, 4 Nov 2023 at 06:51, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> The existence of devices should be checked in the bind method and not in >>>> the probe method. Adjust the RISC-V Zkr RNG driver accordingly. >>>> >>>> Use ENOENT (and not ENODEV) to signal that the device is not available. >>>> >>>> Fixes: ceec977ba1a9 ("rng: Provide a RNG based on the RISC-V Zkr ISA extension") >>>> Reported-by: Andre Przywara <andre.przywara@arm.com> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>> --- >>>> drivers/rng/riscv_zkr_rng.c | 34 ++++++++++++++++++++++++++-------- >>>> 1 file changed, 26 insertions(+), 8 deletions(-) >>> >>> This device should be in the DT, so you have some control over which >>> RNG is used. >> >> The device-tree provided by QEMU does not contain such a device as Zkr >> is an ISA extension and not a device. This device-tree is not under the >> control of the U-Boot project. > > Why do you bring up QEMU? I would expect that it follows the norma dt > bindings, so it should not be any different from real hardware? Yes Simon, QEMU follows the normal bindings. If you want to change these, please, contribute to the RISC-V working groups or to the Linux kernel project. > > Anyway, you could add it. It is just a binding. I believe RISC-V has > all sorts of isa options which result in different machine features. > > What are you going to do when you want to choose between the ISA RNG > and a TPM one? First of all there are different configurations switches for both drivers. But of course you can enable both. They will be different U-Boot devices. The rng command has a parameter to choose a RNG device by device number. This is not different to having two USB drives. Best regards Heinrich
Hi Heinrich, On Sun, 5 Nov 2023 at 14:54, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 11/5/23 18:25, Simon Glass wrote: > > Hi Heinrich, > > > > On Sun, 5 Nov 2023 at 03:47, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> On 11/4/23 21:42, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Sat, 4 Nov 2023 at 06:51, Heinrich Schuchardt > >>> <heinrich.schuchardt@canonical.com> wrote: > >>>> > >>>> The existence of devices should be checked in the bind method and not in > >>>> the probe method. Adjust the RISC-V Zkr RNG driver accordingly. > >>>> > >>>> Use ENOENT (and not ENODEV) to signal that the device is not available. > >>>> > >>>> Fixes: ceec977ba1a9 ("rng: Provide a RNG based on the RISC-V Zkr ISA extension") > >>>> Reported-by: Andre Przywara <andre.przywara@arm.com> > >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>> --- > >>>> drivers/rng/riscv_zkr_rng.c | 34 ++++++++++++++++++++++++++-------- > >>>> 1 file changed, 26 insertions(+), 8 deletions(-) > >>> > >>> This device should be in the DT, so you have some control over which > >>> RNG is used. > >> > >> The device-tree provided by QEMU does not contain such a device as Zkr > >> is an ISA extension and not a device. This device-tree is not under the > >> control of the U-Boot project. > > > > Why do you bring up QEMU? I would expect that it follows the norma dt > > bindings, so it should not be any different from real hardware? > > Yes Simon, QEMU follows the normal bindings. If you want to change > these, please, contribute to the RISC-V working groups or to the Linux > kernel project. Rick, can you help with this? I am thinking of something like this as a top-level node: rng { compatible = "riscv,zkr"; }; > > > > > Anyway, you could add it. It is just a binding. I believe RISC-V has > > all sorts of isa options which result in different machine features. > > > > What are you going to do when you want to choose between the ISA RNG > > and a TPM one? > > First of all there are different configurations switches for both > drivers. But of course you can enable both. They will be different > U-Boot devices. The rng command has a parameter to choose a RNG device > by device number. This is not different to having two USB drives. Did you see the other discussion about this? Without a propert driver, there is no way to choose which RNG device is used. Also the comment is very clear: /** * struct driver_info - Information required to instantiate a device * * NOTE: Avoid using this except in extreme circumstances, where device tree * is not feasible (e.g. serial driver in SPL where <8KB of SRAM is * available). U-Boot's driver model uses device tree for configuration. Do we need a build-time warning so people don't forget this? Regards, Simon
diff --git a/drivers/rng/riscv_zkr_rng.c b/drivers/rng/riscv_zkr_rng.c index 8c9e111e2e..48a5251988 100644 --- a/drivers/rng/riscv_zkr_rng.c +++ b/drivers/rng/riscv_zkr_rng.c @@ -55,7 +55,7 @@ static int riscv_zkr_read(struct udevice *dev, void *data, size_t len) } break; case DEAD: - return -ENODEV; + return -ENOENT; } } @@ -63,16 +63,16 @@ static int riscv_zkr_read(struct udevice *dev, void *data, size_t len) } /** - * riscv_zkr_probe() - check if the seed register is available + * riscv_zkr_bind() - 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. + * If the SBI software has not set mseccfg.sseed=1 or the Zkr extension is not + * available, reading the seed register will result in an exception from which + * this function safely resumes. * * @dev: RNG device * Return: 0 if successfully probed */ -static int riscv_zkr_probe(struct udevice *dev) +static int riscv_zkr_bind(struct udevice *dev) { struct resume_data resume; int ret; @@ -87,7 +87,24 @@ static int riscv_zkr_probe(struct udevice *dev) val = read_seed(); set_resume(NULL); if (ret) - return -ENODEV; + return -ENOENT; + + return 0; +} + +/** + * riscv_zkr_probe() - check if entropy is available + * + * The bind method already checked that the seed register can be read without + * excpetiong. Here we wait for the self test to finish and entropy becoming + * available. + * + * @dev: RNG device + * Return: 0 if successfully probed + */ +static int riscv_zkr_probe(struct udevice *dev) +{ + u32 val; do { val = read_seed(); @@ -95,7 +112,7 @@ static int riscv_zkr_probe(struct udevice *dev) } while (val == BIST || val == WAIT); if (val == DEAD) - return -ENODEV; + return -ENOENT; return 0; } @@ -108,6 +125,7 @@ U_BOOT_DRIVER(riscv_zkr) = { .name = DRIVER_NAME, .id = UCLASS_RNG, .ops = &riscv_zkr_ops, + .bind = riscv_zkr_bind, .probe = riscv_zkr_probe, };
The existence of devices should be checked in the bind method and not in the probe method. Adjust the RISC-V Zkr RNG driver accordingly. Use ENOENT (and not ENODEV) to signal that the device is not available. Fixes: ceec977ba1a9 ("rng: Provide a RNG based on the RISC-V Zkr ISA extension") Reported-by: Andre Przywara <andre.przywara@arm.com> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- drivers/rng/riscv_zkr_rng.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-)