* [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured
@ 2020-12-23 19:25 ` Atish Patra
0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured
@ 2020-12-23 19:25 ` Atish Patra
0 siblings, 0 replies; 8+ messages in thread
From: Atish Patra @ 2020-12-23 19:25 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: Atish Patra, Alistair Francis, Bastian Koppelmann,
Palmer Dabbelt, Sagar Karandikar, Anup Patel
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] 8+ messages in thread
* Re: [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
-1 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured
@ 2021-01-07 17:19 ` Alistair Francis
0 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2021-01-07 17:19 UTC (permalink / raw)
To: Atish Patra
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Bastian Koppelmann, Anup Patel,
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] 8+ 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
-1 siblings, 0 replies; 8+ messages in thread
From: Atish Patra @ 2021-01-07 19:02 UTC (permalink / raw)
To: alistair23
Cc: qemu-riscv, sagark, kbastian, Anup Patel, qemu-devel,
Alistair Francis, palmer
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] 8+ messages in thread
* Re: [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured
@ 2021-01-07 19:02 ` Atish Patra
0 siblings, 0 replies; 8+ messages in thread
From: Atish Patra @ 2021-01-07 19:02 UTC (permalink / raw)
To: alistair23
Cc: qemu-riscv, kbastian, sagark, Alistair Francis, qemu-devel,
Anup Patel, palmer
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] 8+ 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
-1 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2021-01-08 23:34 UTC (permalink / raw)
To: Atish Patra
Cc: qemu-riscv, sagark, kbastian, Anup Patel, qemu-devel,
Alistair Francis, palmer
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] 8+ messages in thread
* Re: [PATCH v2] target/riscv/pmp: Raise exception if no PMP entry is configured
@ 2021-01-08 23:34 ` Alistair Francis
0 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2021-01-08 23:34 UTC (permalink / raw)
To: Atish Patra
Cc: qemu-riscv, kbastian, sagark, Alistair Francis, qemu-devel,
Anup Patel, palmer
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] 8+ messages in thread
end of thread, other threads:[~2021-01-08 23:35 UTC | newest]
Thread overview: 8+ 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
2020-12-23 19:25 ` Atish Patra
2021-01-07 17:19 ` Alistair Francis
2021-01-07 17:19 ` Alistair Francis
2021-01-07 19:02 ` Atish Patra
2021-01-07 19:02 ` Atish Patra
2021-01-08 23:34 ` Alistair Francis
2021-01-08 23:34 ` Alistair Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.