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

[Jonathan Behrens]

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!

  sfence.vma
  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?

Jonathan

On Thu, Jun 27, 2019 at 11:43 AM Palmer Dabbelt <address@hidden> wrote:

From: Hesham Almatary <address@hidden>

The PMP check should be of the memory access size rather
than TARGET_PAGE_SIZE.

Signed-off-by: Hesham Almatary <address@hidden>
Reviewed-by: Alistair Francis <address@hidden>
Signed-off-by: Palmer Dabbelt <address@hidden>
---
 target/riscv/cpu_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 66be83210f11..e1b079e69c60 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -452,8 +452,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,

     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
-        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type,
-        mode)) {
+        !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
         ret = TRANSLATE_PMP_FAIL;
     }
     if (ret == TRANSLATE_PMP_FAIL) {
--
2.21.0