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

[Simon Glass]

Hi,

On Wed, 8 Nov 2023 at 10:38, Palmer Dabbelt  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 +, 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
> > 
> > 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
> .
> 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

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

[Palmer Dabbelt]

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 +, 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

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

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

[Heinrich Schuchardt]

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 +, 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.schucha...@canonical.com/

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

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

[Tom Rini]

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 +, 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.schucha...@canonical.com/
> 
> 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Heinrich Schuchardt]

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 +, 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.schucha...@canonical.com/

moves the detection from probe() to bind(). The patch further corrects 
the return code if the RNG device is not available.


Best regards

Heinrich

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

[Tom Rini]

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 +, 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Heinrich Schuchardt]

On 11/7/23 16:34, Tom Rini wrote:

On Wed, Nov 08, 2023 at 12:29:03AM +, 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.

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

[Ilias Apalodimas]

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
> > > > > > > > > > > > > : 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
> > > > > > > > > > > >
> > > > > > > > > > > > We have weak function platform_get_rng_device() which
> > > > > > > > > > > > should be moved to drivers/rng/rng-uclass.c.
> > > > > > > > > > > >
> > > > > > > > > > > > We could add a function to 

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

[Simon Glass]

Hi Andre,

On Tue, 7 Nov 2023 at 08:12, Andre Przywara  wrote:
>
> On Tue, 7 Nov 2023 05:22:58 -0700
> Simon Glass  wrote:
>
> Hi Simon,
>
> > Hi Andre,
> >
> > On Tue, 7 Nov 2023 at 04:27, Andre Przywara 
> > wrote:
> > >
> > > On Tue, 7 Nov 2023 01:08:15 +
> > > Simon Glass  wrote:
> > >
> > > Hi Simon,
> > >
> > > > On Mon, 6 Nov 2023 at 21:55, Andre Przywara 
> > > > wrote:
> > > > >
> > > > > On Mon, 6 Nov 2023 13:38:39 -0700
> > > > > Simon Glass  wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > > > Simon Glass  wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara
> > > > > > > >  wrote:
> > > > > > > > >
> > > > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > > > Simon Glass  wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > > Hi Heinrich,
> > > > > > > > > >
> > > > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > > > >  wrote:
> > > > > > > > > > >
> > > > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > > > Heinrich Schuchardt
> > > > > > > > > > > >  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.przyw...@arm.com/
> > > > > > > > > > > >
> > > > > > > > > > > > And I see that you seem to use the same mechanism to
> > > > > > > > > > > > probe and init the driver: U_BOOT_DRVINFO and fail
> > > > > > > > > > > > in probe() if the feature is not implemented.
> > > > > > > > > > > > One downside of this approach is that the driver is
> > > > > > > > > > > > always loaded (and visible in the DM tree), even
> > > > > > > > > > > > with the feature not being available. That doesn't
> > > > > > > > > > > > seem too much of a problem on the first glance, but
> > > > > > > > > > > > it occupies a device number, and any subsequent
> > > > > > > > > > > > other DM_RNG devices (like virtio-rng) typically get
> > > > > > > > > > > > higher device numbers. So without the feature, but
> > > > > > > > > > > > with virtio-rng, I get: VExpress64# rng 0 No RNG
> > > > > > > > > > > > device
> > > > > > > > > >
> > > > > > > > > > Why do we get this? If the device is not there, the
> > > > > > > > > > bind() function can return -ENODEV
> > > > > > > > > >
> > > > > > > > > > I see this in U-Boot:
> > > > > > > > > >
> > > > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > > > >
> > > > > > > > > > We should not use this.
> > > > > > > > >
> > > > > > > > > Agreed.
> > > > > > > > >
> > > > > > > > > > Use the devicetree.
> > > > > > > > >
> > > > > > > > > No, this is definitely not something for the DT, at least
> > > > > > > > > not on ARM. It's perfectly discoverable via the
> > > > > > > > > architected CPU ID registers. Similar to PCI and USB
> > > > > > > > > devices, which we don't probe via the DT as well.
> > > > > > > > >
> > > > > > > > > It's arguably not proper "driver" material per se, as I've
> > > > > > > > > argued before, but it's the simplest solution and fits in
> > > > > > > > > nicely otherwise.
> > > > > > > > >
> > > > > > > > > I was wondering if it might be something for UCLASS_CPU,
> > > > > > > > > something like a "CPU feature bus": to let devices
> > > > > > > > > register on one on the many CPU features (instead of
> > > > > > > > > compatible strings), then only bind() those drivers it the
> > > > > > > > > respective bit is set.
> > > > > > > > >
> > > > > > > > > Does that make sense? Would that be doable without boiling
> > > > > > > > > the ocean? As I don't know if we see many users apart from
> > > > > > > > > this.
> > > > > > > >
> > > > > > > > I have seen this so many times, where people want to avoid
> > > > > > > > putting things in the DT and then are surprised that
> > > > > > > > everything is difficult, broken and confusing. Why not just
> > > > > > > > follow the rules? It is not just about whether we can avoid
> > > > > > > > it, etc. It is about how devices fit together cohesively in
> > > > > > > > the system, and how U-Boot operates.
> > > > > > >
> > > > > > > A devicetree is only for peripherals *that cannot 

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

[Tom Rini]

On Wed, Nov 08, 2023 at 12:29:03AM +, 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Conor Dooley]

On Tue, Nov 07, 2023 at 06:23:05PM -0500, Tom Rini wrote:
> On Tue, Nov 07, 2023 at 11:12:16PM +, 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 +, 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
> > 
> > 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.


signature.asc
Description: PGP signature

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

[Tom Rini]

On Tue, Nov 07, 2023 at 11:12:16PM +, 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 +, 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
> 
> 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Tom Rini]

On Tue, Nov 07, 2023 at 03:51:21PM -0700, Simon Glass wrote:
> Hi,
> 
> On Tue, 7 Nov 2023 at 15:38, Tom Rini  wrote:
> >
> > On Tue, Nov 07, 2023 at 10:27:50PM +, 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Conor Dooley]

+CC Palmer

On Tue, Nov 07, 2023 at 05:38:37PM -0500, Tom Rini wrote:
> On Tue, Nov 07, 2023 at 10:27:50PM +, 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

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


signature.asc
Description: PGP signature

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

[Simon Glass]

Hi,

On Tue, 7 Nov 2023 at 15:38, Tom Rini  wrote:
>
> On Tue, Nov 07, 2023 at 10:27:50PM +, 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#platform-data
[3] 
https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#device-sequence-numbers
[4] 
https://u-boot.readthedocs.io/en/latest/api/dm.html?highlight=phandle#c.of_find_node_by_phandle

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

[Tom Rini]

On Tue, Nov 07, 2023 at 10:27:50PM +, 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Conor Dooley]

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?



signature.asc
Description: PGP signature

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

[Tom Rini]

