[coreboot] Some qustion about riscv implement

[Jonathan Neuschäfer]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20170528/646474a6/attachment.asc>