[pmp] Fix ePMP related trap handling #903
Conversation
|
@hcallahan-lowrisc would you be able to have a look at this? |
|
@vogelpi, in case you have time would you be able to review this PR? |
Except for the full random test in which case the signature and stack stay just after the code region. This change is necessary to not break the default configuration of PMP entries, which rely on consecutive TOR regions. Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
…next instruction Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
- Renamed pmp_allow_addr_overlap to pmp_allow_illegal_tor - If illegal tor is enabled make chance of constraint being dropped 1 / XLEN - Add constraint that NAPOT addresses have a random size Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
Signed-off-by: Marno van der Maas <[email protected]>
marnovandermaas
force-pushed
the
pmp_mml_trap_fixes
branch
from
383f030 to
c2eb8fa
Compare
There was a problem hiding this comment.
Overall it looks good, I am not sure about the skipping in the case of seeing a load/store fault solution but also can't see another way as well. Just had a minor thing for the last commit but I think it shouldn't matter that much so I think this one is good to go.
| pmp_cfg[code_entry].offset = assign_default_addr_offset(pmp_num_regions, 0); | ||
| pmp_cfg[code_entry].offset = assign_default_addr_offset(pmp_num_regions, code_entry); |
There was a problem hiding this comment.
nit: I believe this fix probably needs to go to the first commit.
There was a problem hiding this comment.
One tiny nit on a comment but otherwise this looks good. Probably not worth the faff in fixing the comment as marno is away. @hcallahan-lowrisc have you seen any issues with this, are you happy with it? If there's no problems we should get this merged.
| $sformatf("csrw 0x%0x, x%0d", MEPC, scratch_reg[0]), | ||
| $sformatf("j 34f"), | ||
| $sformatf("28: csrr x%0d, 0x%0x", scratch_reg[0], MEPC), | ||
| // Increase MEPC by 4 in case instruction is compressed. |
|
@weicaiyang PTAL |
The RISCV-DV PMP trap handler is quite an intricate procedure. In essence, it iterates through the PMP entries to find the one that caused the trap, then tries to amend that entry to give it the permissions it needs and continues the test program if that was successful. This works quite well when most entries are unlocked since whenever you get an access fault, the entry is changed to allow the operation and the instruction is retried. However, there are a few problems with this approach:
Besides the trap handling, the population of entries had some shortcomings, which lead to poor coverage:
This PR addresses these issues. This is quite a big PR, but I have split it up into smaller commits to make reviewing easier. Please have a look at each commit message, but in summary these changes include:
mscratch