[coreboot] Some qustion about riscv implement

[ron minnich]

Thanks for your questions and for looking at the code.

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.

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.
>


To be honest, I don't know,  Jonathan, do you? I have not looked at that
code in some time.



>
> ## 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
> currently, what's the purpose of that?
>
> 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**.
>

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?

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 :-)



>
>
> ## 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.
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.

Thank you for looking at this code and if you have changes to make we'd be
interested in seeing them.

ron
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20170527/a391f0ab/attachment.html>