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: 0x90000000..0x90000007
csrw pmpaddr0, t0
li t0, 0x00000000240001FF // region: 0x90000000..0x90000fff
csrw pmpaddr1, t0
li t0, 0x1F0000000000989F // cfg = LXRW, cfg = L
csrw pmpcfg0, t0
li t0, 0x90000000
ld s0, 0(t0)
ld s1, 8(t0) // NO TRAP: address is incorrectly in TLB!
ld s1, 8(t0) // TRAP: TLB has been flushed!
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.
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?