| Message ID | 20190627152011.18686-11-palmer@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL] RISC-V Patches for the 4.1 Soft Freeze, Part 2 | expand |
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! sfence.vma ld s1, 8(t0) // TRAP: TLB has been flushed! 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. 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? Jonathan On Thu, Jun 27, 2019 at 11:43 AM Palmer Dabbelt <palmer@sifive.com> wrote: > From: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk> > > The PMP check should be of the memory access size rather > than TARGET_PAGE_SIZE. > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > target/riscv/cpu_helper.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 66be83210f11..e1b079e69c60 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -452,8 +452,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, > - mode)) { > + !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) { > ret = TRANSLATE_PMP_FAIL; > } > if (ret == TRANSLATE_PMP_FAIL) { > -- > 2.21.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. r~
On Thu, 27 Jun 2019 11:23:20 PDT (-0700), richard.henderson@linaro.org 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 >> >> 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. 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 https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/rocket/TLB.scala#L159 but our implementations always have page-granularity PMPs if virtual memory is supported. 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. > > > r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 66be83210f11..e1b079e69c60 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -452,8 +452,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, - mode)) { + !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) { ret = TRANSLATE_PMP_FAIL; } if (ret == TRANSLATE_PMP_FAIL) {