riscv: rework CPU identification [3/6]

[mhorne]

Summary

riscv: rework CPU identification [3/6]

Perform all parsing+reporting on CPU 0.

Make use of the SMP hooks cpu_mp_start() and cpu_mp_announce() to
identify and print secondary CPU info, respectively.

Eliminate the SYSINIT from identcpu.c. We still need to walk the /cpus
list in the device tree, but now do this one CPU at a time, as a step in
the identify_cpu() procedure. This is slightly less error prone, and
allows us to parse ISA features for CPU 0 much earlier.

This causes secondary processor identification to be printed much
earlier in boot; everything is done by SI_SUB_CPU, SI_ORDER_THIRD.
Adjust some other printf calls so that we get enough useful info to
debug under bootverbose.

[mhorne]

Update to fix the existing warning message, and remove the bootverbose check.

[markj]

sys/riscv/riscv/identcpu.c

317–322

Multiple APs can call this function in parallel and nothing serializes the updates to elf_hwcap, I believe. In particular, init_secondary() calls identify_cpu() outside of the ap_boot_mtx.

406

Is there a reason for the extra newline here?

[mhorne]

sys/riscv/riscv/identcpu.c

317–322

Hmm yes, I totally overlooked that part of it. It makes me want to do away with this stuff altogether and just use whatever CPU 0 reports, but I have an awful feeling this type of defensiveness will be needed somewhere, someday.

Future changes will add more global state like this for supervisor-level extensions, which also shouldn't vary between CPUs, but it will need to be protected just the same.

I'll update to add a mutex for this.

406

Not really. In my mind identifying the vendor/CPU is a distinct type of action from parsing the features, so I put the blank line. But the functions themselves make this clear enough.

[mhorne]

Protect updates to elf_hwcap with a mutex (UPDATE_GLOBAL_CAP).

[jrtc27]

Just glancing through this, not a hugely detailed review, but skimming the summaries this sounds like it's all headed in the right direction and if Mark's reviewing it then I trust you both on the implementation side :)

sys/riscv/riscv/identcpu.c

81

Thinking forward to things like hotplug, this creates a bit of asymmetry where APs available at boot influence elf_hwcap but those available later do not (or, at least, should not, otherwise things will surely get very confused). I guess that's the best you can do, so this is more of an observation...

91

Any reason you went for a mutex rather than atomic_clear_long?

[markj]

sys/riscv/riscv/identcpu.c

91

I think using atomics is a bit nicer indeed, then one doesn't have to consider whether the mutex is initialized before using it.

[mhorne]

sys/riscv/riscv/identcpu.c

81

This is true, and one of many considerations to be made here. The structure of having each CPU parse its own features makes most sense to me at present, and allows us to get some of this information early.

If we ever support hotplug, then this can be reevaluated. I am certain there will be other developments before then, so the structure of the code (and really the hw ecosystem) will be more concrete. Personally I do not want to be the one dealing with hot-pluggable heterogenous CPUs... that is a nasty combination.

91

Sorry, missed the reply here.

Atomics are nicer, but the mutex is simply because I want to use this for different types, not just long (e.g. mmu_caps in D39814). I used a macro rather than inline function for the same reason.

[jrtc27]

sys/riscv/riscv/identcpu.c

91

That one can use atomic_clear_int instead?

[mhorne]

sys/riscv/riscv/identcpu.c

91

That one can. I also intend to use this for bool types, for which we do not have atomic primitives.

[mhorne]

Rethink the series again; third time's the charm.

After another look, I think it is fine to perform all the parsing on the BSP.
And it allows us to avoid the synchronization mess altogether. Possibly there
could be in the future some identification step that requires reading e.g. a
CPU register, but we will cross that bridge if it comes to it.

Use CPU_FOREACH() in cpu_mp_start(), which is valid as we have just finished
enumerating and starting APs via cpu_init_fdt().

This ends up absorbing some of the changes that came later in the series out of
necessity, and so the numbering is slightly off. I will probably omit these
altogether when it comes time to push the commits.

[markj]

sys/riscv/riscv/identcpu.c

351

I think this doesn't particularly need to be a macro now?

sys/riscv/riscv/mp_machdep.c

534

It looks a bit odd to do it this way: it happens to work because on riscv we enumerate CPU features by looking at the device tree, but on other platforms we generally have to execute some code on the target CPU.

But, I suppose identify_cpu() could also be implemented using smp_rendezvous(), so your scheme could be made to work for other hypothetical CPU enum methods.

[markj]

sys/riscv/riscv/identcpu.c

351

Looking ahead in the patch series, it's more clear, so I think you can ignore this comment.

[mhorne]

sys/riscv/riscv/mp_machdep.c

534

Yeah, so this is the judgement call to be made, whether or not we should require the identification of CPU n to run on CPU n. In the earlier revision I tried to have it work that way, and ended up with the ugly mutex, so I tried rethinking it. There were other solutions to that minor problem, but I am a bigger fan of what I have now.

At present, the ISA spec itself doesn't really define a method for supervisor software to identify supported extensions, it only defines the format of the ISA string. For better or worse it seems they have decided against ID registers like we see for ARM. Future methods of hardware enumeration we can expect for RISC-V are ACPI, and maybe SMBIOS to a lesser degree [1]. At least in the present draft of a RISC-V ACPI spec, there is reference to a "RISC-V Hart Capabilities Table (RHCT)", which will "communicate information about certian capabilities like ISA string" [2]. This implies that the we will be able to perform the same string parsing that we currently do, with the string being sourced from an ACPI table rather than device tree. This is the assumption I am operating under, and the string parsing code I've added thus far is properly separated from the FDT-specific bits.

If it does become necessary, we can have secondary harts execute some of their own identification code near the beginning of init_secondary(), and wait on it via smp_rendezvous(), as you say.

There are a lot of different concerns to balance here, some of them still being unknowns, due to the relative immaturity of the RISC-V specs and hardware ecosystem.

[1] https://github.com/riscv/riscv-platform-specs/blob/main/riscv-platform-spec.adoc#452-hardware-discovery-mechanisms
[2] https://github.com/riscv-non-isa/riscv-acpi/releases/tag/V0.2, Chapter 4