plic mvp #1476
plic mvp #1476
Conversation
Timmmm
left a comment
Timmmm
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This has been on my todo list for a while.
If you don't mind I can go through the code and fix all the types etc. Then I'll review the actual logic. Let me know so we don't both end up doing it!
| let PLIC_OFF_CTX_BASE : int = unsigned(0x200000) | ||
| let PLIC_CTX_STRIDE : int = unsigned(0x1000) // per-context page | ||
| let PLIC_OFF_THRESHOLD : int = 0 | ||
| let PLIC_OFF_CLAIM : int = 4 |
There was a problem hiding this comment.
Remove all of these : int annotations. Sail will infer more precise types (e.g. let PLIC_OFF_CLAIM : int(4) = 4).
|
|
||
| .comment 0x0000000000000000 0xf | ||
| .comment 0x0000000000000000 0xf /tmp/ccS5KxDq.o | ||
| 0x10 (size before relaxing) |
There was a problem hiding this comment.
I'm guessing you didn't mean to commit this file.
| } | ||
|
|
||
| private function within_plic forall 'n, 0 < 'n <= max_mem_access . | ||
| (Physaddr(addr) : physaddr, width : int('n)) -> bool = { |
There was a problem hiding this comment.
Very minor but can you format this the same as within_clint. We'll probably use an auto-formatter eventually but we should be consistent where possible.
|
|
||
| // Term-level constants (can be used in loops/arith) | ||
| let PLIC_NSRC : int = 64 // IDs 0..63, ID0 reserved | ||
| let PLIC_NCTX : int = 2 // ctx0 = hart0/M, ctx1 = hart0/S |
There was a problem hiding this comment.
You can use the same name for both; e.g. look how xlen is defined.
type plic_nsrc = 64
let plic_nsrc = sizeof(plic_nsrc)
| let PLIC_NSRC : int = 64 // IDs 0..63, ID0 reserved | ||
| let PLIC_NCTX : int = 2 // ctx0 = hart0/M, ctx1 = hart0/S | ||
|
|
||
| register plic_priority : list(bits(32)) = [||] |
There was a problem hiding this comment.
Use a vector instead of a list. Lists are awkward for some backends and we try to avoid them wherever possible. In this case I believe the length is constant? Something like this:
register plic_priority : vector(plic_nctx, bits(32)) = vector_init(zeros())
| private function plic_mask32(idx:int) -> bits(32) = { | ||
| if idx < 0 | idx >= 32 then zeros() | ||
| else (zero_extend(32, 0b1) << idx) | ||
| } |
There was a problem hiding this comment.
Generally using int is a sign that the code could be improved by tighter types. With a few exceptions we shouldn't have int anywhere.
For example in this case you probably want something like:
private function plic_mask32(idx : range(0, 31)) -> bits(32) =
zero_extend(0b1 @ zeros(idx))
Although actually I don't think you need this function at all. Generally in Sail you don't do bitshifts and masking like in C; you use direct bit access and bitvector concatenation instead.
| if idx < 0 then false | ||
| else if idx < 32 then { | ||
| let lo : bits(32) = b[31..0]; | ||
| (lo & plic_mask32(idx)) != zeros() |
There was a problem hiding this comment.
You can just access the bit directly like b[idx] != bitzero.
| # PLIC claim/complete for ctx0(M): PLIC_BASE + 0x200004 | ||
| li t2, 0x0c000000 | ||
| li t3, 0x200004 | ||
| add t2, t2, t3 |
There was a problem hiding this comment.
I think it would be better to introduce a header file and move these definitions into it, so they can be reused for the C and assembly files. This would remove the magic numbers and would make it easier to maintain.
#ifndef PLIC_DEFS_H
#define PLIC_DEFS_H
#define PLIC_BASE 0x0c000000
#define PLIC_CLAIM_CTX0 (PLIC_BASE + 0x200004)
#define IRQ_ID 10
#define MCAUSE_MEI 11
#endif
There was a problem hiding this comment.
Tbh I think it would be easier just to rewrite the test in C. I don't think it really benefits from being in assembly.
| 1: j 1b | ||
|
|
||
| .p2align 2 | ||
| .globl trap_entry | ||
| trap_entry: | ||
| addi sp, sp, -64 | ||
| sd ra, 56(sp) | ||
| sd t0, 0(sp) | ||
| sd t1, 8(sp) | ||
| sd t2, 16(sp) | ||
| sd t3, 24(sp) | ||
| sd t4, 32(sp) | ||
| sd t5, 40(sp) | ||
| sd t6, 48(sp) | ||
|
|
||
| csrr t0, mcause | ||
| bltz t0, 1f # interrupt => mcause is negative | ||
| j 9f |
There was a problem hiding this comment.
| 1: j 1b | |
| .p2align 2 | |
| .globl trap_entry | |
| trap_entry: | |
| addi sp, sp, -64 | |
| sd ra, 56(sp) | |
| sd t0, 0(sp) | |
| sd t1, 8(sp) | |
| sd t2, 16(sp) | |
| sd t3, 24(sp) | |
| sd t4, 32(sp) | |
| sd t5, 40(sp) | |
| sd t6, 48(sp) | |
| csrr t0, mcause | |
| bltz t0, 1f # interrupt => mcause is negative | |
| j 9f | |
| halt: j halt | |
| .p2align 2 | |
| .globl trap_entry | |
| trap_entry: | |
| addi sp, sp, -64 | |
| sd ra, 56(sp) | |
| sd t0, 0(sp) | |
| sd t1, 8(sp) | |
| sd t2, 16(sp) | |
| sd t3, 24(sp) | |
| sd t4, 32(sp) | |
| sd t5, 40(sp) | |
| sd t6, 48(sp) | |
| csrr t0, mcause | |
| bltz t0, 1f # interrupt => mcause is negative | |
| j 9f |
Maybe its just me, but I am not a big fan of reusing labels and use the same labels for backward and forward jumps. I think it would be clearer to give these labels explicit names (I modified one label as an example).
| let MTIME_BASE_HI : physaddrbits = zero_extend(0x0bffc) | ||
|
|
||
| // Type-level sizes (can be used in bits()/vector()) | ||
| type plic_nsrc = 64 |
There was a problem hiding this comment.
This should be configurable.
4 participants
This PR adds MVP PLIC support to the Sail RISC-V platform model and includes a bare-metal test to validate the basic external interrupt flow. The current implementation covers the end-to-end path (trigger → claim/complete → HTIF exit).
Follow-ups will focus on broader coverage (more interrupt sources and scenarios) and improved OS compatibility over time.