| Message ID | 20190521104324.12835-3-Hesham.Almatary@cl.cam.ac.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv3,1/5] RISC-V: Only Check PMP if MMU translation succeeds | expand |
On Tue, May 21, 2019 at 3:44 AM Hesham Almatary <Hesham.Almatary@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 <fintelia@gmail.com> > Signed-off-by: Hesham Almatary <Hesham.Almatary@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 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. 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 > >
On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote: > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary > <Hesham.Almatary@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 <fintelia@gmail.com> > > Signed-off-by: Hesham Almatary <Hesham.Almatary@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 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? > 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 > > > >
ping On Wed, 22 May 2019 at 11:26, Hesham Almatary <hesham.almatary@cl.cam.ac.uk> wrote: > > On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote: > > > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary > > <Hesham.Almatary@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 <fintelia@gmail.com> > > > Signed-off-by: Hesham Almatary <Hesham.Almatary@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 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? > > > 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 > > > > > >
On Wed, May 22, 2019 at 2:27 AM Hesham Almatary <hesham.almatary@cl.cam.ac.uk> wrote: > > On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote: > > > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary > > <Hesham.Almatary@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 <fintelia@gmail.com> > > > Signed-off-by: Hesham Almatary <Hesham.Almatary@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 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. 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 > > > > > >
On Thu, 30 May 2019 at 05:07, Alistair Francis <alistair23@gmail.com> wrote: > > On Wed, May 22, 2019 at 2:27 AM Hesham Almatary > <hesham.almatary@cl.cam.ac.uk> wrote: > > > > On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary > > > <Hesham.Almatary@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 <fintelia@gmail.com> > > > > Signed-off-by: Hesham Almatary <Hesham.Almatary@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 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 > > > > > > > >
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)) { + 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,
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 <fintelia@gmail.com> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk> --- target/riscv/cpu.h | 1 + target/riscv/cpu_helper.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) -- 2.17.1