[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Palmer Dabbelt
Subject: Re: [Qemu-riscv] [Qemu-devel] [PULL 10/34] RISC-V: Fix a PMP check with the correct access size
Date: Mon, 08 Jul 2019 05:46:42 -0700 (PDT)

On Thu, 27 Jun 2019 11:23:20 PDT (-0700), address@hidden wrote:
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


  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.

RISC-V allows another option: support for fine-grained PMPs is optional, which
allows implementations ignore this issue to save on hardware costs.  It looks
like Rocket leaves this as a parameter


but our implementations always have page-granularity PMPs if virtual memory is

Unless anyone objects, I'll restrict the PMP granularity to 4K on all systems
that support virtual memory as a bug fix.  We can add the configuration option
in as a feature.

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]