Re: [Qemu-devel] [Qemu-riscv] [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
> >
> >
>
>