Re: [Qemu-devel] [Qemu-riscv] [PULL 10/34] RISC-V: Fix a PMP check with the correct access size

[Richard Henderson <richard.henderson@linaro.org>]

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~