On Wed, 22 May 2019 at 03:25, Jonathan Behrens <finte...@gmail.com> wrote: > > 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... > I believe the TLB fill function is called on all accesses, but I might be wrong. I will wait for someone to confirm otherwise.
It's mentioned in the spec that sfence.vma has to be executed after changing PMP configs, so it's a SW concern (i.e., not QEMU's). > 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 <alistai...@gmail.com> wrote: >> >> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary >> <hesham.almat...@cl.cam.ac.uk> wrote: >> > >> > The PMP check should be of the memory access size rather >> > than TARGET_PAGE_SIZE. >> > >> > Signed-off-by: Hesham Almatary <hesham.almat...@cl.cam.ac.uk> >> >> Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> >> >> 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 >> > >> > >>