| Message ID | 20220906122243.1243354-2-christoph.muellner@vrull.eu |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for the T-Head vendor extensions | expand |
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei On 2022/9/6 20:22, Christoph Muellner wrote: > From: Christoph Müllner <christoph.muellner@vrull.eu> > > This allows privileged instructions to check the required > privilege level in the translation without calling a helper. > > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> > --- > target/riscv/translate.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 63b04e8a94..fd241ff667 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -59,6 +59,9 @@ typedef struct DisasContext { > /* pc_succ_insn points to the instruction following base.pc_next */ > target_ulong pc_succ_insn; > target_ulong priv_ver; > +#ifndef CONFIG_USER_ONLY > + target_ulong priv; > +#endif > RISCVMXL misa_mxl_max; > RISCVMXL xl; > uint32_t misa_ext; > @@ -1079,6 +1082,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS; > ctx->priv_ver = env->priv_ver; > #if !defined(CONFIG_USER_ONLY) > + ctx->priv = env->priv; > if (riscv_has_ext(env, RVH)) { > ctx->virt_enabled = riscv_cpu_virt_enabled(env); > } else {
On 9/6/22 14:22, Christoph Muellner wrote: > From: Christoph Müllner <christoph.muellner@vrull.eu> > > This allows privileged instructions to check the required > privilege level in the translation without calling a helper. > > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> > --- > target/riscv/translate.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 63b04e8a94..fd241ff667 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -59,6 +59,9 @@ typedef struct DisasContext { > /* pc_succ_insn points to the instruction following base.pc_next */ > target_ulong pc_succ_insn; > target_ulong priv_ver; > +#ifndef CONFIG_USER_ONLY > + target_ulong priv; > +#endif > RISCVMXL misa_mxl_max; > RISCVMXL xl; > uint32_t misa_ext; > @@ -1079,6 +1082,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS; > ctx->priv_ver = env->priv_ver; > #if !defined(CONFIG_USER_ONLY) > + ctx->priv = env->priv; Reading directly from env here is, in general, wrong. Items that are constant, like priv_ver, are ok. But otherwise these values must be recorded by cpu_get_tb_cpu_state(). This instance will happen to work, because the execution context is already constrained. In this case because env->priv == ctx->mem_idx (see cpu_mmu_index) so, really, you don't need this new field at all. Or, keep the field, because it's usage will be more self-documentary, but copy the value from ctx->mmu_idx and add a comment. > if (riscv_has_ext(env, RVH)) { > ctx->virt_enabled = riscv_cpu_virt_enabled(env); > } else { Incidentally, this (existing) usage appears to be a bug. I can see nothing in cpu_get_tb_cpu_state that corresponds directly to this value. r~
On 9/16/22 08:00, Richard Henderson wrote: > Or, keep the field, because it's usage will be more self-documentary, but copy the value > from ctx->mmu_idx and add a comment. Or, add an inline function like static inline int priv_level(DisasContext *ctx) { #ifdef CONFIG_USER_ONLY return PRV_U; #else /* Priv level equals mmu index -- see cpu_mmu_index. */ return ctx->mmu_idx; #endif } so that usages within a user-only build are compile-time constant and folded away. r~
On 2022/9/16 14:00, Richard Henderson wrote: > On 9/6/22 14:22, Christoph Muellner wrote: >> From: Christoph Müllner <christoph.muellner@vrull.eu> >> >> This allows privileged instructions to check the required >> privilege level in the translation without calling a helper. >> >> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> >> --- >> target/riscv/translate.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target/riscv/translate.c b/target/riscv/translate.c >> index 63b04e8a94..fd241ff667 100644 >> --- a/target/riscv/translate.c >> +++ b/target/riscv/translate.c >> @@ -59,6 +59,9 @@ typedef struct DisasContext { >> /* pc_succ_insn points to the instruction following >> base.pc_next */ >> target_ulong pc_succ_insn; >> target_ulong priv_ver; >> +#ifndef CONFIG_USER_ONLY >> + target_ulong priv; >> +#endif >> RISCVMXL misa_mxl_max; >> RISCVMXL xl; >> uint32_t misa_ext; >> @@ -1079,6 +1082,7 @@ static void >> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >> ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS; >> ctx->priv_ver = env->priv_ver; >> #if !defined(CONFIG_USER_ONLY) >> + ctx->priv = env->priv; > > Reading directly from env here is, in general, wrong. Items that are > constant, like priv_ver, are ok. But otherwise these values must be > recorded by cpu_get_tb_cpu_state(). > > This instance will happen to work, because the execution context is > already constrained. Exactly. Thanks for pointing it out. > In this case because env->priv == ctx->mem_idx (see cpu_mmu_index) so, > really, you don't need this new field at all. Or, keep the field, > because it's usage will be more self-documentary, but copy the value > from ctx->mmu_idx and add a comment. > > >> if (riscv_has_ext(env, RVH)) { >> ctx->virt_enabled = riscv_cpu_virt_enabled(env); >> } else { > > Incidentally, this (existing) usage appears to be a bug. I can see > nothing in cpu_get_tb_cpu_state that corresponds directly to this value. Agree. Zhiwei > > > r~
diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 63b04e8a94..fd241ff667 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -59,6 +59,9 @@ typedef struct DisasContext { /* pc_succ_insn points to the instruction following base.pc_next */ target_ulong pc_succ_insn; target_ulong priv_ver; +#ifndef CONFIG_USER_ONLY + target_ulong priv; +#endif RISCVMXL misa_mxl_max; RISCVMXL xl; uint32_t misa_ext; @@ -1079,6 +1082,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS; ctx->priv_ver = env->priv_ver; #if !defined(CONFIG_USER_ONLY) + ctx->priv = env->priv; if (riscv_has_ext(env, RVH)) { ctx->virt_enabled = riscv_cpu_virt_enabled(env); } else {