[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: Richard Henderson
Subject: Re: [Qemu-riscv] [Qemu-devel] [PULL 10/34] RISC-V: Fix a PMP check with the correct access size
Date: Thu, 27 Jun 2019 20:23:20 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

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.


reply via email to

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