riscv-qemu
riscv-qemu copied to clipboard
riscvarchive
implement TSR, TW and TVM for privileged ISA v1.10
The TVM (Trap Virtual Memory) bit supports intercepting supervisor virtual-memory management
operations. When TVM=1, attempts to read or write the satp CSR or execute the
SFENCE.VMA instruction while executing in S-mode will raise an illegal instruction exception.
When TVM=0, these operations are permitted in S-mode. TVM is hard-wired to 0 when S-mode
is not supported.
The TW (Timeout Wait) bit supports intercepting the WFI instruction (see Section 3.2.3). When
TW=0, the WFI instruction is permitted in S-mode. When TW=1, if WFI is executed in S-mode,
and it does not complete within an implementation-specific, bounded time limit, the WFI
instruction causes an illegal instruction trap. The time limit may always be 0, in which case WFI
always causes an illegal instruction trap in S-mode when TW=1. TW is hard-wired to 0 when
S-mode is not supported.
The TSR (Trap SRET) bit supports intercepting the supervisor exception return instruction, SRET.
When TSR=1, attempts to execute SRET while executing in S-mode will raise an illegal instruction
exception. When TSR=0, this operation is permitted in S-mode. TSR is hard-wired to 0 when
S-mode is not supported.
@msuozzo Hi Matthew, yes, no conflict here...
However you may want to be aware of these two planned changes:
- https://github.com/riscv/riscv-qemu/issues/126
- https://github.com/riscv/riscv-qemu/issues/127
If we work on them, we may have conflicts in target/riscv/op_helper.c. The easiest way to avoid conflicts is for you to have a crack at those to issues as well. The second one is basically adding a level of indirection to access CSRs so that the TCG helpers and GDB can both call common access functions, but GDB can be assured to not have exceptions raised when accessing CSRs. This would essentially make the TCG helpers just wrappers to riscv_csr_read and riscv_csr_write, translating a -1 return code into an exception e.g.
do_raise_exception_err(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
I'm not planning on doing this particular refactoring right away, so we either have to do the changes in serial (I can wait) or you could pick up these two issues too... https://github.com/riscv/riscv-qemu/issues/126 is related as well as it makes more CSRs generate exceptions for cores that don't implement S mode. I thought it was relevant because we want to make the CPU model flexible i.e. able to model M-mode only cores with no MMU, cores with M,S and U mode, and then of course cores with TSR/TVM/TW (priv 1.10), and finally cores with everything i.e. HS-mode in (emulated) hardware vs software.
It turns out that for the piece I'm working on at the moment (local-interrupts), I need to implement an atomic read/modify/write CSR interface, and currently, CSRs are implemented using independent csr_read_helper and csr_write_helper methods (making an atomic read/modify/update difficult), so I might in fact be able to work on https://github.com/riscv/riscv-qemu/issues/126 and https://github.com/riscv/riscv-qemu/issues/127
I didn't intend to clash with your work but it seems it now might make sense that I implement those other two issues. I'll share a branch ASAP so that our work fits together...
The changes for this issue i.e. mstatus.TSR/TVM/TW are in the mstatus CSR case clauses, and elsewhere where we need to add checks, e.g. in SRET, WFI and SATP, so we should be able to merge our changes easily. Indeed after the change I'm working on, CSRs will have functions, and the big switch statatements will be replaced by an array. e.g.
/* CSR accessor function table */
static const riscv_csr_operations csr_ops[0xfff] =
{
/* Floating point control and status registers */
[CSR_FFLAGS] = { read_fflags, write_fflags, NULL },
[CSR_FRM] = { read_frm, write_frm, NULL },
[CSR_FCSR] = { read_fcsr, write_fcsr, NULL },
/* User-level cycle and instruction counters */
[CSR_CYCLE] = { read_instret, NULL, NULL },
[CSR_INSTRET] = { read_instret, NULL, NULL },
#if defined(TARGET_RISCV32)
[CSR_CYCLEH] = { read_instreth, NULL, NULL },
[CSR_INSTRETH] = { read_instreth, NULL, NULL },
#endif
/* User-level time CSRs are only available in linux-user
* In privileged mode, the monitor emulates these CSRs */
#if defined(CONFIG_USER_ONLY)
[CSR_TIME] = { read_time, NULL, NULL },
#if defined(TARGET_RISCV32)
[CSR_TIMEH] = { read_timeh, NULL, NULL },
#endif
#endif
/* M-mode privileged cycle and instruction counters */
#if !defined(CONFIG_USER_ONLY)
[CSR_MCYCLE] = { read_instret, NULL, NULL },
[CSR_MINSTRET] = { read_instret, NULL, NULL },
#if defined(TARGET_RISCV32)
[CSR_MCYCLEH] = { read_instreth, NULL, NULL },
[CSR_MINSTRETH] = { read_instreth, NULL, NULL },
#endif
#endif
/* M-mode privileged CSRs */
#if !defined(CONFIG_USER_ONLY)
[CSR_MSTATUS] = { read_mstatus, write_mstatus, NULL },
[CSR_MIP] = { read_mip, write_mip, NULL },
[CSR_MIE] = { read_mie, write_mie, NULL },
[CSR_MIDELEG] = { read_mideleg, write_mideleg, NULL },
[CSR_MEDELEG] = { read_medeleg, write_medeleg, NULL },
#endif
};
The third column is a function pointer for CSRs that want to implement an atomic read/modify/update instead of separate read and write functions.
It shouldn't be too hard to merge your changes. Soon i'll have an experimental branch with the new array of function pointers based CSR dispatch...
Nice. That seems like an easy merge.
I did have one question regarding WFI. The spec defines the mstatus.TW register's behavior as follows:
if WFI is executed in S-mode, and it does not complete within an implementation-specific, bounded time limit, the WFI instruction causes an illegal instruction trap. The time limit may always be 0...
Looking at Spike, it doesn't look like there is any time limit interface supported:
require_privilege(get_field(STATE.mstatus, MSTATUS_TW) ? PRV_M : PRV_S);
Does it make sense to implement the configurable timer in QEMU?
I suspect that this won't be a very popular feature for hardware, at least in the beginning, so my plan was to use the same behavior as Spike and hard-code it to 0.
I think just trapping on WFI in S-mode if mstatus.TW is set is the easiest. i.e. timelimit = 0
BTW - This branch contains the new scheme for CSRs:
- https://github.com/michaeljclark/riscv-qemu/blob/wip-local-interrupts/target/riscv/csr.c#L778-L866
If you have any logic inside of the two big switch statements (csr_read_helper, or csr_write_helper) in target/riscv/op_helper.c then it can be moved into one of the functions in target/riscv/csr.c which will contain all control and status register update logic. target/riscv/op_helper.c now only contains instruction helpers and some non-instruction helpers have moved to target/riscv/cpu_helper.c.
There is now table driven CSR dispatch with separate read and write methods for each CSR. IMHO it is much easier to read and the logic is more clearly organised. It takes several more lines that the old switch statement due to one or two functions per CSR, i.e. there is some extra whitespace and curly brackets, but it allows us some more flexibility. It's now possible to implement an atomic read/modify/write CSR which is next on my list... That was not possible while CSRs were implemented using csr_read_helper and csr_write_helper. The new array of function pointer dispatch allows a CSR to define a combined r/m/w interface (riscv_csr_op_fn vs riscv_csr_read_fn and riscv_csr_write_fn). If an atomic method exists, it will be called instead of the separate read and write methods. Likewise non-writable CSRs are handled by simply not defining a write function. We have also fixed the issue with gdbstub.c https://github.com/riscv/riscv-qemu/issues/127 with non-trapping CSR accessors.
I managed to cut idle CPU usage for SMP/MTTCG by 25% with some optimization to interrupt handling (using atomic_cmgxchg and avoiding unnecessarily taking the iothread_mutex).
I didn't realize you had made this change and was just about to merge something essentially the same.
- https://github.com/michaeljclark/riscv-qemu/commit/ae8dbab3cc250dabe5031c412b7b202fd1e50a9c
Both of our implementations are close to exactly the same. I noticed you had the S-mode check but it is missing on satp, and is redundant on SRET as it is checked a few lines earlier. I made changes to my implementation based on yours. So we were both doing it at the same time I think...
Thanks, Michael.