<div dir="ltr">Thanks for your questions and for looking at the code. <div><br></div><div>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. </div><div><br><div class="gmail_quote"><div dir="ltr">On Sat, May 27, 2017 at 2:42 AM 王翔 <merle@tya.email> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div># Some qustion about riscv implement</div><div><br></div><div>## 1. SMP</div><div><br></div><div>Coreboot for riscv does not support SMP, now. </div><div>why does the secondary hart not halt in **src/arch/riscv/bootblock.S**?</div><div>Now, secondary hart halts in **src/arch/riscv/trap_util.S**.</div><div>This may affect playload implementation.</div></div></blockquote><div><br></div><div><br></div><div>To be honest, I don't know,  Jonathan, do you? I have not looked at that code in some time.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br></div><div><br></div><div>## 2. Privilege level for hypervisor</div><div><br></div><div>Privilege level2 is Reserved In newest **rescv-privileged-v1.10.pdf**.</div><div>According to the latest **rescv-privileged-v1.10.pdf**, the privilege level 2 is</div><div>reserved. However, it appears in the source code. </div></div></blockquote><div><br></div><div>yes, this is a recent change with the removal of hypervisor mode. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br></div><div><br></div><div>## 3. Exception </div><div><br></div><div>Both **supervisor_trap_entry** and **trap_entry** in </div><div>**src/arch/riscv/trap_util.S** have similar functionality. The only difference </div><div>is the latter halts the secondary hart. **supervisor_trap_entry** is unused </div><div>currently, what's the purpose of that?</div><div><br></div><div>The two restore context handlers **supervisor_call_return** </div><div>**machine_call_return** in **src/arch/riscv/trap_util.S** are the same and </div><div>they're in another section. Why not put these codes right after </div><div>**supervisor_trap_entry**/**trap_entry** so there's no need to insert assembly </div><div>codes in **src/arch/riscv/trap_handler.c**.</div></div></blockquote><div><br></div><div>We would welcome a patch so we could see what you'd like to change.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br></div><div><br></div><div>## 4. CSR(mtime mtimecmp)</div><div><br></div><div>Hart-local storage records the address of mtime/mtimecmp. However, I could not </div><div>find information regarding CSR mapped to memory space in the latest document </div><div>**rescv-privileged-v1.10.pdf**.</div></div></blockquote><div><br></div><div>That's weird, but this may be because mtime/mtimecmp are not really CSRs any more, and hence are optional?</div><div><br></div><div>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 :-)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br></div><div><br></div><div>## 5. S/M Privilege level use same stack</div><div><br></div><div>Function **riscvpayload** in **src/arch/riscv/payload.S** does not create a new</div><div>stack for S privilege level. This may destroy the stack of M privilege level.</div></div><div><br></div><div><u></u><div style="color:#909090;font-family:Arial Narrow;font-size:12px"><br></div></div><br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Thank you for looking at this code and if you have changes to make we'd be interested in seeing them.</div><div><br></div><div>ron</div></div></div></div>