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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7975 bytes --]

On Tue, Nov 07, 2023 at 03:52:36PM -0600, Rob Herring wrote:
> On Tue, Nov 7, 2023 at 1:30 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Nov 07, 2023 at 01:10:44AM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 6 Nov 2023 at 13:46, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Nov 06, 2023 at 01:38:39PM -0700, Simon Glass wrote:
> > > > > Hi Andre,
> > > > >
> > > > > On Mon, 6 Nov 2023 at 10:26, Andre Przywara <andre.przywara@arm.com> wrote:
> > > > > >
> > > > > > On Sat, 4 Nov 2023 19:45:06 +0000
> > > > > > Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > > On Sat, 4 Nov 2023 at 17:13, Andre Przywara <andre.przywara@arm.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, 3 Nov 2023 13:38:58 -0600
> > > > > > > > Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > > Hi Heinrich,
> > > > > > > > >
> > > > > > > > > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On 11/1/23 19:05, Andre Przywara wrote:
> > > > > > > > > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > > > > > > > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Heinrich,
> > > > > > > > > > >
> > > > > > > > > > >> The Zkr ISA extension (ratified Nov 2021) introduced the seed CSR. It
> > > > > > > > > > >> provides an interface to a physical entropy source.
> > > > > > > > > > >>
> > > > > > > > > > >> A RNG driver based on the seed CSR is provided. It depends on
> > > > > > > > > > >> mseccfg.sseed being set in the SBI firmware.
> > > > > > > > > > >
> > > > > > > > > > > As you might have seen, I added a similar driver for the respective Arm
> > > > > > > > > > > functionality:
> > > > > > > > > > > https://lore.kernel.org/u-boot/20230830113230.3925868-1-andre.przywara@arm.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 detected in some other reliable manner" for the latter.
> 
> I think Andre's statement is just missing 1 word: required
> 
> A devicetree is only *required* for peripherals *that cannot be
> located by probing*.
> 
> But really I'd phrase it in terms of what's needed for discoverable devices.
> 
> I'm somewhat surprised at this point in time we need a statement, but
> happy to add something to the DT spec. DT has been optional for
> PCI/USB since the advent of FDT and only used there when there's extra
> resources which are not discoverable. It only seems to be a question
> when it's a not $sig bus.

I think the need for a statement about this will help with less long
time and/or Linux kernel centric communities. So really do think putting
something in the spec will be helpful, and maybe further clarify or not
the RISC-V ISA thing that's elsewhere in this thread (and part of the
kernel, not a U-Boot thing).

[snip]
> > But I think drivers/timer/ is the better example of
> > needing to just have the source present, with minimal run-time checking
> > (since it's a feature of the CPU).
> 
> Timers come up frequently (well, less so with Arm arch timer now) with
> various ways to assign timers to Linux clocksource and clockevent. My
> response there is what's the difference in the instances that you care
> about assignment? If they are all the same, then you shouldn't. If
> they aren't the same, describe the difference. Sometimes it's just one
> instance has an interrupt, so it's the clockevent. Or you need the one
> that's always on. The OS/client can figure that out if you describe
> those properties. That's better than creating arbitrary indices.

In details, this is about the U-Boot API for "how long has elapsed in
the system" and not clocks for peripherals, which gets confusing in that
I _think_ in Linux it's all drivers/clocksource/ but in U-Boot we have
drivers/clk for peripherals and too-many-things for "elapsed time" and
so for example the ARMv8 generic timer isn't in the DM framework but nor
do most platforms using it trigger the "no DM migration!" warning
because the "driver" is so simple.

I introduced the "timer" part to the "rng" thread because when I saw
this going on Friday it reminded me that I'd seen another case where
"CPU lets us do X" and "this is not happy in current DM" was true, sorry
for the confusion.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]