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. > Unrelated question: should I be sending "Reviewed By" lines if I read > through patches that seem reasonable? Or there some formal process I'd have > to go through first to be approved? No formal process. More eyes are always welcome. r~
