New subject: Some qustion about riscv implement

28 May 2017


      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

Re: [coreboot] Some qustion about riscv implement