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

[Jonathan Behrens]

Hesham,

I don't think this is quite right. If I understand correctly, PMP permissions are only validated on TLB fills, not on all accesses. (Is anyone able to confirm this?) If so, this function can't just validate the range of a single access and then place the entire page into the TLB. However, the current code is also wrong because an access should succeed/fail based on the permissions only for the range it actually touches even regardless of the permissions on the rest of the page. Now that I think about it, I'd also expect that somewhere in the PMP logic would flush the TLB every time any of the related control registers change though I can't find anywhere that this is happening...

Sorry to keep raising complaints about this patch set, the interaction between physical memory protection and paging is very subtle. Even some real hardware has had errata related to it!

Jonathan

On Tue, May 21, 2019 at 6:33 PM Alistair Francis <address@hidden> wrote:

On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
<address@hidden> wrote:
>
> 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>

Alistair

> ---
>  target/riscv/cpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d0b0f9cf88..ce1f47e4e3 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -410,7 +410,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)) {
> +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
>          ret = TRANSLATE_PMP_FAIL;
>      }
>      if (ret == TRANSLATE_PMP_FAIL) {
> --
> 2.17.1
>
>