| Message ID | 20211214092659.15709-1-nikita.shubin@maquefel.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv/pmp: fix no pmp illegal intrs | expand |
On 12/14/21 1:26 AM, Nikita Shubin wrote: > - if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > + if (riscv_feature(env, RISCV_FEATURE_PMP) && > + !pmp_get_num_rules(env) && (prev_priv != PRV_M)) { When would the number of rules become non-zero with PMP disabled? When does this test make a difference? r~
On 12/14/21 1:12 PM, Richard Henderson wrote: > On 12/14/21 1:26 AM, Nikita Shubin wrote: >> - if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { >> + if (riscv_feature(env, RISCV_FEATURE_PMP) && >> + !pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > > When would the number of rules become non-zero with PMP disabled? > When does this test make a difference? Oh, nevermind, I see what you mean. r~
Hello Richard! On Tue, 14 Dec 2021 13:13:57 -0800 Richard Henderson <richard.henderson@linaro.org> wrote: > On 12/14/21 1:12 PM, Richard Henderson wrote: > > On 12/14/21 1:26 AM, Nikita Shubin wrote: > >> - if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > >> + if (riscv_feature(env, RISCV_FEATURE_PMP) && > >> + !pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > > > > When would the number of rules become non-zero with PMP disabled? > > When does this test make a difference? > > Oh, nevermind, I see what you mean. > Np, let me explain in details: The ISA states: > Platforms vary widely in demands for physical memory protection, and > some platforms may provide other PMP structures in addition to or > instead of the scheme described in this section. So we might don't have PMP at all, but if we set qdev_prop_set_bit(DEVICE(obj), "pmp", false); for some CPU we still end up in illegal inst on mret, cause we get pmp_get_num_rules(env) == 0, becouse we have no PMP which leads to zero available rules. > > r~ >
On Wed, Dec 15, 2021 at 1:00 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > As per the privilege specification, any access from S/U mode should fail > if no pmp region is configured and pmp is present, othwerwise access > should succeed. > > Fixes: d102f19a208 (target/riscv/pmp: Raise exception if no PMP entry is configured) > Signed-off-by: Nikita Shubin <n.shubin@yadro.com> Whoops! I sent a patch to fix the exact same issue :) I'll drop mine and we can merge yours. Do you mind adding this and resending the patch Resolves: https://gitlab.com/qemu-project/qemu/-/issues/585 Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/op_helper.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index ee7c24efe7..58d992e98a 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -146,7 +146,8 @@ 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)) { > + if (riscv_feature(env, RISCV_FEATURE_PMP) && > + !pmp_get_num_rules(env) && (prev_priv != PRV_M)) { > riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); > } > > -- > 2.31.1 > >
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index ee7c24efe7..58d992e98a 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -146,7 +146,8 @@ 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)) { + if (riscv_feature(env, RISCV_FEATURE_PMP) && + !pmp_get_num_rules(env) && (prev_priv != PRV_M)) { riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); }