Hi,
On Sat, May 27, 2017 at 06:06:33PM +0000, ron minnich wrote:
Thanks for your questions and for looking at the code.
And welcome to coreboot! :-)
It's a new summer and I guess it's time for the riscv privilege model to change again, as it has for the last two summers :-), which means coreboot gets to change too.
Indeed.
On Sat, May 27, 2017 at 2:42 AM 王翔 merle@tya.email wrote:
# Some qustion about riscv implement
## 1. SMP
Coreboot for riscv does not support SMP, now. why does the secondary hart not halt in **src/arch/riscv/bootblock.S**? Now, secondary hart halts in **src/arch/riscv/trap_util.S**. This may affect playload implementation.
I never tested the code on an SMP system or properly thought through supporting SMP, but I remember the following problem: I wanted to give each hart its own stack (IOW its own initial stack pointer value) to avoid race conditions, but I didn't have enough free registers to calculate it. So I gave up and only allowed hart 0 to do SBI calls. What I didn't think about is that I could *probably* use all registers that are defined as caller-saved by the user-level spec. ("probably" because I don't know if that's a valid thing to do. The Privileged Spec should specify which registers are saved by the M-mode code across SBI calls, and which aren't.)
I guess it would make sense to catch those "other" harts in the bootblock, so even less unexpected stuff can happen, but eventually, this code needs to be reworked to deal with SMP in a more meaningful way:
* Reject calls on non-first harts by returning an error, or * Use locks (ewww, firmware-induced jitter!), or * Reserve one stack per hart, calculate sp upon entry, run SBI calls in parallel and only lock what's necessary.
The whole SBI call handling code needs to be updated to conform to the newest spec, of course. I briefly skimmed over the Privileged Spec 1.10 and it seem to have a clear definition of the SBI, though :(
To be honest, I don't know, Jonathan, do you? I have not looked at that code in some time.
I haven't, either, but until ECC2017 I plan to get up to speed with coreboot development again.
## 2. Privilege level for hypervisor
Privilege level2 is Reserved In newest **rescv-privileged-v1.10.pdf**. According to the latest **rescv-privileged-v1.10.pdf**, the privilege level 2 is reserved. However, it appears in the source code.
yes, this is a recent change with the removal of hypervisor mode.
## 3. Exception
Both **supervisor_trap_entry** and **trap_entry** in **src/arch/riscv/trap_util.S** have similar functionality. The only difference is the latter halts the secondary hart. **supervisor_trap_entry** is unused
The trap handling code needs to be cleaned up. As you noticed, it's currently not easy to understand.
currently, what's the purpose of that?
To be honest, I don't know what the exact purpose of supervisor_trap_entry was, because I haven't looked at the code in a while.
The two restore context handlers **supervisor_call_return** **machine_call_return** in **src/arch/riscv/trap_util.S** are the same and they're in another section. Why not put these codes right after **supervisor_trap_entry**/**trap_entry** so there's no need to insert assembly codes in **src/arch/riscv/trap_handler.c**.
I agree. I'd like to let the functions in trap_handler.c simply return and let the caller handle the details of switching back to lower privilege levels, etc.
We would welcome a patch so we could see what you'd like to change.
## 4. CSR(mtime mtimecmp)
Hart-local storage records the address of mtime/mtimecmp. However, I could not find information regarding CSR mapped to memory space in the latest document **rescv-privileged-v1.10.pdf**.
That's weird, but this may be because mtime/mtimecmp are not really CSRs any more, and hence are optional?
That's right. Since the Privileged Spec 1.9 (I think), mtime and mtimecmp are not CSRs anymore, but confusingly they are still named like CSRs.
You used to use the config string to find these resources, but that is possibly deprecated too. I don't know how to locate these resources at present. There seems to be a move to the FDT, which is regrettable, but at least it's not ACPI like ARM V8 :-)
Honestly, with all these details that are still in motion, I'd like to have a SoC/board that implements *something* (and documents how it works). If we had such a board, we could at least port coreboot without having to guess the correct interpretation of half-written specs. :-\
Luckily riscv-linux is now being upstreamed so they will have to define a boot protocol (like https://www.kernel.org/doc/Documentation/arm/Booting: What goes into which register before the bootloader jumps into the kernel, etc).
Back on topic: I wonder if we should just import libfdt when we need to parse or modify devicetree blobs. A quick check on the libfdt.a on my system (x86_64) shows that the set of .o files takes about 15kB, so it's not huge.
## 5. S/M Privilege level use same stack
Function **riscvpayload** in **src/arch/riscv/payload.S** does not create a new stack for S privilege level. This may destroy the stack of M privilege level.
The M privilege level is not really a level in any normal sense, or at least it was not. Supervisor level can examine and rewrite all of M memory.
From what I've heard, M-mode code is supposed to be able to protect itself from rogue S-mode code, on processors that implement PMP ("Physical Memory Protection", see Priv Spec 1.10, chapter 3.6).
Also, we assume the payload will create its own stack if needed, as it should. Finally, it makes no sense for M level to create a stack for S level; this would imply the M level has knowledge of the S level activities and that's just plain wrong. S level should always set up its own stack.
Overall the M level / S level interactions had some real problems in the earlier spec, and I'll have to look at the new spec to see what's changed.
I'm not sure how SBI calls are supposed to work now (in 1.10 and later), but I *heard* they use plain ECALLs now, instead of the trampoline page.
Thank you for looking at this code and if you have changes to make we'd be interested in seeing them.
ron
BTW, Ron, I think we should consider splitting the memory-resident part of coreboot on RISC-V into its own stage, similar to the SMM code on x86. That makes it easier to see what and how much of coreboot is actually available and active after boot.
Jonathan