Re: [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

On Thu, 30 May 2019 at 05:07, Alistair Francis <address@hidden> wrote:
>
> On Wed, May 22, 2019 at 2:27 AM Hesham Almatary
> <address@hidden> wrote:
> >
> > On Tue, 21 May 2019 at 23:40, Alistair Francis <address@hidden> wrote:
> > >
> > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > > <address@hidden> 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 <address@hidden>
> > > > Signed-off-by: Hesham Almatary <address@hidden>
> > > > ---
> > > >  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 7c7282c680..d0b0f9cf88 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)) {
> > >
> > > I see a problem here.
> > >
> > > pmp_hart_has_privs() checks permissions based on the current value of
> > > env->priv. This might not always be the correct permissions to check,
> > > we should instead use the current mode to check permissions.
> > >
> > That is not clear to me. Isn't env->priv the current privildge mode?
> > Could you please elaborate on what other cases this might not be the case?
>
> Sorry for the delay. The RISC-V Hypervisor Extension allows load/store
> operations to be carried out as a previous privilege. The mstatus.MPRV
> and hstatus.SPRV allow this.
>
No problem, thanks for the clarification.
You are right, I haven't considered MPRV. I fixed that in a separate
commit in a v4 series of patches.
> Alistair
>
> >
> > > The best way to do this to me is to probably provide a privileged mode
> > > override to the function, can you add that?
> > >
> > > Alistair
> > >
> > > > +            return TRANSLATE_PMP_FAIL;
> > > > +        }
> > > >  #if defined(TARGET_RISCV32)
> > > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > > >  #elif defined(TARGET_RISCV64)
> > > > @@ -405,8 +411,10 @@ 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)) {
> > > > +        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
> > > >
> > > >