On 6/27/19 7:44 PM, Jonathan Behrens wrote:
I think this patch is slightly incorrect. If the PMP region is valid for
the size of the access, but not the rest of the page then a few lines down
in this function the entire page should not be placed into the TLB. Instead
only the portion of the page that passed the access check should be
included. To give an example of where this goes wrong, in the code below
access to address 0x90000008 should always fail due to PMP rules, but if
the TLB has already been primed by loading the (allowed) address 0x90000000
then no fault will be triggered. Notably, this code also executes
improperly without the patch because the first `ld` instruction traps when
it shouldn't.
li t0, 0x0000000024000000 // region[0]: 0x90000000..0x90000007
csrw pmpaddr0, t0
li t0, 0x00000000240001FF // region[1]: 0x90000000..0x90000fff
csrw pmpaddr1, t0
li t0, 0x1F0000000000989F // cfg[0] = LXRW, cfg[1] = L
csrw pmpcfg0, t0
sfence.vma
li t0, 0x90000000
ld s0, 0(t0)
ld s1, 8(t0) // NO TRAP: address is incorrectly in TLB!
Nice test case.
I think that the proper fix would be to first do a PMP check for the full
PAGE_SIZE and execute normally if it passes. Then in the event the full
page fails, there could be a more granular PMP check with only the accessed
region inserted as an entry in the TLB.
This feature looks to be almost identical to the ARM m-profile MPU.
The fix is:
If the PMP check is valid for the entire page, then continue to call
tlb_set_page with size=TARGET_PAGE_SIZE.
If the PMP check is valid for the current access, but not for the entire page,
then call tlb_set_page with any size < TARGET_PAGE_SIZE. This change alone is
sufficient, even though the full argument tuple (paddr, vaddr, size) no longer
quite make perfect sense. (For the arm mpu, we compute some 1 << rsize, but
the actual value is never used; setting size=1 would be sufficient.)
Any size < TARGET_PAGE_SIZE will cause TLB_RECHECK to be set for the page,
which will force all accesses to the page to go back through riscv_cpu_tlb_fill
for re-validation.