On Wed, Jun 5, 2019 at 3:59 PM Hesham Almatary
<hesham.almat...@cl.cam.ac.uk> wrote:
>
> On Wed, 5 Jun 2019 at 23:07, Alistair Francis <alistai...@gmail.com> wrote:
> >
> > On Thu, May 30, 2019 at 6:52 AM Hesham Almatary
> > <hesham.almat...@cl.cam.ac.uk> wrote:
> > >
> > > The PMP should be checked when doing a page table walk, and report access
> > > fault exception if the to-be-read PTE failed the PMP check.
> > >
> > > Suggested-by: Jonathan Behrens <finte...@gmail.com>
> > > Signed-off-by: Hesham Almatary <hesham.almat...@cl.cam.ac.uk>
> > > ---
> > > target/riscv/cpu.h | 1 +
> > > target/riscv/cpu_helper.c | 10 +++++++++-
> > > 2 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index c17184f4e4..ab3ba3f15a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -94,6 +94,7 @@ enum {
> > > #define PRIV_VERSION_1_09_1 0x00010901
> > > #define PRIV_VERSION_1_10_0 0x00011000
> > >
> > > +#define TRANSLATE_PMP_FAIL 2
> > > #define TRANSLATE_FAIL 1
> > > #define TRANSLATE_SUCCESS 0
> > > #define NB_MMU_MODES 4
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 5a1cd7cf96..00bc4f1712 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -211,6 +211,12 @@ restart:
> > >
> > > /* check that physical address of PTE is legal */
> > > target_ulong pte_addr = base + idx * ptesize;
> > > +
> > > + if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > > + !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
> > > + 1 << MMU_DATA_LOAD, PRV_S)) {
> >
> > Shouldn't we be passing mode in here?
> >
> I actually thought this way at the start. But then I made it PRV_S for
> intentionality; as in PTW (in the current master, without hypervisor
> extensions) always goes under PMP protection in S-Mode.
Yep, you are right, I see this in the spec:
"PMP checks are also applied to page-table accesses for
virtual-address translation, for which the effective privilege mode is
S."
In which case:
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
> This also aligns with Spike implementation here [1].
>
> [1]
> https://github.com/riscv/riscv-isa-sim/blob/8ac902f6ff877e976af434bfe8fa8445930174a1/riscv/mmu.cc#L288
>
>
> > Alistair
> >
> > > + return TRANSLATE_PMP_FAIL;
> > > + }
> > > #if defined(TARGET_RISCV32)
> > > target_ulong pte = ldl_phys(cs->as, pte_addr);
> > > #elif defined(TARGET_RISCV64)
> > > @@ -413,8 +419,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> > > int size,
> > > (ret == TRANSLATE_SUCCESS) &&
> > > !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type,
> > > mode)) {
> > > + ret = TRANSLATE_PMP_FAIL;
> > > + }
> > > + if (ret == TRANSLATE_PMP_FAIL) {
> > > pmp_violation = true;
> > > - ret = TRANSLATE_FAIL;
> > > }
> > > if (ret == TRANSLATE_SUCCESS) {
> > > tlb_set_page(cs, address & TARGET_PAGE_MASK, pa &
> > > TARGET_PAGE_MASK,
> > > --
> > > 2.17.1
> > >
> > >
