ot_earlgrey: add ePMP configuration machine property with facade feature
#241
Conversation
Signed-off-by: James Wainwright <[email protected]>
Signed-off-by: James Wainwright <[email protected]>
| if (csrno <= CSR_PMPCFG3) { | ||
| uint32_t reg_index = csrno - CSR_PMPCFG0; | ||
|
|
||
| /* TODO: RV128 restriction check */ |
There was a problem hiding this comment.
I think you can remove this: OT is neither RV64 nor RV128
| * Facades for PMP CSRs - reads and writes to masked regions end up here instead | ||
| * of the effective PMP configuration. | ||
| */ | ||
| static target_ulong reg_pmpcfg_facade[MAX_RISCV_PMPS / 4] = { 0 }; |
There was a problem hiding this comment.
No need to { 0 } since it a static declaration.
4u
|
|
||
| /* each PMPCFG CSR covers four PMP regions; mask the affected parts */ | ||
| target_ulong mask = 0; | ||
| for (unsigned i = 0; i < 4; i++) { |
There was a problem hiding this comment.
nit pick: ix, idx, ... avoid single char var.
| target_ulong mask = 0; | ||
| for (unsigned i = 0; i < 4; i++) { | ||
| if (ms->pmp_region_masked[reg_index * 4 + i]) { | ||
| mask |= (0xff << (8 * i)); |
| } | ||
|
|
||
| /* only read from the PMP if there are unmasked regions in this CSR */ | ||
| if (mask != -1) { |
There was a problem hiding this comment.
explicit cast or maybe a mask based on TARGET_LONG_BITS?
| */ | ||
|
|
||
| #include "qemu/osdep.h" | ||
| #include <string.h> |
| const char *config = ms->epmp_regions; | ||
|
|
||
| /* configure the CSR hooks for all `PMPCFG` and `PMPADDR` CSRs */ | ||
| for (int csr = CSR_PMPCFG0; csr <= CSR_PMPCFG3; csr++) { |
| } | ||
|
|
||
| /* parse the flags */ | ||
| bool l = false, r = false, w = false, x = false, f = false; |
| .write = write_pmpaddr_masked, | ||
| }; | ||
|
|
||
| /* |
There was a problem hiding this comment.
I would think this is such a large feature that it deserves its own file (ot_epmp_eg.c or the like).
There was a problem hiding this comment.
It is a bit weird that the epmp configuration is defined in the machine itself.
where you would define the epmp propery on the epmp device. It would be much cleaner and could be re-used for another SoC.
| bool verilator; | ||
| /* ePMP region specification string */ | ||
| char *epmp_regions; | ||
| /* Whether to redirect an PMP region's CSR reads and writes to the facade */ |
There was a problem hiding this comment.
pmp_region_masked is not a property, so it would be better to move it before them as with other devices.
|
We're having problems with using this as a workaround:
For now I'm going to mark this PR as draft, we may continue it later but for now it's on hold. Thank you @rivos-eblot for reviewing anyway, I hope I didn't waste your time. |
No worries. We kinda have the same issue (with multiple ePMP reconfigurations at runtime), but I think we sorted out the |
|
Yeah, we also found that the default configuration for PMP is hardcoded in the SV and it might be good to have |
2 participants
This PR adds a way to configure the default ePMP configuration with a machine property (
epmp_regions). This is applied after the configuration specified in the.cfile.Along with the regular region flags (
LWRX) the configuration supports a specialF(facade) flag. This causes writes to a region's CSRs to have no effect on the PMP and instead be applied to a facade instead. Ibex reads back from this facade and doesn't know the PMP was unchanged.The facade feature can be used for debugging and performance purposes without affecting software which reads back the CSRs to ensure they were correctly configured (i.e. the ROM). The expected use-case of this configuration is for over-aligning regions to prevent TLB misses and improve performance in the ROM.
It's currently earlgrey-only because I'm storing state in the machine context. To avoid duplicating code, I think I would like to introduce a shared OpenTitan machine context used by both earlgrey and darjeeling if this turns out to be a feature that Darjeeling wants as well.