On Tue, Nov 07, 2023 at 03:52:36PM -0600, Rob Herring wrote:
> On Tue, Nov 7, 2023 at 1:30 PM Tom Rini  wrote:
> >
> > On Tue, Nov 07, 2023 at 01:10:44AM +, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 6 Nov 2023 at 13:46, Tom Rini  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  
> > > > > wrote:
> > > > > >
> > > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > > Simon Glass  wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara 
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > > Simon Glass  wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > > Hi Heinrich,
> > > > > > > > >
> > > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > > >  wrote:
> > > > > > > > > >
> > > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > > Heinrich Schuchardt  
> > > > > > > > > > > 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.przyw...@arm.com/
> > > > > > > > > > >
> > > > > > > > > > > And I see that you seem to use the same mechanism to 
> > > > > > > > > > > probe and init the
> > > > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature 
> > > > > > > > > > > is not
> > > > > > > > > > > implemented.
> > > > > > > > > > > One downside of this approach is that the driver is 
> > > > > > > > > > > always loaded (and
> > > > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > > > available.
> > > > > > > > > > > That doesn't seem too much of a problem on the first 
> > > > > > > > > > > glance, but it
> > > > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > > > devices
> > > > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > > > without
> > > > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > > > VExpress64# rng 0
> > > > > > > > > > > No RNG device
> > > > > > > > >
> > > > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > > > function
> > > > > > > > > can return -ENODEV
> > > > > > > > >
> > > > > > > > > I see this in U-Boot:
> > > > > > > > >
> > > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > > >
> > > > > > > > > We should not use this.
> > > > > > > >
> > > > > > > > Agreed.
> > > > > > > >
> > > > > > > > > Use the devicetree.
> > > > > > > >
> > > > > > > > No, this is definitely not something for the DT, at least not 
> > > > > > > > on ARM.
> > > > > > > > It's perfectly discoverable via the architected CPU ID 
> > > > > > > > registers.
> > > > > > > > Similar to PCI and USB devices, which we don't probe via the DT 
> > > > > > > > as well.
> > > > > > > >
> > > > > > > > It's arguably not proper "driver" material per se, as I've 
> > > > > > > > argued before, but
> > > > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > > > >
> > > > > > > > I was wondering if it might be something for UCLASS_CPU, 
> > > > > > > > something like
> > > > > > > > a "CPU feature bus": to let devices register on one on the many 
> > > > > > > > CPU
> > > > > > > > features (instead of compatible strings), then only bind() those
> > > > > > > > drivers it the respective bit is set.
> > > > > > > >
> > > > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > > > ocean?
> > > > > > > > As I don't know if we see many users apart from this.
> > > > > > >
> > > > > > > I have seen this so many times, where people want to avoid putting
> > > > > > > things in the DT and then are surprised that everything is 
> > > > > > > difficult,
> > > > > > > broken and confusing. Why not just follow the rules? It is not 
> > > > > > > just
> > > > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > > > together cohesively in the system, and how U-Boot operates.
> > > > > >
> > > > > > A devicetree is only for peripherals *that cannot be located by 
> > > > > > probing*.
> > > > >
> > > > > I have to stop you 

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

[Tom Rini]

On Tue, Nov 07, 2023 at 03:12:48PM +, Andre Przywara wrote:
> On Tue, 7 Nov 2023 05:22:58 -0700
> Simon Glass  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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Tom Rini]

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  wrote:
> >
> > On Tue, 7 Nov 2023 01:08:15 +
> > Simon Glass  wrote:
> >
> > Hi Simon,
> >
> > > On Mon, 6 Nov 2023 at 21:55, Andre Przywara  
> > > wrote:
> > > >
> > > > On Mon, 6 Nov 2023 13:38:39 -0700
> > > > Simon Glass  wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara  
> > > > > wrote:
> > > > > >
> > > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > > Simon Glass  wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara 
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > > Simon Glass  wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > > Hi Heinrich,
> > > > > > > > >
> > > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > > >  wrote:
> > > > > > > > > >
> > > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > > Heinrich Schuchardt  
> > > > > > > > > > > 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.przyw...@arm.com/
> > > > > > > > > > >
> > > > > > > > > > > And I see that you seem to use the same mechanism to 
> > > > > > > > > > > probe and init the
> > > > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature 
> > > > > > > > > > > is not
> > > > > > > > > > > implemented.
> > > > > > > > > > > One downside of this approach is that the driver is 
> > > > > > > > > > > always loaded (and
> > > > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > > > available.
> > > > > > > > > > > That doesn't seem too much of a problem on the first 
> > > > > > > > > > > glance, but it
> > > > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > > > devices
> > > > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > > > without
> > > > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > > > VExpress64# rng 0
> > > > > > > > > > > No RNG device
> > > > > > > > >
> > > > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > > > function
> > > > > > > > > can return -ENODEV
> > > > > > > > >
> > > > > > > > > I see this in U-Boot:
> > > > > > > > >
> > > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > > >
> > > > > > > > > We should not use this.
> > > > > > > >
> > > > > > > > Agreed.
> > > > > > > >
> > > > > > > > > Use the devicetree.
> > > > > > > >
> > > > > > > > No, this is definitely not something for the DT, at least not 
> > > > > > > > on ARM.
> > > > > > > > It's perfectly discoverable via the architected CPU ID 
> > > > > > > > registers.
> > > > > > > > Similar to PCI and USB devices, which we don't probe via the DT 
> > > > > > > > as well.
> > > > > > > >
> > > > > > > > It's arguably not proper "driver" material per se, as I've 
> > > > > > > > argued before, but
> > > > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > > > >
> > > > > > > > I was wondering if it might be something for UCLASS_CPU, 
> > > > > > > > something like
> > > > > > > > a "CPU feature bus": to let devices register on one on the many 
> > > > > > > > CPU
> > > > > > > > features (instead of compatible strings), then only bind() those
> > > > > > > > drivers it the respective bit is set.
> > > > > > > >
> > > > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > > > ocean?
> > > > > > > > As I don't know if we see many users apart from this.
> > > > > > >
> > > > > > > I have seen this so many times, where people want to avoid putting
> > > > > > > things in the DT and then are surprised that everything is 
> > > > > > > difficult,
> > > > > > > broken and confusing. Why not just follow the rules? It is not 
> > > > > > > just
> > > > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > > > together cohesively in the system, and how U-Boot operates.
> > > > > >
> > > > > > A devicetree is only for peripherals *that cannot be located by 
> > > > > > probing*.
> > > 

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

[Rob Herring]

On Tue, Nov 7, 2023 at 1:30 PM Tom Rini  wrote:
>
> On Tue, Nov 07, 2023 at 01:10:44AM +, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 6 Nov 2023 at 13:46, Tom Rini  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  
> > > > wrote:
> > > > >
> > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > Simon Glass  wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara 
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > Simon Glass  wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > >  wrote:
> > > > > > > > >
> > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > Heinrich Schuchardt  
> > > > > > > > > > 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.przyw...@arm.com/
> > > > > > > > > >
> > > > > > > > > > And I see that you seem to use the same mechanism to probe 
> > > > > > > > > > and init the
> > > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature 
> > > > > > > > > > is not
> > > > > > > > > > implemented.
> > > > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > > > loaded (and
> > > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > > available.
> > > > > > > > > > That doesn't seem too much of a problem on the first 
> > > > > > > > > > glance, but it
> > > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > > devices
> > > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > > without
> > > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > > VExpress64# rng 0
> > > > > > > > > > No RNG device
> > > > > > > >
> > > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > > function
> > > > > > > > can return -ENODEV
> > > > > > > >
> > > > > > > > I see this in U-Boot:
> > > > > > > >
> > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > >
> > > > > > > > We should not use this.
> > > > > > >
> > > > > > > Agreed.
> > > > > > >
> > > > > > > > Use the devicetree.
> > > > > > >
> > > > > > > No, this is definitely not something for the DT, at least not on 
> > > > > > > ARM.
> > > > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > > > Similar to PCI and USB devices, which we don't probe via the DT 
> > > > > > > as well.
> > > > > > >
> > > > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > > > before, but
> > > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > > >
> > > > > > > I was wondering if it might be something for UCLASS_CPU, 
> > > > > > > something like
> > > > > > > a "CPU feature bus": to let devices register on one on the many 
> > > > > > > CPU
> > > > > > > features (instead of compatible strings), then only bind() those
> > > > > > > drivers it the respective bit is set.
> > > > > > >
> > > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > > ocean?
> > > > > > > As I don't know if we see many users apart from this.
> > > > > >
> > > > > > I have seen this so many times, where people want to avoid putting
> > > > > > things in the DT and then are surprised that everything is 
> > > > > > difficult,
> > > > > > broken and confusing. Why not just follow the rules? It is not just
> > > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > > together cohesively in the system, and how U-Boot operates.
> > > > >
> > > > > 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
> 

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

[Tom Rini]

On Tue, Nov 07, 2023 at 11:27:08AM +, Andre Przywara wrote:
> On Tue, 7 Nov 2023 01:08:15 +
> Simon Glass  wrote:
> 
> Hi Simon,
> 
> > On Mon, 6 Nov 2023 at 21:55, Andre Przywara  wrote:
> > >
> > > On Mon, 6 Nov 2023 13:38:39 -0700
> > > Simon Glass  wrote:
> > >
> > > Hi Simon,
> > >  
> > > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara  
> > > > wrote:  
> > > > >
> > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > Simon Glass  wrote:
> > > > >
> > > > > Hi,
> > > > >  
> > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara 
> > > > > >  wrote:  
> > > > > > >
> > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > Simon Glass  wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >  
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > >  wrote:  
> > > > > > > > >
> > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > Heinrich Schuchardt  
> > > > > > > > > > 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.przyw...@arm.com/
> > > > > > > > > >
> > > > > > > > > > And I see that you seem to use the same mechanism to probe 
> > > > > > > > > > and init the
> > > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature 
> > > > > > > > > > is not
> > > > > > > > > > implemented.
> > > > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > > > loaded (and
> > > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > > available.
> > > > > > > > > > That doesn't seem too much of a problem on the first 
> > > > > > > > > > glance, but it
> > > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > > devices
> > > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > > without
> > > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > > VExpress64# rng 0
> > > > > > > > > > No RNG device  
> > > > > > > >
> > > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > > function
> > > > > > > > can return -ENODEV
> > > > > > > >
> > > > > > > > I see this in U-Boot:
> > > > > > > >
> > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > >
> > > > > > > > We should not use this.  
> > > > > > >
> > > > > > > Agreed.
> > > > > > >  
> > > > > > > > Use the devicetree.  
> > > > > > >
> > > > > > > No, this is definitely not something for the DT, at least not on 
> > > > > > > ARM.
> > > > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > > > Similar to PCI and USB devices, which we don't probe via the DT 
> > > > > > > as well.
> > > > > > >
> > > > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > > > before, but
> > > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > > >
> > > > > > > I was wondering if it might be something for UCLASS_CPU, 
> > > > > > > something like
> > > > > > > a "CPU feature bus": to let devices register on one on the many 
> > > > > > > CPU
> > > > > > > features (instead of compatible strings), then only bind() those
> > > > > > > drivers it the respective bit is set.
> > > > > > >
> > > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > > ocean?
> > > > > > > As I don't know if we see many users apart from this.  
> > > > > >
> > > > > > I have seen this so many times, where people want to avoid putting
> > > > > > things in the DT and then are surprised that everything is 
> > > > > > difficult,
> > > > > > broken and confusing. Why not just follow the rules? It is not just
> > > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > > together cohesively in the system, and how U-Boot operates.  
> > > > >
> > > > > 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 

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

[Tom Rini]

On Tue, Nov 07, 2023 at 01:10:44AM +, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 6 Nov 2023 at 13:46, Tom Rini  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  
> > > wrote:
> > > >
> > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > Simon Glass  wrote:
> > > >
> > > > Hi,
> > > >
> > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  
> > > > > wrote:
> > > > > >
> > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > Simon Glass  wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > > > > >
> > > > > > > > > And I see that you seem to use the same mechanism to probe 
> > > > > > > > > and init the
> > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is 
> > > > > > > > > not
> > > > > > > > > implemented.
> > > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > > loaded (and
> > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > available.
> > > > > > > > > That doesn't seem too much of a problem on the first glance, 
> > > > > > > > > but it
> > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > devices
> > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > without
> > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > VExpress64# rng 0
> > > > > > > > > No RNG device
> > > > > > >
> > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > function
> > > > > > > can return -ENODEV
> > > > > > >
> > > > > > > I see this in U-Boot:
> > > > > > >
> > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > >
> > > > > > > We should not use this.
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > > Use the devicetree.
> > > > > >
> > > > > > No, this is definitely not something for the DT, at least not on 
> > > > > > ARM.
> > > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > > Similar to PCI and USB devices, which we don't probe via the DT as 
> > > > > > well.
> > > > > >
> > > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > > before, but
> > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > >
> > > > > > I was wondering if it might be something for UCLASS_CPU, something 
> > > > > > like
> > > > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > > > features (instead of compatible strings), then only bind() those
> > > > > > drivers it the respective bit is set.
> > > > > >
> > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > ocean?
> > > > > > As I don't know if we see many users apart from this.
> > > > >
> > > > > I have seen this so many times, where people want to avoid putting
> > > > > things in the DT and then are surprised that everything is difficult,
> > > > > broken and confusing. Why not just follow the rules? It is not just
> > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > together cohesively in the system, and how U-Boot operates.
> > > >
> > > > 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 

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

[Andre Przywara]

On Tue, 7 Nov 2023 05:22:58 -0700
Simon Glass  wrote:

Hi Simon,

> Hi Andre,
> 
> On Tue, 7 Nov 2023 at 04:27, Andre Przywara 
> wrote:
> >
> > On Tue, 7 Nov 2023 01:08:15 +
> > Simon Glass  wrote:
> >
> > Hi Simon,
> >  
> > > On Mon, 6 Nov 2023 at 21:55, Andre Przywara 
> > > wrote:  
> > > >
> > > > On Mon, 6 Nov 2023 13:38:39 -0700
> > > > Simon Glass  wrote:
> > > >
> > > > Hi Simon,
> > > >  
> > > > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara
> > > > >  wrote:  
> > > > > >
> > > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > > Simon Glass  wrote:
> > > > > >
> > > > > > Hi,
> > > > > >  
> > > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara
> > > > > > >  wrote:  
> > > > > > > >
> > > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > > Simon Glass  wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >  
> > > > > > > > > Hi Heinrich,
> > > > > > > > >
> > > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > > >  wrote:  
> > > > > > > > > >
> > > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > > Heinrich Schuchardt
> > > > > > > > > > >  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.przyw...@arm.com/
> > > > > > > > > > >
> > > > > > > > > > > And I see that you seem to use the same mechanism to
> > > > > > > > > > > probe and init the driver: U_BOOT_DRVINFO and fail
> > > > > > > > > > > in probe() if the feature is not implemented.
> > > > > > > > > > > One downside of this approach is that the driver is
> > > > > > > > > > > always loaded (and visible in the DM tree), even
> > > > > > > > > > > with the feature not being available. That doesn't
> > > > > > > > > > > seem too much of a problem on the first glance, but
> > > > > > > > > > > it occupies a device number, and any subsequent
> > > > > > > > > > > other DM_RNG devices (like virtio-rng) typically get
> > > > > > > > > > > higher device numbers. So without the feature, but
> > > > > > > > > > > with virtio-rng, I get: VExpress64# rng 0 No RNG
> > > > > > > > > > > device  
> > > > > > > > >
> > > > > > > > > Why do we get this? If the device is not there, the
> > > > > > > > > bind() function can return -ENODEV
> > > > > > > > >
> > > > > > > > > I see this in U-Boot:
> > > > > > > > >
> > > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > > >
> > > > > > > > > We should not use this.  
> > > > > > > >
> > > > > > > > Agreed.
> > > > > > > >  
> > > > > > > > > Use the devicetree.  
> > > > > > > >
> > > > > > > > No, this is definitely not something for the DT, at least
> > > > > > > > not on ARM. It's perfectly discoverable via the
> > > > > > > > architected CPU ID registers. Similar to PCI and USB
> > > > > > > > devices, which we don't probe via the DT as well.
> > > > > > > >
> > > > > > > > It's arguably not proper "driver" material per se, as I've
> > > > > > > > argued before, but it's the simplest solution and fits in
> > > > > > > > nicely otherwise.
> > > > > > > >
> > > > > > > > I was wondering if it might be something for UCLASS_CPU,
> > > > > > > > something like a "CPU feature bus": to let devices
> > > > > > > > register on one on the many CPU features (instead of
> > > > > > > > compatible strings), then only bind() those drivers it the
> > > > > > > > respective bit is set.
> > > > > > > >
> > > > > > > > Does that make sense? Would that be doable without boiling
> > > > > > > > the ocean? As I don't know if we see many users apart from
> > > > > > > > this.  
> > > > > > >
> > > > > > > I have seen this so many times, where people want to avoid
> > > > > > > putting things in the DT and then are surprised that
> > > > > > > everything is difficult, broken and confusing. Why not just
> > > > > > > follow the rules? It is not just about whether we can avoid
> > > > > > > it, etc. It is about how devices fit together cohesively in
> > > > > > > the system, and how U-Boot operates.  
> > > > > >
> > > > > > 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.  
> 

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

[Simon Glass]

Hi Andre,

On Tue, 7 Nov 2023 at 04:27, Andre Przywara  wrote:
>
> On Tue, 7 Nov 2023 01:08:15 +
> Simon Glass  wrote:
>
> Hi Simon,
>
> > On Mon, 6 Nov 2023 at 21:55, Andre Przywara  wrote:
> > >
> > > On Mon, 6 Nov 2023 13:38:39 -0700
> > > Simon Glass  wrote:
> > >
> > > Hi Simon,
> > >
> > > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara  
> > > > wrote:
> > > > >
> > > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > > Simon Glass  wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara 
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > Simon Glass  wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > >  wrote:
> > > > > > > > >
> > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > Heinrich Schuchardt  
> > > > > > > > > > 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.przyw...@arm.com/
> > > > > > > > > >
> > > > > > > > > > And I see that you seem to use the same mechanism to probe 
> > > > > > > > > > and init the
> > > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature 
> > > > > > > > > > is not
> > > > > > > > > > implemented.
> > > > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > > > loaded (and
> > > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > > available.
> > > > > > > > > > That doesn't seem too much of a problem on the first 
> > > > > > > > > > glance, but it
> > > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > > devices
> > > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > > without
> > > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > > VExpress64# rng 0
> > > > > > > > > > No RNG device
> > > > > > > >
> > > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > > function
> > > > > > > > can return -ENODEV
> > > > > > > >
> > > > > > > > I see this in U-Boot:
> > > > > > > >
> > > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > > >
> > > > > > > > We should not use this.
> > > > > > >
> > > > > > > Agreed.
> > > > > > >
> > > > > > > > Use the devicetree.
> > > > > > >
> > > > > > > No, this is definitely not something for the DT, at least not on 
> > > > > > > ARM.
> > > > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > > > Similar to PCI and USB devices, which we don't probe via the DT 
> > > > > > > as well.
> > > > > > >
> > > > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > > > before, but
> > > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > > >
> > > > > > > I was wondering if it might be something for UCLASS_CPU, 
> > > > > > > something like
> > > > > > > a "CPU feature bus": to let devices register on one on the many 
> > > > > > > CPU
> > > > > > > features (instead of compatible strings), then only bind() those
> > > > > > > drivers it the respective bit is set.
> > > > > > >
> > > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > > ocean?
> > > > > > > As I don't know if we see many users apart from this.
> > > > > >
> > > > > > I have seen this so many times, where people want to avoid putting
> > > > > > things in the DT and then are surprised that everything is 
> > > > > > difficult,
> > > > > > broken and confusing. Why not just follow the rules? It is not just
> > > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > > together cohesively in the system, and how U-Boot operates.
> > > > >
> > > > > 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 

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

[Andre Przywara]

On Tue, 7 Nov 2023 01:08:15 +
Simon Glass  wrote:

Hi Simon,

> On Mon, 6 Nov 2023 at 21:55, Andre Przywara  wrote:
> >
> > On Mon, 6 Nov 2023 13:38:39 -0700
> > Simon Glass  wrote:
> >
> > Hi Simon,
> >  
> > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara  
> > > wrote:  
> > > >
> > > > On Sat, 4 Nov 2023 19:45:06 +
> > > > Simon Glass  wrote:
> > > >
> > > > Hi,
> > > >  
> > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  
> > > > > wrote:  
> > > > > >
> > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > Simon Glass  wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >  
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > >  wrote:  
> > > > > > > >
> > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > > > > >
> > > > > > > > > And I see that you seem to use the same mechanism to probe 
> > > > > > > > > and init the
> > > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is 
> > > > > > > > > not
> > > > > > > > > implemented.
> > > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > > loaded (and
> > > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > > available.
> > > > > > > > > That doesn't seem too much of a problem on the first glance, 
> > > > > > > > > but it
> > > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > > devices
> > > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > > without
> > > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > > VExpress64# rng 0
> > > > > > > > > No RNG device  
> > > > > > >
> > > > > > > Why do we get this? If the device is not there, the bind() 
> > > > > > > function
> > > > > > > can return -ENODEV
> > > > > > >
> > > > > > > I see this in U-Boot:
> > > > > > >
> > > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > > >
> > > > > > > We should not use this.  
> > > > > >
> > > > > > Agreed.
> > > > > >  
> > > > > > > Use the devicetree.  
> > > > > >
> > > > > > No, this is definitely not something for the DT, at least not on 
> > > > > > ARM.
> > > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > > Similar to PCI and USB devices, which we don't probe via the DT as 
> > > > > > well.
> > > > > >
> > > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > > before, but
> > > > > > it's the simplest solution and fits in nicely otherwise.
> > > > > >
> > > > > > I was wondering if it might be something for UCLASS_CPU, something 
> > > > > > like
> > > > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > > > features (instead of compatible strings), then only bind() those
> > > > > > drivers it the respective bit is set.
> > > > > >
> > > > > > Does that make sense? Would that be doable without boiling the 
> > > > > > ocean?
> > > > > > As I don't know if we see many users apart from this.  
> > > > >
> > > > > I have seen this so many times, where people want to avoid putting
> > > > > things in the DT and then are surprised that everything is difficult,
> > > > > broken and confusing. Why not just follow the rules? It is not just
> > > > > about whether we can avoid it, etc. It is about how devices fit
> > > > > together cohesively in the system, and how U-Boot operates.  
> > > >
> > > > 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.  

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

[Simon Glass]

Hi Tom,

On Mon, 6 Nov 2023 at 13:46, Tom Rini  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  wrote:
> > >
> > > On Sat, 4 Nov 2023 19:45:06 +
> > > Simon Glass  wrote:
> > >
> > > Hi,
> > >
> > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  
> > > > wrote:
> > > > >
> > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > Simon Glass  wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > >  wrote:
> > > > > > >
> > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > > > >
> > > > > > > > And I see that you seem to use the same mechanism to probe and 
> > > > > > > > init the
> > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > > > implemented.
> > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > loaded (and
> > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > available.
> > > > > > > > That doesn't seem too much of a problem on the first glance, 
> > > > > > > > but it
> > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > devices
> > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > without
> > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > VExpress64# rng 0
> > > > > > > > No RNG device
> > > > > >
> > > > > > Why do we get this? If the device is not there, the bind() function
> > > > > > can return -ENODEV
> > > > > >
> > > > > > I see this in U-Boot:
> > > > > >
> > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > >
> > > > > > We should not use this.
> > > > >
> > > > > Agreed.
> > > > >
> > > > > > Use the devicetree.
> > > > >
> > > > > No, this is definitely not something for the DT, at least not on ARM.
> > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > Similar to PCI and USB devices, which we don't probe via the DT as 
> > > > > well.
> > > > >
> > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > before, but
> > > > > it's the simplest solution and fits in nicely otherwise.
> > > > >
> > > > > I was wondering if it might be something for UCLASS_CPU, something 
> > > > > like
> > > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > > features (instead of compatible strings), then only bind() those
> > > > > drivers it the respective bit is set.
> > > > >
> > > > > Does that make sense? Would that be doable without boiling the ocean?
> > > > > As I don't know if we see many users apart from this.
> > > >
> > > > I have seen this so many times, where people want to avoid putting
> > > > things in the DT and then are surprised that everything is difficult,
> > > > broken and confusing. Why not just follow the rules? It is not just
> > > > about whether we can avoid it, etc. It is about how devices fit
> > > > together cohesively in the system, and how U-Boot operates.
> > >
> > > 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

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

[Simon Glass]

Hi Andre,

On Mon, 6 Nov 2023 at 21:55, Andre Przywara  wrote:
>
> On Mon, 6 Nov 2023 13:38:39 -0700
> Simon Glass  wrote:
>
> Hi Simon,
>
> > On Mon, 6 Nov 2023 at 10:26, Andre Przywara  wrote:
> > >
> > > On Sat, 4 Nov 2023 19:45:06 +
> > > Simon Glass  wrote:
> > >
> > > Hi,
> > >
> > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  
> > > > wrote:
> > > > >
> > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > Simon Glass  wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > >  wrote:
> > > > > > >
> > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > > > >
> > > > > > > > And I see that you seem to use the same mechanism to probe and 
> > > > > > > > init the
> > > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > > > implemented.
> > > > > > > > One downside of this approach is that the driver is always 
> > > > > > > > loaded (and
> > > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > > available.
> > > > > > > > That doesn't seem too much of a problem on the first glance, 
> > > > > > > > but it
> > > > > > > > occupies a device number, and any subsequent other DM_RNG 
> > > > > > > > devices
> > > > > > > > (like virtio-rng) typically get higher device numbers. So 
> > > > > > > > without
> > > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > > VExpress64# rng 0
> > > > > > > > No RNG device
> > > > > >
> > > > > > Why do we get this? If the device is not there, the bind() function
> > > > > > can return -ENODEV
> > > > > >
> > > > > > I see this in U-Boot:
> > > > > >
> > > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > > >
> > > > > > We should not use this.
> > > > >
> > > > > Agreed.
> > > > >
> > > > > > Use the devicetree.
> > > > >
> > > > > No, this is definitely not something for the DT, at least not on ARM.
> > > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > > Similar to PCI and USB devices, which we don't probe via the DT as 
> > > > > well.
> > > > >
> > > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > > before, but
> > > > > it's the simplest solution and fits in nicely otherwise.
> > > > >
> > > > > I was wondering if it might be something for UCLASS_CPU, something 
> > > > > like
> > > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > > features (instead of compatible strings), then only bind() those
> > > > > drivers it the respective bit is set.
> > > > >
> > > > > Does that make sense? Would that be doable without boiling the ocean?
> > > > > As I don't know if we see many users apart from this.
> > > >
> > > > I have seen this so many times, where people want to avoid putting
> > > > things in the DT and then are surprised that everything is difficult,
> > > > broken and confusing. Why not just follow the rules? It is not just
> > > > about whether we can avoid it, etc. It is about how devices fit
> > > > together cohesively in the system, and how U-Boot operates.
> > >
> > > 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 

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

[Andre Przywara]

On Mon, 6 Nov 2023 13:38:39 -0700
Simon Glass  wrote:

Hi Simon,

> On Mon, 6 Nov 2023 at 10:26, Andre Przywara  wrote:
> >
> > On Sat, 4 Nov 2023 19:45:06 +
> > Simon Glass  wrote:
> >
> > Hi,
> >  
> > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  
> > > wrote:  
> > > >
> > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > Simon Glass  wrote:
> > > >
> > > > Hi Simon,
> > > >  
> > > > > Hi Heinrich,
> > > > >
> > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > >  wrote:  
> > > > > >
> > > > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > > >
> > > > > > > And I see that you seem to use the same mechanism to probe and 
> > > > > > > init the
> > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > > implemented.
> > > > > > > One downside of this approach is that the driver is always loaded 
> > > > > > > (and
> > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > available.
> > > > > > > That doesn't seem too much of a problem on the first glance, but 
> > > > > > > it
> > > > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > VExpress64# rng 0
> > > > > > > No RNG device  
> > > > >
> > > > > Why do we get this? If the device is not there, the bind() function
> > > > > can return -ENODEV
> > > > >
> > > > > I see this in U-Boot:
> > > > >
> > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > >
> > > > > We should not use this.  
> > > >
> > > > Agreed.
> > > >  
> > > > > Use the devicetree.  
> > > >
> > > > No, this is definitely not something for the DT, at least not on ARM.
> > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > Similar to PCI and USB devices, which we don't probe via the DT as well.
> > > >
> > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > before, but
> > > > it's the simplest solution and fits in nicely otherwise.
> > > >
> > > > I was wondering if it might be something for UCLASS_CPU, something like
> > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > features (instead of compatible strings), then only bind() those
> > > > drivers it the respective bit is set.
> > > >
> > > > Does that make sense? Would that be doable without boiling the ocean?
> > > > As I don't know if we see many users apart from this.  
> > >
> > > I have seen this so many times, where people want to avoid putting
> > > things in the DT and then are surprised that everything is difficult,
> > > broken and confusing. Why not just follow the rules? It is not just
> > > about whether we can avoid it, etc. It is about how devices fit
> > > together cohesively in the system, and how U-Boot operates.  
> >
> > 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 

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

[Tom Rini]

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  wrote:
> >
> > On Sat, 4 Nov 2023 19:45:06 +
> > Simon Glass  wrote:
> >
> > Hi,
> >
> > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  
> > > wrote:
> > > >
> > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > Simon Glass  wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > > Hi Heinrich,
> > > > >
> > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > >  wrote:
> > > > > >
> > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > > >
> > > > > > > And I see that you seem to use the same mechanism to probe and 
> > > > > > > init the
> > > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > > implemented.
> > > > > > > One downside of this approach is that the driver is always loaded 
> > > > > > > (and
> > > > > > > visible in the DM tree), even with the feature not being 
> > > > > > > available.
> > > > > > > That doesn't seem too much of a problem on the first glance, but 
> > > > > > > it
> > > > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > > > the feature, but with virtio-rng, I get:
> > > > > > > VExpress64# rng 0
> > > > > > > No RNG device
> > > > >
> > > > > Why do we get this? If the device is not there, the bind() function
> > > > > can return -ENODEV
> > > > >
> > > > > I see this in U-Boot:
> > > > >
> > > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > > >
> > > > > We should not use this.
> > > >
> > > > Agreed.
> > > >
> > > > > Use the devicetree.
> > > >
> > > > No, this is definitely not something for the DT, at least not on ARM.
> > > > It's perfectly discoverable via the architected CPU ID registers.
> > > > Similar to PCI and USB devices, which we don't probe via the DT as well.
> > > >
> > > > It's arguably not proper "driver" material per se, as I've argued 
> > > > before, but
> > > > it's the simplest solution and fits in nicely otherwise.
> > > >
> > > > I was wondering if it might be something for UCLASS_CPU, something like
> > > > a "CPU feature bus": to let devices register on one on the many CPU
> > > > features (instead of compatible strings), then only bind() those
> > > > drivers it the respective bit is set.
> > > >
> > > > Does that make sense? Would that be doable without boiling the ocean?
> > > > As I don't know if we see many users apart from this.
> > >
> > > I have seen this so many times, where people want to avoid putting
> > > things in the DT and then are surprised that everything is difficult,
> > > broken and confusing. Why not just follow the rules? It is not just
> > > about whether we can avoid it, etc. It is about how devices fit
> > > together cohesively in the system, and how U-Boot operates.
> >
> > 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Simon Glass]

Hi Andre,

On Mon, 6 Nov 2023 at 10:26, Andre Przywara  wrote:
>
> On Sat, 4 Nov 2023 19:45:06 +
> Simon Glass  wrote:
>
> Hi,
>
> > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  wrote:
> > >
> > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > Simon Glass  wrote:
> > >
> > > Hi Simon,
> > >
> > > > Hi Heinrich,
> > > >
> > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > >  wrote:
> > > > >
> > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > >
> > > > > > And I see that you seem to use the same mechanism to probe and init 
> > > > > > the
> > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > implemented.
> > > > > > One downside of this approach is that the driver is always loaded 
> > > > > > (and
> > > > > > visible in the DM tree), even with the feature not being available.
> > > > > > That doesn't seem too much of a problem on the first glance, but it
> > > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > > the feature, but with virtio-rng, I get:
> > > > > > VExpress64# rng 0
> > > > > > No RNG device
> > > >
> > > > Why do we get this? If the device is not there, the bind() function
> > > > can return -ENODEV
> > > >
> > > > I see this in U-Boot:
> > > >
> > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > >
> > > > We should not use this.
> > >
> > > Agreed.
> > >
> > > > Use the devicetree.
> > >
> > > No, this is definitely not something for the DT, at least not on ARM.
> > > It's perfectly discoverable via the architected CPU ID registers.
> > > Similar to PCI and USB devices, which we don't probe via the DT as well.
> > >
> > > It's arguably not proper "driver" material per se, as I've argued before, 
> > > but
> > > it's the simplest solution and fits in nicely otherwise.
> > >
> > > I was wondering if it might be something for UCLASS_CPU, something like
> > > a "CPU feature bus": to let devices register on one on the many CPU
> > > features (instead of compatible strings), then only bind() those
> > > drivers it the respective bit is set.
> > >
> > > Does that make sense? Would that be doable without boiling the ocean?
> > > As I don't know if we see many users apart from this.
> >
> > I have seen this so many times, where people want to avoid putting
> > things in the DT and then are surprised that everything is difficult,
> > broken and confusing. Why not just follow the rules? It is not just
> > about whether we can avoid it, etc. It is about how devices fit
> > together cohesively in the system, and how U-Boot operates.
>
> 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 

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

[Tom Rini]

On Mon, Nov 06, 2023 at 05:26:01PM +, Andre Przywara wrote:
> On Sat, 4 Nov 2023 19:45:06 +
> Simon Glass  wrote:
> 
> Hi,
> 
> > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  wrote:
> > >
> > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > Simon Glass  wrote:
> > >
> > > Hi Simon,
> > >  
> > > > Hi Heinrich,
> > > >
> > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > >  wrote:  
> > > > >
> > > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > > >
> > > > > > And I see that you seem to use the same mechanism to probe and init 
> > > > > > the
> > > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > > implemented.
> > > > > > One downside of this approach is that the driver is always loaded 
> > > > > > (and
> > > > > > visible in the DM tree), even with the feature not being available.
> > > > > > That doesn't seem too much of a problem on the first glance, but it
> > > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > > the feature, but with virtio-rng, I get:
> > > > > > VExpress64# rng 0
> > > > > > No RNG device  
> > > >
> > > > Why do we get this? If the device is not there, the bind() function
> > > > can return -ENODEV
> > > >
> > > > I see this in U-Boot:
> > > >
> > > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > >
> > > > We should not use this.  
> > >
> > > Agreed.
> > >  
> > > > Use the devicetree.  
> > >
> > > No, this is definitely not something for the DT, at least not on ARM.
> > > It's perfectly discoverable via the architected CPU ID registers.
> > > Similar to PCI and USB devices, which we don't probe via the DT as well.
> > >
> > > It's arguably not proper "driver" material per se, as I've argued before, 
> > > but
> > > it's the simplest solution and fits in nicely otherwise.
> > >
> > > I was wondering if it might be something for UCLASS_CPU, something like
> > > a "CPU feature bus": to let devices register on one on the many CPU
> > > features (instead of compatible strings), then only bind() those
> > > drivers it the respective bit is set.
> > >
> > > Does that make sense? Would that be doable without boiling the ocean?
> > > As I don't know if we see many users apart from this.  
> > 
> > I have seen this so many times, where people want to avoid putting
> > things in the DT and then are surprised that everything is difficult,
> > broken and confusing. Why not just follow the rules? It is not just
> > about whether we can avoid it, etc. It is about how devices fit
> > together cohesively in the system, and how U-Boot operates.
> 
> 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 

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

[Andre Przywara]

On Mon, 6 Nov 2023 11:46:27 -0500
Tom Rini  wrote:

Hi Tom,

> On Sat, Nov 04, 2023 at 05:12:12PM +, Andre Przywara wrote:
> > On Fri, 3 Nov 2023 13:38:58 -0600
> > Simon Glass  wrote:
> > 
> > Hi Simon,
> >   
> > > Hi Heinrich,
> > > 
> > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > >  wrote:  
> > > >
> > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > >
> > > > > And I see that you seem to use the same mechanism to probe and init 
> > > > > the
> > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > implemented.
> > > > > One downside of this approach is that the driver is always loaded (and
> > > > > visible in the DM tree), even with the feature not being available.
> > > > > That doesn't seem too much of a problem on the first glance, but it
> > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > the feature, but with virtio-rng, I get:
> > > > > VExpress64# rng 0
> > > > > No RNG device
> > > 
> > > Why do we get this? If the device is not there, the bind() function
> > > can return -ENODEV
> > > 
> > > I see this in U-Boot:
> > > 
> > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > > 
> > > We should not use this.  
> > 
> > Agreed.
> >   
> > > Use the devicetree.  
> > 
> > No, this is definitely not something for the DT, at least not on ARM.
> > It's perfectly discoverable via the architected CPU ID registers.
> > Similar to PCI and USB devices, which we don't probe via the DT as well.
> > 
> > It's arguably not proper "driver" material per se, as I've argued before, 
> > but
> > it's the simplest solution and fits in nicely otherwise.
> > 
> > I was wondering if it might be something for UCLASS_CPU, something like
> > a "CPU feature bus": to let devices register on one on the many CPU
> > features (instead of compatible strings), then only bind() those
> > drivers it the respective bit is set.
> > 
> > Does that make sense? Would that be doable without boiling the ocean?
> > As I don't know if we see many users apart from this.  
> 
> I 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.

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

[Andre Przywara]

On Sat, 4 Nov 2023 19:45:06 +
Simon Glass  wrote:

Hi,

> On Sat, 4 Nov 2023 at 17:13, Andre Przywara  wrote:
> >
> > On Fri, 3 Nov 2023 13:38:58 -0600
> > Simon Glass  wrote:
> >
> > Hi Simon,
> >  
> > > Hi Heinrich,
> > >
> > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > >  wrote:  
> > > >
> > > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > >
> > > > > And I see that you seem to use the same mechanism to probe and init 
> > > > > the
> > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > implemented.
> > > > > One downside of this approach is that the driver is always loaded (and
> > > > > visible in the DM tree), even with the feature not being available.
> > > > > That doesn't seem too much of a problem on the first glance, but it
> > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > the feature, but with virtio-rng, I get:
> > > > > VExpress64# rng 0
> > > > > No RNG device  
> > >
> > > Why do we get this? If the device is not there, the bind() function
> > > can return -ENODEV
> > >
> > > I see this in U-Boot:
> > >
> > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > >
> > > We should not use this.  
> >
> > Agreed.
> >  
> > > Use the devicetree.  
> >
> > No, this is definitely not something for the DT, at least not on ARM.
> > It's perfectly discoverable via the architected CPU ID registers.
> > Similar to PCI and USB devices, which we don't probe via the DT as well.
> >
> > It's arguably not proper "driver" material per se, as I've argued before, 
> > but
> > it's the simplest solution and fits in nicely otherwise.
> >
> > I was wondering if it might be something for UCLASS_CPU, something like
> > a "CPU feature bus": to let devices register on one on the many CPU
> > features (instead of compatible strings), then only bind() those
> > drivers it the respective bit is set.
> >
> > Does that make sense? Would that be doable without boiling the ocean?
> > As I don't know if we see many users apart from this.  
> 
> I have seen this so many times, where people want to avoid putting
> things in the DT and then are surprised that everything is difficult,
> broken and confusing. Why not just follow the rules? It is not just
> about whether we can avoid it, etc. It is about how devices fit
> together cohesively in the system, and how U-Boot operates.

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

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

[Simon Glass]

Hi Tom,

On Mon, 6 Nov 2023 at 09:46, Tom Rini  wrote:
>
> On Sat, Nov 04, 2023 at 05:12:12PM +, Andre Przywara wrote:
> > On Fri, 3 Nov 2023 13:38:58 -0600
> > Simon Glass  wrote:
> >
> > Hi Simon,
> >
> > > Hi Heinrich,
> > >
> > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > >  wrote:
> > > >
> > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > > >
> > > > > And I see that you seem to use the same mechanism to probe and init 
> > > > > the
> > > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > > implemented.
> > > > > One downside of this approach is that the driver is always loaded (and
> > > > > visible in the DM tree), even with the feature not being available.
> > > > > That doesn't seem too much of a problem on the first glance, but it
> > > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > > (like virtio-rng) typically get higher device numbers. So without
> > > > > the feature, but with virtio-rng, I get:
> > > > > VExpress64# rng 0
> > > > > No RNG device
> > >
> > > Why do we get this? If the device is not there, the bind() function
> > > can return -ENODEV
> > >
> > > I see this in U-Boot:
> > >
> > > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > >
> > > We should not use this.
> >
> > Agreed.
> >
> > > Use the devicetree.
> >
> > No, this is definitely not something for the DT, at least not on ARM.
> > It's perfectly discoverable via the architected CPU ID registers.
> > Similar to PCI and USB devices, which we don't probe via the DT as well.
> >
> > It's arguably not proper "driver" material per se, as I've argued before, 
> > but
> > it's the simplest solution and fits in nicely otherwise.
> >
> > I was wondering if it might be something for UCLASS_CPU, something like
> > a "CPU feature bus": to let devices register on one on the many CPU
> > features (instead of compatible strings), then only bind() those
> > drivers it the respective bit is set.
> >
> > Does that make sense? Would that be doable without boiling the ocean?
> > As I don't know if we see many users apart from this.
>
> I 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.

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

[Tom Rini]

On Sat, Nov 04, 2023 at 05:12:12PM +, Andre Przywara wrote:
> On Fri, 3 Nov 2023 13:38:58 -0600
> Simon Glass  wrote:
> 
> Hi Simon,
> 
> > Hi Heinrich,
> > 
> > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> >  wrote:
> > >
> > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > >
> > > > And I see that you seem to use the same mechanism to probe and init the
> > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > implemented.
> > > > One downside of this approach is that the driver is always loaded (and
> > > > visible in the DM tree), even with the feature not being available.
> > > > That doesn't seem too much of a problem on the first glance, but it
> > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > (like virtio-rng) typically get higher device numbers. So without
> > > > the feature, but with virtio-rng, I get:
> > > > VExpress64# rng 0
> > > > No RNG device  
> > 
> > Why do we get this? If the device is not there, the bind() function
> > can return -ENODEV
> > 
> > I see this in U-Boot:
> > 
> > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> > 
> > We should not use this.
> 
> Agreed.
> 
> > Use the devicetree.
> 
> No, this is definitely not something for the DT, at least not on ARM.
> It's perfectly discoverable via the architected CPU ID registers.
> Similar to PCI and USB devices, which we don't probe via the DT as well.
> 
> It's arguably not proper "driver" material per se, as I've argued before, but
> it's the simplest solution and fits in nicely otherwise.
> 
> I was wondering if it might be something for UCLASS_CPU, something like
> a "CPU feature bus": to let devices register on one on the many CPU
> features (instead of compatible strings), then only bind() those
> drivers it the respective bit is set.
> 
> Does that make sense? Would that be doable without boiling the ocean?
> As I don't know if we see many users apart from this.

I 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.

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


signature.asc
Description: PGP signature

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

[Simon Glass]

Hi Heinrich,

On Sat, 4 Nov 2023 at 20:36, Heinrich Schuchardt
 wrote:
>
> On 11/4/23 21:45, Simon Glass wrote:
> > Hi Andre,
> >
> > On Sat, 4 Nov 2023 at 17:13, Andre Przywara  wrote:
> >>
> >> On Fri, 3 Nov 2023 13:38:58 -0600
> >> Simon Glass  wrote:
> >>
> >> Hi Simon,
> >>
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> >>>  wrote:
> 
>  On 11/1/23 19:05, Andre Przywara wrote:
> > On Tue, 31 Oct 2023 14:55:50 +0200
> > Heinrich Schuchardt  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.przyw...@arm.com/
> >
> > And I see that you seem to use the same mechanism to probe and init the
> > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > implemented.
> > One downside of this approach is that the driver is always loaded (and
> > visible in the DM tree), even with the feature not being available.
> > That doesn't seem too much of a problem on the first glance, but it
> > occupies a device number, and any subsequent other DM_RNG devices
> > (like virtio-rng) typically get higher device numbers. So without
> > the feature, but with virtio-rng, I get:
> > VExpress64# rng 0
> > No RNG device
> >>>
> >>> Why do we get this? If the device is not there, the bind() function
> >>> can return -ENODEV
> >>>
> >>> I see this in U-Boot:
> >>>
> >>> U_BOOT_DRVINFO(cpu_arm_rndr) = {
> >>>
> >>> We should not use this.
> >>
> >> Agreed.
> >>
> >>> Use the devicetree.
> >>
> >> No, this is definitely not something for the DT, at least not on ARM.
> >> It's perfectly discoverable via the architected CPU ID registers.
> >> Similar to PCI and USB devices, which we don't probe via the DT as well.
> >>
> >> It's arguably not proper "driver" material per se, as I've argued before, 
> >> but
> >> it's the simplest solution and fits in nicely otherwise.
> >>
> >> I was wondering if it might be something for UCLASS_CPU, something like
> >> a "CPU feature bus": to let devices register on one on the many CPU
> >> features (instead of compatible strings), then only bind() those
> >> drivers it the respective bit is set.
> >>
> >> Does that make sense? Would that be doable without boiling the ocean?
> >> As I don't know if we see many users apart from this.
> >
> > I have seen this so many times, where people want to avoid putting
> > things in the DT and then are surprised that everything is difficult,
> > broken and confusing. Why not just follow the rules? It is not just
> > about whether we can avoid it, etc. It is about how devices fit
> > together cohesively in the system, and how U-Boot operates.
>
> The Zkr extension isn't a device. It is an instruction set architecture
> extension. It is represented by the ISA extension strings of the
> respective harts in the device-tree.
>
> If the seed register of the hart can be accessed by U-Boot, is
> determined by bit sseed in the mseccfg register.
>
> I don't expect the encoding of RISC-V ISA extensions in device-trees to
> be changed.

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

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

Nothing is changed until someone sends a patch.

Regards,
Simon



>
> Best regards
>
> Heinrich
>
> >
> >>
> > VExpress64# rng 1
> > : 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
> 
>  We have weak function platform_get_rng_device() which should be moved to
>  drivers/rng/rng-uclass.c.
> 
>  We could add a function to drivers/core/uclass.c to retrieve the first
>  successfully probed device. Another approach would be to implement
>  uclass_driver.post_probe() in the RNG uclass to take note of the first
>  successfully probed device.
> 

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

[Heinrich Schuchardt]

On 11/4/23 21:45, Simon Glass wrote:

Hi Andre,

On Sat, 4 Nov 2023 at 17:13, Andre Przywara  wrote:


On Fri, 3 Nov 2023 13:38:58 -0600
Simon Glass  wrote:

Hi Simon,


Hi Heinrich,

On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
 wrote:


On 11/1/23 19:05, Andre Przywara wrote:

On Tue, 31 Oct 2023 14:55:50 +0200
Heinrich Schuchardt  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.przyw...@arm.com/

And I see that you seem to use the same mechanism to probe and init the
driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
implemented.
One downside of this approach is that the driver is always loaded (and
visible in the DM tree), even with the feature not being available.
That doesn't seem too much of a problem on the first glance, but it
occupies a device number, and any subsequent other DM_RNG devices
(like virtio-rng) typically get higher device numbers. So without
the feature, but with virtio-rng, I get:
VExpress64# rng 0
No RNG device


Why do we get this? If the device is not there, the bind() function
can return -ENODEV

I see this in U-Boot:

U_BOOT_DRVINFO(cpu_arm_rndr) = {

We should not use this.


Agreed.


Use the devicetree.


No, this is definitely not something for the DT, at least not on ARM.
It's perfectly discoverable via the architected CPU ID registers.
Similar to PCI and USB devices, which we don't probe via the DT as well.

It's arguably not proper "driver" material per se, as I've argued before, but
it's the simplest solution and fits in nicely otherwise.

I was wondering if it might be something for UCLASS_CPU, something like
a "CPU feature bus": to let devices register on one on the many CPU
features (instead of compatible strings), then only bind() those
drivers it the respective bit is set.

Does that make sense? Would that be doable without boiling the ocean?
As I don't know if we see many users apart from this.


I have seen this so many times, where people want to avoid putting
things in the DT and then are surprised that everything is difficult,
broken and confusing. Why not just follow the rules? It is not just
about whether we can avoid it, etc. It is about how devices fit
together cohesively in the system, and how U-Boot operates.


The Zkr extension isn't a device. It is an instruction set architecture 
extension. It is represented by the ISA extension strings of the 
respective harts in the device-tree.


If the seed register of the hart can be accessed by U-Boot, is 
determined by bit sseed in the mseccfg register.


I don't expect the encoding of RISC-V ISA extensions in device-trees to 
be changed.


Best regards

Heinrich






VExpress64# rng 1
: 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95

We have weak function platform_get_rng_device() which should be moved to
drivers/rng/rng-uclass.c.

We could add a function to drivers/core/uclass.c to retrieve the first
successfully probed device. Another approach would be to implement
uclass_driver.post_probe() in the RNG uclass to take note of the first
successfully probed device.

@Simon:
What would make most sense from a DM design standpoint?


I am sure I provided feedback on this at the time, but I don't
remember. OK I just found it here [1]. So the problem is entirely
because my feedback was not addressed. Please just address it and
avoid this sort of mess.


Yeah, Tom just merged it, but that's not Heinrich's fault ;-)


So arm_rndr should have a devicetree compatible string and be bound
like anything else. If for some reason the device doesn't exist in the
hardware, it can return -ENODEV from its bind() method.

If you want to control which RNG device is used for booting, you could
add a property to /bootstd with a phandle to the device. We are trying
to provide a standard approach to booting in U-Boot, used by all
methods. Doing one-off things for particular cases is best avoided.


Picking the first usable device doesn't sound much like a one-off to me.
After all the caller (be it UEFI or the rng command) later detect that
this 

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

[Simon Glass]

Hi Andre,

On Sat, 4 Nov 2023 at 17:13, Andre Przywara  wrote:
>
> On Fri, 3 Nov 2023 13:38:58 -0600
> Simon Glass  wrote:
>
> Hi Simon,
>
> > Hi Heinrich,
> >
> > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> >  wrote:
> > >
> > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > Heinrich Schuchardt  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.przyw...@arm.com/
> > > >
> > > > And I see that you seem to use the same mechanism to probe and init the
> > > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > > implemented.
> > > > One downside of this approach is that the driver is always loaded (and
> > > > visible in the DM tree), even with the feature not being available.
> > > > That doesn't seem too much of a problem on the first glance, but it
> > > > occupies a device number, and any subsequent other DM_RNG devices
> > > > (like virtio-rng) typically get higher device numbers. So without
> > > > the feature, but with virtio-rng, I get:
> > > > VExpress64# rng 0
> > > > No RNG device
> >
> > Why do we get this? If the device is not there, the bind() function
> > can return -ENODEV
> >
> > I see this in U-Boot:
> >
> > U_BOOT_DRVINFO(cpu_arm_rndr) = {
> >
> > We should not use this.
>
> Agreed.
>
> > Use the devicetree.
>
> No, this is definitely not something for the DT, at least not on ARM.
> It's perfectly discoverable via the architected CPU ID registers.
> Similar to PCI and USB devices, which we don't probe via the DT as well.
>
> It's arguably not proper "driver" material per se, as I've argued before, but
> it's the simplest solution and fits in nicely otherwise.
>
> I was wondering if it might be something for UCLASS_CPU, something like
> a "CPU feature bus": to let devices register on one on the many CPU
> features (instead of compatible strings), then only bind() those
> drivers it the respective bit is set.
>
> Does that make sense? Would that be doable without boiling the ocean?
> As I don't know if we see many users apart from this.

I have seen this so many times, where people want to avoid putting
things in the DT and then are surprised that everything is difficult,
broken and confusing. Why not just follow the rules? It is not just
about whether we can avoid it, etc. It is about how devices fit
together cohesively in the system, and how U-Boot operates.

>
> > > > VExpress64# rng 1
> > > > : 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
> > >
> > > We have weak function platform_get_rng_device() which should be moved to
> > > drivers/rng/rng-uclass.c.
> > >
> > > We could add a function to drivers/core/uclass.c to retrieve the first
> > > successfully probed device. Another approach would be to implement
> > > uclass_driver.post_probe() in the RNG uclass to take note of the first
> > > successfully probed device.
> > >
> > > @Simon:
> > > What would make most sense from a DM design standpoint?
> >
> > I am sure I provided feedback on this at the time, but I don't
> > remember. OK I just found it here [1]. So the problem is entirely
> > because my feedback was not addressed. Please just address it and
> > avoid this sort of mess.
>
> Yeah, Tom just merged it, but that's not Heinrich's fault ;-)
>
> > So arm_rndr should have a devicetree compatible string and be bound
> > like anything else. If for some reason the device doesn't exist in the
> > hardware, it can return -ENODEV from its bind() method.
> >
> > If you want to control which RNG device is used for booting, you could
> > add a property to /bootstd with a phandle to the device. We are trying
> > to provide a standard approach to booting in U-Boot, used by all
> > methods. Doing one-off things for particular cases is best avoided.
>
> Picking the first usable device doesn't sound much like a one-off to me.
> After all 

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

[Andre Przywara]

On Fri, 3 Nov 2023 13:38:58 -0600
Simon Glass  wrote:

Hi Simon,

> Hi Heinrich,
> 
> On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
>  wrote:
> >
> > On 11/1/23 19:05, Andre Przywara wrote:  
> > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > Heinrich Schuchardt  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.przyw...@arm.com/
> > >
> > > And I see that you seem to use the same mechanism to probe and init the
> > > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > > implemented.
> > > One downside of this approach is that the driver is always loaded (and
> > > visible in the DM tree), even with the feature not being available.
> > > That doesn't seem too much of a problem on the first glance, but it
> > > occupies a device number, and any subsequent other DM_RNG devices
> > > (like virtio-rng) typically get higher device numbers. So without
> > > the feature, but with virtio-rng, I get:
> > > VExpress64# rng 0
> > > No RNG device  
> 
> Why do we get this? If the device is not there, the bind() function
> can return -ENODEV
> 
> I see this in U-Boot:
> 
> U_BOOT_DRVINFO(cpu_arm_rndr) = {
> 
> We should not use this.

Agreed.

> Use the devicetree.

No, this is definitely not something for the DT, at least not on ARM.
It's perfectly discoverable via the architected CPU ID registers.
Similar to PCI and USB devices, which we don't probe via the DT as well.

It's arguably not proper "driver" material per se, as I've argued before, but
it's the simplest solution and fits in nicely otherwise.

I was wondering if it might be something for UCLASS_CPU, something like
a "CPU feature bus": to let devices register on one on the many CPU
features (instead of compatible strings), then only bind() those
drivers it the respective bit is set.

Does that make sense? Would that be doable without boiling the ocean?
As I don't know if we see many users apart from this.

> > > VExpress64# rng 1
> > > : 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
> >
> > We have weak function platform_get_rng_device() which should be moved to
> > drivers/rng/rng-uclass.c.
> >
> > We could add a function to drivers/core/uclass.c to retrieve the first
> > successfully probed device. Another approach would be to implement
> > uclass_driver.post_probe() in the RNG uclass to take note of the first
> > successfully probed device.
> >
> > @Simon:
> > What would make most sense from a DM design standpoint?  
> 
> I am sure I provided feedback on this at the time, but I don't
> remember. OK I just found it here [1]. So the problem is entirely
> because my feedback was not addressed. Please just address it and
> avoid this sort of mess.

Yeah, Tom just merged it, but that's not Heinrich's fault ;-)

> So arm_rndr should have a devicetree compatible string and be bound
> like anything else. If for some reason the device doesn't exist in the
> hardware, it can return -ENODEV from its bind() method.
> 
> If you want to control which RNG device is used for booting, you could
> add a property to /bootstd with a phandle to the device. We are trying
> to provide a standard approach to booting in U-Boot, used by all
> methods. Doing one-off things for particular cases is best avoided.

Picking the first usable device doesn't sound much like a one-off to me.
After all the caller (be it UEFI or the rng command) later detect that
this is not usable. So there might be some merit to cover this more
automatically, either in the caller, or by providing a suitable wrapper
function?

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
> 

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

[Simon Glass]

Hi Heinrich,

On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
 wrote:
>
> On 11/1/23 19:05, Andre Przywara wrote:
> > On Tue, 31 Oct 2023 14:55:50 +0200
> > Heinrich Schuchardt  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.przyw...@arm.com/
> >
> > And I see that you seem to use the same mechanism to probe and init the
> > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > implemented.
> > One downside of this approach is that the driver is always loaded (and
> > visible in the DM tree), even with the feature not being available.
> > That doesn't seem too much of a problem on the first glance, but it
> > occupies a device number, and any subsequent other DM_RNG devices
> > (like virtio-rng) typically get higher device numbers. So without
> > the feature, but with virtio-rng, I get:
> > VExpress64# rng 0
> > No RNG device

Why do we get this? If the device is not there, the bind() function
can return -ENODEV

I see this in U-Boot:

U_BOOT_DRVINFO(cpu_arm_rndr) = {

We should not use this. Use the devicetree.

> > VExpress64# rng 1
> > : 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
>
> We have weak function platform_get_rng_device() which should be moved to
> drivers/rng/rng-uclass.c.
>
> We could add a function to drivers/core/uclass.c to retrieve the first
> successfully probed device. Another approach would be to implement
> uclass_driver.post_probe() in the RNG uclass to take note of the first
> successfully probed device.
>
> @Simon:
> What would make most sense from a DM design standpoint?

I am sure I provided feedback on this at the time, but I don't
remember. OK I just found it here [1]. So the problem is entirely
because my feedback was not addressed. Please just address it and
avoid this sort of mess.

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-sa...@rivosinc.com/
> >> [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
> >>  
> >> https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schucha...@canonical.com/
> >> [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
> >>  
> >> 

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

[Heinrich Schuchardt]

On 11/1/23 19:05, Andre Przywara wrote:

On Tue, 31 Oct 2023 14:55:50 +0200
Heinrich Schuchardt  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.przyw...@arm.com/

And I see that you seem to use the same mechanism to probe and init the
driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
implemented.
One downside of this approach is that the driver is always loaded (and
visible in the DM tree), even with the feature not being available.
That doesn't seem too much of a problem on the first glance, but it
occupies a device number, and any subsequent other DM_RNG devices
(like virtio-rng) typically get higher device numbers. So without
the feature, but with virtio-rng, I get:
VExpress64# rng 0
No RNG device
VExpress64# rng 1
: 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-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95

We have weak function platform_get_rng_device() which should be moved to 
drivers/rng/rng-uclass.c.


We could add a function to drivers/core/uclass.c to retrieve the first 
successfully probed device. Another approach would be to implement 
uclass_driver.post_probe() in the RNG uclass to take note of the first 
successfully probed device.


@Simon:
What would make most sense from a DM design standpoint?

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-sa...@rivosinc.com/
[2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
 
https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schucha...@canonical.com/
[3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
 
https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sa...@rivosinc.com/

v3:
Add API documentation.
v2:
Catch exception if mseccfg.sseed=0.

Heinrich Schuchardt (2):
   riscv: allow resume after exception
   rng: Provide a RNG based on the RISC-V Zkr ISA extension

  arch/riscv/lib/interrupts.c |  13 
  doc/api/index.rst   |   1 +
  drivers/rng/Kconfig |   8 +++
  drivers/rng/Makefile|   1 +
  drivers/rng/riscv_zkr_rng.c | 116 
  include/interrupt.h |  45 ++
  6 files changed, 184 insertions(+)
  create mode 100644 drivers/rng/riscv_zkr_rng.c
  create mode 100644 include/interrupt.h

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

[Sean Anderson]

On 11/1/23 13:49, Andre Przywara wrote:

On Wed, 1 Nov 2023 13:16:24 -0400
Sean Anderson  wrote:

Hi Sean,


On 11/1/23 13:05, Andre Przywara wrote:

On Tue, 31 Oct 2023 14:55:50 +0200
Heinrich Schuchardt  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.przyw...@arm.com/

And I see that you seem to use the same mechanism to probe and init the
driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
implemented.
One downside of this approach is that the driver is always loaded (and
visible in the DM tree), even with the feature not being available.
That doesn't seem too much of a problem on the first glance, but it
occupies a device number, and any subsequent other DM_RNG devices
(like virtio-rng) typically get higher device numbers. So without
the feature, but with virtio-rng, I get:
VExpress64# rng 0
No RNG device
VExpress64# rng 1
: 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 fef3d744 (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-sa...@rivosinc.com/
[2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
  
https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schucha...@canonical.com/
[3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
  
https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sa...@rivosinc.com/

v3:
Add API documentation.
v2:
Catch exception if mseccfg.sseed=0.

Heinrich Schuchardt (2):
riscv: allow resume after exception
rng: Provide a RNG based on the RISC-V Zkr ISA extension

   arch/riscv/lib/interrupts.c |  13 
   doc/api/index.rst   |   1 +
   drivers/rng/Kconfig |   8 +++
   drivers/rng/Makefile|   1 +
   drivers/rng/riscv_zkr_rng.c | 116 
   include/interrupt.h |  45 ++
   6 files changed, 184 insertions(+)
   create mode 100644 drivers/rng/riscv_zkr_rng.c
   create mode 100644 include/interrupt.h
  
   

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

[Andre Przywara]

On Wed, 1 Nov 2023 13:16:24 -0400
Sean Anderson  wrote:

Hi Sean,

> On 11/1/23 13:05, Andre Przywara wrote:
> > On Tue, 31 Oct 2023 14:55:50 +0200
> > Heinrich Schuchardt  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.przyw...@arm.com/
> > 
> > And I see that you seem to use the same mechanism to probe and init the
> > driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
> > implemented.
> > One downside of this approach is that the driver is always loaded (and
> > visible in the DM tree), even with the feature not being available.
> > That doesn't seem too much of a problem on the first glance, but it
> > occupies a device number, and any subsequent other DM_RNG devices
> > (like virtio-rng) typically get higher device numbers. So without
> > the feature, but with virtio-rng, I get:
> > VExpress64# rng 0
> > No RNG device
> > VExpress64# rng 1
> > : 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 fef3d744 (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-sa...@rivosinc.com/
> >> [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
> >>  
> >> https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schucha...@canonical.com/
> >> [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
> >>  
> >> https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sa...@rivosinc.com/
> >>
> >> v3:
> >>Add API documentation.
> >> v2:
> >>Catch exception if mseccfg.sseed=0.
> >>
> >> Heinrich Schuchardt (2):
> >>riscv: allow resume after exception
> >>rng: Provide a RNG based on the RISC-V Zkr ISA extension
> >>
> >>   arch/riscv/lib/interrupts.c |  13 
> >>   doc/api/index.rst   |   1 +
> >>   drivers/rng/Kconfig |   8 +++
> >>   drivers/rng/Makefile|   1 +
> >>   drivers/rng/riscv_zkr_rng.c | 116 
> >>   include/interrupt.h |  45 ++
> >>   6 files changed, 184 insertions(+)
> >>   create mode 100644 drivers/rng/riscv_zkr_rng.c
> >>   create mode 100644 include/interrupt.h
> >>  
> >   
> 

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

[Sean Anderson]

On 11/1/23 13:05, Andre Przywara wrote:

On Tue, 31 Oct 2023 14:55:50 +0200
Heinrich Schuchardt  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.przyw...@arm.com/

And I see that you seem to use the same mechanism to probe and init the
driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
implemented.
One downside of this approach is that the driver is always loaded (and
visible in the DM tree), even with the feature not being available.
That doesn't seem too much of a problem on the first glance, but it
occupies a device number, and any subsequent other DM_RNG devices
(like virtio-rng) typically get higher device numbers. So without
the feature, but with virtio-rng, I get:
VExpress64# rng 0
No RNG device
VExpress64# rng 1
: 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-sa...@rivosinc.com/
[2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
 
https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schucha...@canonical.com/
[3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
 
https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sa...@rivosinc.com/

v3:
Add API documentation.
v2:
Catch exception if mseccfg.sseed=0.

Heinrich Schuchardt (2):
   riscv: allow resume after exception
   rng: Provide a RNG based on the RISC-V Zkr ISA extension

  arch/riscv/lib/interrupts.c |  13 
  doc/api/index.rst   |   1 +
  drivers/rng/Kconfig |   8 +++
  drivers/rng/Makefile|   1 +
  drivers/rng/riscv_zkr_rng.c | 116 
  include/interrupt.h |  45 ++
  6 files changed, 184 insertions(+)
  create mode 100644 drivers/rng/riscv_zkr_rng.c
  create mode 100644 include/interrupt.h

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

[Andre Przywara]

On Tue, 31 Oct 2023 14:55:50 +0200
Heinrich Schuchardt  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.przyw...@arm.com/

And I see that you seem to use the same mechanism to probe and init the
driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
implemented.
One downside of this approach is that the driver is always loaded (and
visible in the DM tree), even with the feature not being available.
That doesn't seem too much of a problem on the first glance, but it
occupies a device number, and any subsequent other DM_RNG devices
(like virtio-rng) typically get higher device numbers. So without
the feature, but with virtio-rng, I get:
VExpress64# rng 0
No RNG device
VExpress64# rng 1
: 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-sa...@rivosinc.com/
> [2] [PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]
> 
> https://lore.kernel.org/qemu-devel/20231030102105.19501-1-heinrich.schucha...@canonical.com/
> [3] [PATCH v4 4/4] RISC-V: Implement archrandom when Zkr is available
> 
> https://lore.kernel.org/linux-riscv/20230712084134.1648008-5-sa...@rivosinc.com/
> 
> v3:
>   Add API documentation.
> v2:
>   Catch exception if mseccfg.sseed=0.
> 
> Heinrich Schuchardt (2):
>   riscv: allow resume after exception
>   rng: Provide a RNG based on the RISC-V Zkr ISA extension
> 
>  arch/riscv/lib/interrupts.c |  13 
>  doc/api/index.rst   |   1 +
>  drivers/rng/Kconfig |   8 +++
>  drivers/rng/Makefile|   1 +
>  drivers/rng/riscv_zkr_rng.c | 116 
>  include/interrupt.h |  45 ++
>  6 files changed, 184 insertions(+)
>  create mode 100644 drivers/rng/riscv_zkr_rng.c
>  create mode 100644 include/interrupt.h
>