* [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured @ 2020-12-23 19:25 Atish Patra 2021-01-07 17:19 ` Alistair Francis 0 siblings, 1 reply; 4+ messages in thread From: Atish Patra @ 2020-12-23 19:25 UTC (permalink / raw) To: qemu-devel, qemu-riscv Cc: Sagar Karandikar, Bastian Koppelmann, Anup Patel, Atish Patra, Alistair Francis, Palmer Dabbelt As per the privilege specification, any access from S/U mode should fail if no pmp region is configured. Signed-off-by: Atish Patra <atish.patra@wdc.com> --- Changes from v2->v1 1. Removed the static from the function definition --- target/riscv/op_helper.c | 5 +++++ target/riscv/pmp.c | 4 ++-- target/riscv/pmp.h | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index d55def76cffd..1eddcb94de7e 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -150,6 +150,11 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb) uint64_t mstatus = env->mstatus; target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP); + + if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); + } + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); mstatus = set_field(mstatus, MSTATUS_MIE, get_field(mstatus, MSTATUS_MPIE)); diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index 2eda8e1e2f07..80d0334e1bfc 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -74,7 +74,7 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index) /* * Count the number of active rules. */ -static inline uint32_t pmp_get_num_rules(CPURISCVState *env) +uint32_t pmp_get_num_rules(CPURISCVState *env) { return env->pmp_state.num_rules; } @@ -237,7 +237,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, /* Short cut if no rules */ if (0 == pmp_get_num_rules(env)) { - return true; + return (env->priv == PRV_M) ? true : false; } if (size == 0) { diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h index 6c6b4c9befe8..c8d5ef4a694e 100644 --- a/target/riscv/pmp.h +++ b/target/riscv/pmp.h @@ -64,5 +64,6 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa, target_ulong *tlb_size); void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index); void pmp_update_rule_nums(CPURISCVState *env); +uint32_t pmp_get_num_rules(CPURISCVState *env); #endif -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured 2020-12-23 19:25 [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured Atish Patra @ 2021-01-07 17:19 ` Alistair Francis 2021-01-07 19:02 ` Atish Patra 0 siblings, 1 reply; 4+ messages in thread From: Alistair Francis @ 2021-01-07 17:19 UTC (permalink / raw) To: Atish Patra Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann, Anup Patel, qemu-devel@nongnu.org Developers, Alistair Francis, Palmer Dabbelt On Wed, Dec 23, 2020 at 11:26 AM Atish Patra <atish.patra@wdc.com> wrote: > > As per the privilege specification, any access from S/U mode should fail > if no pmp region is configured. This doesn't sound right, the spec says: "If no PMP entry matches an S-mode or U-mode access, but at least one PMP entry is implemented, the access fails." I don't see anything saying that an access will fail if there are no PMP regions configred. Alistair > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > --- > Changes from v2->v1 > 1. Removed the static from the function definition > --- > target/riscv/op_helper.c | 5 +++++ > target/riscv/pmp.c | 4 ++-- > target/riscv/pmp.h | 1 + > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index d55def76cffd..1eddcb94de7e 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -150,6 +150,11 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb) > > uint64_t mstatus = env->mstatus; > target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP); > + > + if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); > + } > + > target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > mstatus = set_field(mstatus, MSTATUS_MIE, > get_field(mstatus, MSTATUS_MPIE)); > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 2eda8e1e2f07..80d0334e1bfc 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -74,7 +74,7 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index) > /* > * Count the number of active rules. > */ > -static inline uint32_t pmp_get_num_rules(CPURISCVState *env) > +uint32_t pmp_get_num_rules(CPURISCVState *env) > { > return env->pmp_state.num_rules; > } > @@ -237,7 +237,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > > /* Short cut if no rules */ > if (0 == pmp_get_num_rules(env)) { > - return true; > + return (env->priv == PRV_M) ? true : false; > } > > if (size == 0) { > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h > index 6c6b4c9befe8..c8d5ef4a694e 100644 > --- a/target/riscv/pmp.h > +++ b/target/riscv/pmp.h > @@ -64,5 +64,6 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa, > target_ulong *tlb_size); > void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index); > void pmp_update_rule_nums(CPURISCVState *env); > +uint32_t pmp_get_num_rules(CPURISCVState *env); > > #endif > -- > 2.25.1 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured 2021-01-07 17:19 ` Alistair Francis @ 2021-01-07 19:02 ` Atish Patra 2021-01-08 23:34 ` Alistair Francis 0 siblings, 1 reply; 4+ messages in thread From: Atish Patra @ 2021-01-07 19:02 UTC (permalink / raw) To: alistair23@gmail.com Cc: qemu-riscv@nongnu.org, sagark@eecs.berkeley.edu, kbastian@mail.uni-paderborn.de, Anup Patel, qemu-devel@nongnu.org, Alistair Francis, palmer@dabbelt.com On Thu, 2021-01-07 at 09:19 -0800, Alistair Francis wrote: > On Wed, Dec 23, 2020 at 11:26 AM Atish Patra <atish.patra@wdc.com> > wrote: > > > > As per the privilege specification, any access from S/U mode should > > fail > > if no pmp region is configured. > > This doesn't sound right, the spec says: > > "If no PMP entry matches an S-mode or U-mode access, but at least one > PMP entry is implemented, the access fails." > > I don't see anything saying that an access will fail if there are no > PMP regions configred. > It also says If at least one PMP entry is implemented, but all PMP entries’ A fields are set to OFF, then all S-mode and U-mode memory accesses will fail. My understanding is that if PMP is implemented in hardware, but not configured, S/U-mode memory access should fail. At least that's how hardware behave. > Alistair > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > --- > > Changes from v2->v1 > > 1. Removed the static from the function definition > > --- > > target/riscv/op_helper.c | 5 +++++ > > target/riscv/pmp.c | 4 ++-- > > target/riscv/pmp.h | 1 + > > 3 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > > index d55def76cffd..1eddcb94de7e 100644 > > --- a/target/riscv/op_helper.c > > +++ b/target/riscv/op_helper.c > > @@ -150,6 +150,11 @@ target_ulong helper_mret(CPURISCVState *env, > > target_ulong cpu_pc_deb) > > > > uint64_t mstatus = env->mstatus; > > target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP); > > + > > + if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > > + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, > > GETPC()); > > + } > > + > > target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > > mstatus = set_field(mstatus, MSTATUS_MIE, > > get_field(mstatus, MSTATUS_MPIE)); > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > > index 2eda8e1e2f07..80d0334e1bfc 100644 > > --- a/target/riscv/pmp.c > > +++ b/target/riscv/pmp.c > > @@ -74,7 +74,7 @@ static inline int pmp_is_locked(CPURISCVState > > *env, uint32_t pmp_index) > > /* > > * Count the number of active rules. > > */ > > -static inline uint32_t pmp_get_num_rules(CPURISCVState *env) > > +uint32_t pmp_get_num_rules(CPURISCVState *env) > > { > > return env->pmp_state.num_rules; > > } > > @@ -237,7 +237,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, > > target_ulong addr, > > > > /* Short cut if no rules */ > > if (0 == pmp_get_num_rules(env)) { > > - return true; > > + return (env->priv == PRV_M) ? true : false; > > } > > > > if (size == 0) { > > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h > > index 6c6b4c9befe8..c8d5ef4a694e 100644 > > --- a/target/riscv/pmp.h > > +++ b/target/riscv/pmp.h > > @@ -64,5 +64,6 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, > > hwaddr tlb_sa, > > target_ulong *tlb_size); > > void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index); > > void pmp_update_rule_nums(CPURISCVState *env); > > +uint32_t pmp_get_num_rules(CPURISCVState *env); > > > > #endif > > -- > > 2.25.1 > > > > -- Regards, Atish ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured 2021-01-07 19:02 ` Atish Patra @ 2021-01-08 23:34 ` Alistair Francis 0 siblings, 0 replies; 4+ messages in thread From: Alistair Francis @ 2021-01-08 23:34 UTC (permalink / raw) To: Atish Patra Cc: qemu-riscv@nongnu.org, sagark@eecs.berkeley.edu, kbastian@mail.uni-paderborn.de, Anup Patel, qemu-devel@nongnu.org, Alistair Francis, palmer@dabbelt.com On Thu, Jan 7, 2021 at 11:02 AM Atish Patra <Atish.Patra@wdc.com> wrote: > > On Thu, 2021-01-07 at 09:19 -0800, Alistair Francis wrote: > > On Wed, Dec 23, 2020 at 11:26 AM Atish Patra <atish.patra@wdc.com> > > wrote: > > > > > > As per the privilege specification, any access from S/U mode should > > > fail > > > if no pmp region is configured. > > > > This doesn't sound right, the spec says: > > > > "If no PMP entry matches an S-mode or U-mode access, but at least one > > PMP entry is implemented, the access fails." > > > > I don't see anything saying that an access will fail if there are no > > PMP regions configred. > > > > It also says > > If at least one PMP entry is implemented, but all PMP entries’ A fields > are set to OFF, then all S-mode and U-mode memory accesses will fail. > > My understanding is that if PMP is implemented in hardware, but not > configured, S/U-mode memory access should fail. At least that's how > hardware behave. Ah, I misinterpreted what implemented means. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Applied to riscv-to-apply.next Alistair > > > Alistair > > > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > --- > > > Changes from v2->v1 > > > 1. Removed the static from the function definition > > > --- > > > target/riscv/op_helper.c | 5 +++++ > > > target/riscv/pmp.c | 4 ++-- > > > target/riscv/pmp.h | 1 + > > > 3 files changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > > > index d55def76cffd..1eddcb94de7e 100644 > > > --- a/target/riscv/op_helper.c > > > +++ b/target/riscv/op_helper.c > > > @@ -150,6 +150,11 @@ target_ulong helper_mret(CPURISCVState *env, > > > target_ulong cpu_pc_deb) > > > > > > uint64_t mstatus = env->mstatus; > > > target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP); > > > + > > > + if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > > > + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, > > > GETPC()); > > > + } > > > + > > > target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > > > mstatus = set_field(mstatus, MSTATUS_MIE, > > > get_field(mstatus, MSTATUS_MPIE)); > > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > > > index 2eda8e1e2f07..80d0334e1bfc 100644 > > > --- a/target/riscv/pmp.c > > > +++ b/target/riscv/pmp.c > > > @@ -74,7 +74,7 @@ static inline int pmp_is_locked(CPURISCVState > > > *env, uint32_t pmp_index) > > > /* > > > * Count the number of active rules. > > > */ > > > -static inline uint32_t pmp_get_num_rules(CPURISCVState *env) > > > +uint32_t pmp_get_num_rules(CPURISCVState *env) > > > { > > > return env->pmp_state.num_rules; > > > } > > > @@ -237,7 +237,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, > > > target_ulong addr, > > > > > > /* Short cut if no rules */ > > > if (0 == pmp_get_num_rules(env)) { > > > - return true; > > > + return (env->priv == PRV_M) ? true : false; > > > } > > > > > > if (size == 0) { > > > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h > > > index 6c6b4c9befe8..c8d5ef4a694e 100644 > > > --- a/target/riscv/pmp.h > > > +++ b/target/riscv/pmp.h > > > @@ -64,5 +64,6 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, > > > hwaddr tlb_sa, > > > target_ulong *tlb_size); > > > void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index); > > > void pmp_update_rule_nums(CPURISCVState *env); > > > +uint32_t pmp_get_num_rules(CPURISCVState *env); > > > > > > #endif > > > -- > > > 2.25.1 > > > > > > > > -- > Regards, > Atish ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-08 23:35 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-23 19:25 [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured Atish Patra 2021-01-07 17:19 ` Alistair Francis 2021-01-07 19:02 ` Atish Patra 2021-01-08 23:34 ` Alistair Francis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).